plugins: improve remove command to support multiple plugins (#17030)

Removal works in a single pass by finding plugins that would have unmet
dependencies if all of the specified plugins were to be removed, and
proceeding with the removal only if no conflicts were created.

> ~~~
> ╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-remove-multiple ✘) }
> ╰─● bin/logstash-plugin remove logstash-input-syslog logstash-filter-grok
> Using system java: /Users/rye/.jenv/shims/java
> Resolving dependencies......
> Successfully removed logstash-input-syslog
> Successfully removed logstash-filter-grok
> [success (00:00:05)]
~~~

lib/pluginmanager/bundler/logstash_uninstall.rb

@@ -34,54 +34,50 @@ module Bundler
       @lockfile_path = lockfile_path
     end
 
-    # To be uninstalled the candidate gems need to be standalone.
+    def uninstall!(gems_to_remove)
-    def dependants_gems(gem_name)
+      gems_to_remove = Array(gems_to_remove)
+
+      unsatisfied_dependency_mapping = Dsl.evaluate(gemfile_path, lockfile_path, {}).specs.each_with_object({}) do |spec, memo|
+        next if gems_to_remove.include?(spec.name)
+        deps = spec.runtime_dependencies.collect(&:name)
+        deps.intersection(gems_to_remove).each do |missing_dependency|
+          memo[missing_dependency] ||= []
+          memo[missing_dependency] << spec.name
+        end
+      end
+      if unsatisfied_dependency_mapping.any?
+        unsatisfied_dependency_mapping.each { |gem_to_remove, gems_depending_on_removed| display_cant_remove_message(gem_to_remove, gems_depending_on_removed) }
+        LogStash::PluginManager.ui.info("No plugins were removed.")
+        return false
+      end
+
+      with_mutable_gemfile do |gemfile|
+        gems_to_remove.each { |gem_name| gemfile.remove(gem_name) }
+
         builder = Dsl.new
-      builder.eval_gemfile(::File.join(::File.dirname(gemfile_path), "original gemfile"), File.read(gemfile_path))
-      definition = builder.to_definition(lockfile_path, {})
-
-      definition.specs
-        .select { |spec| spec.dependencies.collect(&:name).include?(gem_name) }
-        .collect(&:name).sort.uniq
-    end
-
-    def uninstall!(gem_name)
-      unfreeze_gemfile do
-        dependencies_from = dependants_gems(gem_name)
-
-        if dependencies_from.size > 0
-          display_cant_remove_message(gem_name, dependencies_from)
-          false
-        else
-          remove_gem(gem_name)
-          true
-        end
-      end
-    end
-
-    def remove_gem(gem_name)
-      builder = Dsl.new
-      file = File.new(gemfile_path, "r+")
-
-      gemfile = LogStash::Gemfile.new(file).load
-      gemfile.remove(gem_name)
         builder.eval_gemfile(::File.join(::File.dirname(gemfile_path), "gemfile to changes"), gemfile.generate)
 
+        # build a definition, providing an intentionally-empty "unlock" mapping
+        # to ensure that all gem versions remain locked
         definition = builder.to_definition(lockfile_path, {})
+
+        # lock the definition and save our modified gemfile
         definition.lock(lockfile_path)
         gemfile.save
 
+        gems_to_remove.each do |gem_name|
           LogStash::PluginManager.ui.info("Successfully removed #{gem_name}")
-    ensure
+        end
-      file.close if file
+
+        return true
+      end
     end
 
     def display_cant_remove_message(gem_name, dependencies_from)
-        message = <<-eos
+        message = <<~EOS
           Failed to remove \"#{gem_name}\" because the following plugins or libraries depend on it:
-
           * #{dependencies_from.join("\n* ")}
-        eos
+        EOS
         LogStash::PluginManager.ui.info(message)
     end
 
@@ -93,10 +89,22 @@ Failed to remove \"#{gem_name}\" because the following plugins or libraries depe
       end
     end
 
-    def self.uninstall!(gem_name, options = { :gemfile => LogStash::Environment::GEMFILE, :lockfile => LogStash::Environment::LOCKFILE })
+    ##
-      gemfile_path = options[:gemfile]
+    # Yields a mutable gemfile backed by an open, writable file handle.
-      lockfile_path = options[:lockfile]
+    # It is the responsibility of the caller to send `LogStash::Gemfile#save` to persist the result.
-      LogstashUninstall.new(gemfile_path, lockfile_path).uninstall!(gem_name)
+    # @yieldparam [LogStash::Gemfile]
+    def with_mutable_gemfile
+      unfreeze_gemfile do
+        File.open(gemfile_path, 'r+') do |file|
+          yield LogStash::Gemfile.new(file).load
+        end
+      end
+    end
+
+    def self.uninstall!(gem_names, options={})
+      gemfile_path = options[:gemfile] || LogStash::Environment::GEMFILE
+      lockfile_path = options[:lockfile] || LogStash::Environment::LOCKFILE
+      LogstashUninstall.new(gemfile_path, lockfile_path).uninstall!(Array(gem_names))
     end
   end
 end

lib/pluginmanager/remove.rb

@@ -20,13 +20,15 @@ require "pluginmanager/x_pack_interceptor.rb"
 require "pluginmanager/command"
 
 class LogStash::PluginManager::Remove < LogStash::PluginManager::Command
-  parameter "PLUGIN", "plugin name"
+  parameter "PLUGIN ...", "plugin name(s)"
 
   def execute
+    signal_error("One or more plugins must be specified") if plugin_list.empty?
     signal_error("File #{LogStash::Environment::GEMFILE_PATH} does not exist or is not writable, aborting") unless File.writable?(LogStash::Environment::GEMFILE_PATH)
 
     LogStash::Bundler.prepare({:without => [:build, :development]})
 
+    plugin_list.each do |plugin|
       if LogStash::PluginManager::ALIASES.has_key?(plugin)
         unless LogStash::PluginManager.installed_plugin?(plugin, gemfile)
           aliased_plugin = LogStash::PluginManager::ALIASES[plugin]
@@ -41,10 +43,10 @@ class LogStash::PluginManager::Remove < LogStash::PluginManager::Command
 
       # if the plugin is provided by an integration plugin. abort.
       if integration_plugin = LogStash::PluginManager.which_integration_plugin_provides(plugin, gemfile)
-      signal_error("This plugin is already provided by '#{integration_plugin.name}' so it can't be removed individually")
+        signal_error("The plugin `#{plugin}` is provided by '#{integration_plugin.name}' so it can't be removed individually")
       end
 
-    not_installed_message = "This plugin has not been previously installed"
+      not_installed_message = "The plugin `#{plugin}` has not been previously installed"
       plugin_gem_spec = LogStash::PluginManager.find_plugins_gem_specs(plugin).any?
       if plugin_gem_spec
         # make sure this is an installed plugin and present in Gemfile.
@@ -60,8 +62,9 @@ class LogStash::PluginManager::Remove < LogStash::PluginManager::Command
         local_gem = gemfile.locally_installed_gems.detect { |local_gem| local_gem.name == plugin }
         signal_error(not_installed_message) unless local_gem
       end
+    end
 
-    exit(1) unless ::Bundler::LogstashUninstall.uninstall!(plugin)
+    exit(1) unless ::Bundler::LogstashUninstall.uninstall!(plugin_list)
     LogStash::Bundler.genericize_platform
     remove_unused_locally_installed_gems!
   rescue => exception

qa/integration/fixtures/plugins/generate-gems.sh

@@ -0,0 +1,4 @@
+#!/usr/bin/env sh
+
+cd "$( dirname "$0" )"
+find . -name '*.gemspec' | xargs -n1 gem build

qa/integration/fixtures/plugins/logstash-filter-four_depends_on_one_and_three.gemspec

@@ -0,0 +1,26 @@
+Gem::Specification.new do |s|
+  s.name            = File.basename(__FILE__, ".gemspec")
+  s.version         = '0.1.1'
+  s.licenses        = ['Apache-2.0']
+  s.summary         = "A dummy plugin with two plugin dependencies"
+  s.description     = "This plugin is only used in the acceptance test"
+  s.authors         = ["Elasticsearch"]
+  s.email           = 'info@elasticsearch.com'
+  s.homepage        = "http://www.elasticsearch.org/guide/en/logstash/current/index.html"
+  s.require_paths = ["lib"]
+
+  # Files
+  s.files = [__FILE__]
+
+  # Tests
+  s.test_files = []
+
+  # Special flag to let us know this is actually a logstash plugin
+  s.metadata = { "logstash_plugin" => "true", "logstash_group" => "filter" }
+
+  # Gem dependencies
+  s.add_runtime_dependency "logstash-core-plugin-api", ">= 1.60", "<= 2.99"
+
+  s.add_runtime_dependency "logstash-filter-one_no_dependencies", "~> 0.1"
+  s.add_runtime_dependency "logstash-filter-three_no_dependencies", "~> 0.1"
+end

qa/integration/fixtures/plugins/logstash-filter-one_no_dependencies.gemspec

@@ -0,0 +1,23 @@
+Gem::Specification.new do |s|
+  s.name            = File.basename(__FILE__, ".gemspec")
+  s.version         = '0.1.1'
+  s.licenses        = ['Apache-2.0']
+  s.summary         = "A dummy plugin with no dependencies"
+  s.description     = "This plugin is only used in the acceptance test"
+  s.authors         = ["Elasticsearch"]
+  s.email           = 'info@elasticsearch.com'
+  s.homepage        = "http://www.elasticsearch.org/guide/en/logstash/current/index.html"
+  s.require_paths = ["lib"]
+
+  # Files
+  s.files = [__FILE__]
+
+  # Tests
+  s.test_files = []
+
+  # Special flag to let us know this is actually a logstash plugin
+  s.metadata = { "logstash_plugin" => "true", "logstash_group" => "filter" }
+
+  # Gem dependencies
+  s.add_runtime_dependency "logstash-core-plugin-api", ">= 1.60", "<= 2.99"
+end

qa/integration/fixtures/plugins/logstash-filter-three_no_dependencies.gemspec

@@ -0,0 +1,23 @@
+Gem::Specification.new do |s|
+  s.name            = File.basename(__FILE__, ".gemspec")
+  s.version         = '0.1.1'
+  s.licenses        = ['Apache-2.0']
+  s.summary         = "A dummy plugin with no dependencies"
+  s.description     = "This plugin is only used in the acceptance test"
+  s.authors         = ["Elasticsearch"]
+  s.email           = 'info@elasticsearch.com'
+  s.homepage        = "http://www.elasticsearch.org/guide/en/logstash/current/index.html"
+  s.require_paths = ["lib"]
+
+  # Files
+  s.files = [__FILE__]
+
+  # Tests
+  s.test_files = []
+
+  # Special flag to let us know this is actually a logstash plugin
+  s.metadata = { "logstash_plugin" => "true", "logstash_group" => "filter" }
+
+  # Gem dependencies
+  s.add_runtime_dependency "logstash-core-plugin-api", ">= 1.60", "<= 2.99"
+end

qa/integration/fixtures/plugins/logstash-filter-two_depends_on_one.gemspec

@@ -0,0 +1,25 @@
+Gem::Specification.new do |s|
+  s.name            = File.basename(__FILE__, ".gemspec")
+  s.version         = '0.1.1'
+  s.licenses        = ['Apache-2.0']
+  s.summary         = "A dummy plugin with one plugin dependency"
+  s.description     = "This plugin is only used in the acceptance test"
+  s.authors         = ["Elasticsearch"]
+  s.email           = 'info@elasticsearch.com'
+  s.homepage        = "http://www.elasticsearch.org/guide/en/logstash/current/index.html"
+  s.require_paths = ["lib"]
+
+  # Files
+  s.files = [__FILE__]
+
+  # Tests
+  s.test_files = []
+
+  # Special flag to let us know this is actually a logstash plugin
+  s.metadata = { "logstash_plugin" => "true", "logstash_group" => "filter" }
+
+  # Gem dependencies
+  s.add_runtime_dependency "logstash-core-plugin-api", ">= 1.60", "<= 2.99"
+
+  s.add_runtime_dependency "logstash-filter-one_no_dependencies", "~> 0.1"
+end

qa/integration/fixtures/plugins/logstash-filter-zero_no_dependencies.gemspec

@@ -0,0 +1,23 @@
+Gem::Specification.new do |s|
+  s.name            = File.basename(__FILE__, ".gemspec")
+  s.version         = '0.1.1'
+  s.licenses        = ['Apache-2.0']
+  s.summary         = "A dummy plugin with no dependencies"
+  s.description     = "This plugin is only used in the acceptance test"
+  s.authors         = ["Elasticsearch"]
+  s.email           = 'info@elasticsearch.com'
+  s.homepage        = "http://www.elasticsearch.org/guide/en/logstash/current/index.html"
+  s.require_paths = ["lib"]
+
+  # Files
+  s.files = [__FILE__]
+
+  # Tests
+  s.test_files = []
+
+  # Special flag to let us know this is actually a logstash plugin
+  s.metadata = { "logstash_plugin" => "true", "logstash_group" => "filter" }
+
+  # Gem dependencies
+  s.add_runtime_dependency "logstash-core-plugin-api", ">= 1.60", "<= 2.99"
+end

qa/integration/services/logstash_service.rb

@@ -20,6 +20,7 @@ require_relative "monitoring_api"
 require "childprocess"
 require "bundler"
 require "socket"
+require "shellwords"
 require "tempfile"
 require 'yaml'
 
@@ -337,8 +338,9 @@ class LogstashService < Service
       @logstash_plugin = File.join(@logstash.logstash_home, LOGSTASH_PLUGIN)
     end
 
-    def remove(plugin_name)
+    def remove(plugin_name, *additional_plugins)
-      run("remove #{plugin_name}")
+      plugin_list = Shellwords.shelljoin([plugin_name]+additional_plugins)
+      run("remove #{plugin_list}")
     end
 
     def prepare_offline_pack(plugins, output_zip = nil)
@@ -351,12 +353,16 @@ class LogstashService < Service
       end
     end
 
-    def list(plugin_name, verbose = false)
+    def list(*plugins, verbose: false)
-      run("list #{plugin_name} #{verbose ? "--verbose" : ""}")
+      command = "list"
+      command << " --verbose" if verbose
+      command << " #{Shellwords.shelljoin(plugins)}" if plugins.any?
+      run(command)
     end
 
-    def install(plugin_name)
+    def install(plugin_name, *additional_plugins)
-      run("install #{plugin_name}")
+      plugin_list = ([plugin_name]+additional_plugins).flatten
+      run("install #{Shellwords.shelljoin(plugin_list)}")
     end
 
     def run(command)
@@ -364,7 +370,7 @@ class LogstashService < Service
     end
 
     def run_raw(cmd, change_dir = true, environment = {})
-      @logstash.run_cmd(cmd.split(' '), change_dir, environment)
+      @logstash.run_cmd(Shellwords.shellsplit(cmd), change_dir, environment)
     end
   end
 end

qa/integration/specs/cli/remove_spec.rb

@@ -22,7 +22,7 @@ require_relative '../../framework/helpers'
 require "logstash/devutils/rspec/spec_helper"
 
 describe "CLI > logstash-plugin remove" do
-  before(:all) do
+  before(:each) do
     @fixture = Fixture.new(__FILE__)
     @logstash_plugin = @fixture.get_service("logstash").plugin_cli
   end
@@ -109,5 +109,131 @@ describe "CLI > logstash-plugin remove" do
           expect(presence_check.stderr_and_stdout).to match(/logstash-codec-json/)
         end
       end
+
+      context "multiple plugins" do
+
+        let(:setup_plugin_list) do
+          fail("spec must override `setup_plugin_list`")
+        end
+
+        before(:each) do
+          if setup_plugin_list.any?
+            search_dir = File.expand_path(File.join(__dir__, "..", "..", "fixtures", "plugins"))
+            plugin_paths = []
+
+            aggregate_failures('setup: resolve plugin paths') do
+              setup_plugin_list.each do |requested_plugin|
+                found = Dir.glob(File.join(search_dir, "#{requested_plugin}-*.gem"))
+                expect(found).to have_attributes(:size => 1), lambda { "expected exactly one `#{requested_plugin}` in `#{search_dir}`, got #{found.inspect}" }
+                plugin_paths << found.first
+              end
+            end
+
+            aggregate_failures('setup: installing plugins') do
+              puts "installing plugins #{plugin_paths.inspect}"
+              outcome = @logstash_plugin.install(*plugin_paths)
+
+              expect(outcome.exit_code).to eq(0)
+              expect(outcome.stderr_and_stdout).to match(/Installation successful/)
+            end
+          end
+        end
+
+        context "when a remaining plugin has a dependency on a removed plugin" do
+          let(:setup_plugin_list) do
+            %w(
+              logstash-filter-zero_no_dependencies
+              logstash-filter-one_no_dependencies
+              logstash-filter-two_depends_on_one
+              logstash-filter-three_no_dependencies
+              logstash-filter-four_depends_on_one_and_three
+            )
+          end
+          it "errors helpfully without removing any of the plugins" do
+            execute = @logstash_plugin.remove("logstash-filter-three_no_dependencies", "logstash-filter-zero_no_dependencies")
+
+            expect(execute.exit_code).to eq(1)
+            expect(execute.stderr_and_stdout).to include('Failed to remove "logstash-filter-three_no_dependencies"')
+            expect(execute.stderr_and_stdout).to include("* logstash-filter-four_depends_on_one_and_three") # one of the dependency
+            expect(execute.stderr_and_stdout).to include("No plugins were removed.")
+
+            aggregate_failures("list plugins") do
+              presence_check = @logstash_plugin.list
+              expect(presence_check.exit_code).to eq(0)
+              expect(presence_check.stderr_and_stdout).to include('logstash-filter-three_no_dependencies')
+              expect(presence_check.stderr_and_stdout).to include('logstash-filter-zero_no_dependencies')
+            end
+          end
+        end
+        context "when multiple remaining plugins have a dependency on a removed plugin" do
+          let(:setup_plugin_list) do
+            %w(
+              logstash-filter-zero_no_dependencies
+              logstash-filter-one_no_dependencies
+              logstash-filter-two_depends_on_one
+              logstash-filter-three_no_dependencies
+              logstash-filter-four_depends_on_one_and_three
+            )
+          end
+          it "errors helpfully without removing any of the plugins" do
+            execute = @logstash_plugin.remove("logstash-filter-one_no_dependencies", "logstash-filter-zero_no_dependencies")
+
+            expect(execute.exit_code).to eq(1)
+            expect(execute.stderr_and_stdout).to include('Failed to remove "logstash-filter-one_no_dependencies"')
+            expect(execute.stderr_and_stdout).to include("* logstash-filter-four_depends_on_one_and_three") # one of the dependency
+            expect(execute.stderr_and_stdout).to include("* logstash-filter-two_depends_on_one") # one of the dependency
+            expect(execute.stderr_and_stdout).to include("No plugins were removed.")
+
+            aggregate_failures("list plugins") do
+              presence_check = @logstash_plugin.list
+              expect(presence_check.exit_code).to eq(0)
+              expect(presence_check.stderr_and_stdout).to include('logstash-filter-one_no_dependencies')
+              expect(presence_check.stderr_and_stdout).to include('logstash-filter-zero_no_dependencies')
+            end
+          end
+        end
+        context "when removing plugins and all plugins that depend on them" do
+          let(:setup_plugin_list) do
+            %w(
+              logstash-filter-zero_no_dependencies
+              logstash-filter-one_no_dependencies
+              logstash-filter-two_depends_on_one
+              logstash-filter-three_no_dependencies
+              logstash-filter-four_depends_on_one_and_three
+            )
+          end
+          it "removes the plugins" do
+            plugins_to_remove = %w(
+              logstash-filter-one_no_dependencies
+              logstash-filter-two_depends_on_one
+              logstash-filter-three_no_dependencies
+              logstash-filter-four_depends_on_one_and_three
+            ).shuffle #random order
+            execute = @logstash_plugin.remove(*plugins_to_remove)
+
+            aggregate_failures("removal action") do
+              expect(execute).to have_attributes(:exit_code => 0, :stderr_and_stdout => include("Success"))
+              plugins_to_remove.each do |gem_name|
+                expect(execute.stderr_and_stdout).to include("Successfully removed #{gem_name}")
+              end
+            end
+
+            aggregate_failures("list plugins") do
+              presence_check = @logstash_plugin.list
+              expect(presence_check.exit_code).to eq(0)
+              aggregate_failures("removed plugins") do
+                plugins_to_remove.each do |expected_removed_plugin|
+                  expect(presence_check.stderr_and_stdout).to_not include(expected_removed_plugin)
+                end
+              end
+              aggregate_failures("non-removed plugins") do
+                (setup_plugin_list - plugins_to_remove).each do |expected_remaining_plugin|
+                  expect(presence_check.stderr_and_stdout).to include(expected_remaining_plugin)
+                end
+              end
+            end
+          end
+        end
+      end
     end
 end

rakelib/plugin.rake

@@ -24,9 +24,9 @@ namespace "plugin" do
     LogStash::PluginManager::Main.run("bin/logstash-plugin", ["install"] + args)
   end
 
-  def remove_plugin(plugin)
+  def remove_plugin(plugin, *more_plugins)
     require_relative "../lib/pluginmanager/main"
-    LogStash::PluginManager::Main.run("bin/logstash-plugin", ["remove", plugin])
+    LogStash::PluginManager::Main.run("bin/logstash-plugin", ["remove", plugin] + more_plugins)
   end
 
   task "install-base" => "bootstrap" do