Refactoring of the version check using comparable

Fixes #2331

lib/logstash/config/mixin.rb

@@ -41,7 +41,8 @@ module LogStash::Config::Mixin
     Regexp => 100,
   }
 
-  GEM_NAME_PREFIX = 'logstash'
+  PLUGIN_VERSION_1_0_0 = LogStash::Util::PluginVersion.new(1, 0, 0)
+  PLUGIN_VERSION_0_9_0 = LogStash::Util::PluginVersion.new(0, 9, 0)
 
   # This method is called when someone does 'include LogStash::Config'
   def self.included(base)
@@ -210,36 +211,36 @@ module LogStash::Config::Mixin
       return is_valid
     end # def validate
 
-    def plugin_version
-      specification = Gem::Specification.find_by_name(plugin_gem_name)
-      major, minor, patch = specification.version.segments
-
-      return LogStash::Util::PluginVersion.new(major, minor, patch)
-    end
-
-    def plugin_gem_name
-      [GEM_NAME_PREFIX, @plugin_type, @plugin_name].join('-')
-    end
-
     def print_version_notice
-      return true if @@version_notice_given
+      return if @@version_notice_given
 
-      if plugin_version.major < 1
-        if plugin_version.minor == 9
-          @logger.warn(I18n.t("logstash.plugin.version.0-9-x", 
-                              :type => @plugin_type,
-                              :name => @config_name,
-                              :LOGSTASH_VERSION => LOGSTASH_VERSION))
-        else
-          @logger.warn(I18n.t("logstash.plugin.version.0-1-x", 
-                              :type => @plugin_type,
-                              :name => @config_name,
-                              :LOGSTASH_VERSION => LOGSTASH_VERSION))
+      begin
+        plugin_version = LogStash::Util::PluginVersion.find_plugin_version!(@plugin_type, @config_name)
+
+        if plugin_version < PLUGIN_VERSION_1_0_0
+          if plugin_version < PLUGIN_VERSION_0_9_0
+            @logger.warn(I18n.t("logstash.plugin.version.0-1-x", 
+                                :type => @plugin_type,
+                                :name => @config_name,
+                                :LOGSTASH_VERSION => LOGSTASH_VERSION))
+          else
+            @logger.warn(I18n.t("logstash.plugin.version.0-9-x", 
+                                :type => @plugin_type,
+                                :name => @config_name,
+                                :LOGSTASH_VERSION => LOGSTASH_VERSION))
+          end
         end
+      rescue LogStash::PluginNoVersionError
+        # If we cannot find a version in the currently installed gems we
+        # will display this message. This could happen in the test, if you 
+        # create an anonymous class to test a plugin.
+        @logger.warn(I18n.t("logstash.plugin.no_version",
+                                :type => @plugin_type,
+                                :name => @config_name,
+                                :LOGSTASH_VERSION => LOGSTASH_VERSION))
+      ensure 
+        @@version_notice_given = true
       end
-
-      @@version_notice_given = true
-      return true
     end
 
     def validate_check_invalid_parameter_names(params)

lib/logstash/errors.rb

@@ -5,6 +5,7 @@ module LogStash
   class ConfigurationError < Error; end
   class PluginLoadingError < Error; end
   class ShutdownSignal < StandardError; end
+  class PluginNoVersionError < Error; end
 
   class Bug < Error; end
   class ThisMethodWasRemoved < Bug; end

lib/logstash/util/plugin_version.rb

@@ -1 +1,43 @@
-LogStash::Util::PluginVersion = Struct.new(:major, :minor, :patch)
+require 'logstash/errors'
+require 'rubygems/version'
+require 'forwardable'
+
+module LogStash::Util
+  class PluginVersion
+    extend Forwardable
+    include Comparable
+
+    GEM_NAME_PREFIX = 'logstash'
+
+    def_delegators :@version, :to_s
+    attr_reader :version
+
+    def initialize(*options)
+      if options.size == 1 && options.first.is_a?(Gem::Version)
+        @version = options.first
+      else
+        @version = Gem::Version.new(options.join('.'))
+      end
+    end
+
+    def self.find_version!(name)
+      begin
+        specification = Gem::Specification.find_by_name(name)
+        new(specification.version)
+      rescue Gem::LoadError
+        # Rescuing the LoadError and raise a Logstash specific error.
+        # Likely we can't find the gem in the current GEM_PATH
+        raise LogStash::PluginNoVersionError
+      end
+    end
+
+    def self.find_plugin_version!(type, name)
+      plugin_name = [GEM_NAME_PREFIX, type, name].join('-')
+      find_version!(plugin_name)
+    end
+
+    def <=>(obj)
+      version <=> obj.version
+    end
+  end
+end

locales/en.yml

@@ -38,6 +38,9 @@ en:
         %{plugin} plugin is using the 'milestone' method to declare the version
         of the plugin this method is deprecated in favor of declaring the
         version inside the gemspec.
+      no_version: >-
+        %{name} plugin doesn't have a version. This plugin isn't well
+         supported by the community and likely has no maintainer.
       version:
         0-9-x:
          Using version 0.9.x %{type} plugin '%{name}'. This plugin should work but

spec/util/plugin_version_spec.rb

@@ -1,13 +1,48 @@
 require "logstash/util/plugin_version"
+require "logstash/errors"
 
 describe LogStash::Util::PluginVersion do
   subject { LogStash::Util::PluginVersion }
 
-  it 'contains the semver parts of a plugin version' do
-    version = subject.new(1, 2, 3)
+  context "#find_version!" do
+    it 'raises an PluginNoVersionError if we cant find the plugin in the gem path' do
+      dummy_name ='this-character-doesnt-exist-in-the-marvel-universe'
+      expect { subject.find_version!(dummy_name) }.to raise_error(LogStash::PluginNoVersionError)
+    end
 
-    expect(version.major).to eq(1)
-    expect(version.minor).to eq(2)
-    expect(version.patch).to eq(3)
+    it 'returns the version of the gem' do
+      expect { subject.find_version!('bundler') }.not_to raise_error
+    end
+  end
+
+  context "#new" do
+    it 'accepts a Gem::Version instance as argument' do
+      version = Gem::Version.new('1.0.1')
+      expect(subject.new(version).to_s).to eq(version.to_s)
+    end
+
+    it 'accepts an array for defining the version' do
+      version = subject.new(1, 0, 2)
+      expect(version.to_s).to eq('1.0.2')
+    end
+  end
+
+  context "When comparing instances" do
+    it 'allow to check if the version is newer or older' do
+      old_version = subject.new(0, 1, 0)
+      new_version = subject.new(1, 0, 1)
+
+      expect(old_version).to be < new_version
+      expect(old_version).to be <= new_version
+      expect(new_version).to be > old_version
+      expect(new_version).to be >= old_version
+    end
+
+    it 'return true if the version are equal' do
+      version1 = subject.new(0, 1, 0)
+      version2 = subject.new(0, 1, 0)
+
+      expect(version1).to eq(version2)
+    end
   end
 end