Merge branch 'bug/master/6995-current-is-not-always-calculated-correctly'

4 files changed, 135 insertions, 78 deletions

diff --git a/lib/puppet/string/string_collection.rb b/lib/puppet/string/string_collection.rb
index f8fa38b9c..ecd99359d 100644
--- a/lib/puppet/string/string_collection.rb
+++ b/lib/puppet/string/string_collection.rb
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
 require 'puppet/string'
 
 module Puppet::String::StringCollection
@@ -30,37 +31,82 @@ module Puppet::String::StringCollection
     !!(SEMVER_VERSION =~ version.to_s)
   end
 
+  def self.cmp_versions(a, b)
+    a, b = [a, b].map do |x|
+      parts = SEMVER_VERSION.match(x).to_a[1..4]
+      parts[0..2] = parts[0..2].map { |e| e.to_i }
+      parts
+    end
+
+    cmp = a[0..2] <=> b[0..2]
+    if cmp == 0
+      cmp = a[3] <=> b[3]
+      cmp = +1 if a[3].empty? && !b[3].empty?
+      cmp = -1 if b[3].empty? && !a[3].empty?
+    end
+    cmp
+  end
+
   def self.[](name, version)
     @strings[underscorize(name)][version] if string?(name, version)
   end
 
   def self.string?(name, version)
     name = underscorize(name)
-    cache = @strings[name]
-    return true if cache.has_key?(version)
-
-    loaded = cache.keys
+    return true if @strings[name].has_key?(version)
 
-    module_names = ["puppet/string/#{name}"]
-    unless version == :current
-      module_names << "#{name}@#{version}/puppet/string/#{name}"
-    end
+    # We always load the current version file; the common case is that we have
+    # the expected version and any compatibility versions in the same file,
+    # the default.  Which means that this is almost always the case.
+    #
+    # We use require to avoid executing the code multiple times, like any
+    # other Ruby library that we might want to use.  --daniel 2011-04-06
+    begin
+      require "puppet/string/#{name}"
 
-    module_names.each do |module_name|
-      begin
-        require module_name
-        if version == :current || !module_name.include?('@')
-          loaded = (cache.keys - loaded).first
-          cache[:current] = cache[loaded] unless loaded.nil?
-        end
-        return true if cache.has_key?(version)
-      rescue LoadError => e
-        raise unless e.message =~ /-- #{module_name}$/
-        # pass
+      # If we wanted :current, we need to index to find that; direct version
+      # requests just work™ as they go. --daniel 2011-04-06
+      if version == :current then
+        # We need to find current out of this.  This is the largest version
+        # number that doesn't have a dedicated on-disk file present; those
+        # represent "experimental" versions of strings, which we don't fully
+        # support yet.
+        #
+        # We walk the versions from highest to lowest and take the first version
+        # that is not defined in an explicitly versioned file on disk as the
+        # current version.
+        #
+        # This constrains us to only ship experimental versions with *one*
+        # version in the file, not multiple, but given you can't reliably load
+        # them except by side-effect when you ignore that rule this seems safe
+        # enough...
+        #
+        # Given those constraints, and that we are not going to ship a versioned
+        # interface that is not :current in this release, we are going to leave
+        # these thoughts in place, and just punt on the actual versioning.
+        #
+        # When we upgrade the core to support multiple versions we can solve the
+        # problems then; as lazy as possible.
+        #
+        # We do support multiple versions in the same file, though, so we sort
+        # versions here and return the last item in that set.
+        #
+        # --daniel 2011-04-06
+        latest_ver = @strings[name].keys.sort {|a, b| cmp_versions(a, b) }.last
+        @strings[name][:current] = @strings[name][latest_ver]
       end
+    rescue LoadError => e
+      raise unless e.message =~ %r{-- puppet/string/#{name}$}
+      # ...guess we didn't find the file; return a much better problem.
     end
 
-    return false
+    # Now, either we have the version in our set of strings, or we didn't find
+    # the version they were looking for.  In the future we will support
+    # loading versioned stuff from some look-aside part of the Ruby load path,
+    # but we don't need that right now.
+    #
+    # So, this comment is a place-holder for that.  --daniel 2011-04-06
+    return !! @strings[name].has_key?(version)
   end
 
   def self.register(string)
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 4073cb60b..bb71fca73 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -12,31 +12,47 @@ Pathname.glob("#{dir}/shared_behaviours/**/*.rb") do |behaviour|
 end
 
 RSpec.configure do |config|
-    config.mock_with :mocha
+  config.mock_with :mocha
 
-    config.before :each do
-      # Set the confdir and vardir to gibberish so that tests
-      # have to be correctly mocked.
-      Puppet[:confdir] = "/dev/null"
-      Puppet[:vardir] = "/dev/null"
+  config.before :each do
+    # Set the confdir and vardir to gibberish so that tests
+    # have to be correctly mocked.
+    Puppet[:confdir] = "/dev/null"
+    Puppet[:vardir] = "/dev/null"
 
-      # Avoid opening ports to the outside world
-      Puppet.settings[:bindaddress] = "127.0.0.1"
+    # Avoid opening ports to the outside world
+    Puppet.settings[:bindaddress] = "127.0.0.1"
 
-      @logs = []
-      Puppet::Util::Log.newdestination(@logs)
-    end
+    @logs = []
+    Puppet::Util::Log.newdestination(@logs)
+
+    @load_path_scratch_dir = Dir.mktmpdir
+    $LOAD_PATH.push @load_path_scratch_dir
+    FileUtils.mkdir_p(File.join @load_path_scratch_dir, 'puppet', 'string')
+  end
+
+  config.after :each do
+    Puppet.settings.clear
+
+    @logs.clear
+    Puppet::Util::Log.close_all
 
-    config.after :each do
-      Puppet.settings.clear
+    $LOAD_PATH.delete @load_path_scratch_dir
+    FileUtils.remove_entry_secure @load_path_scratch_dir
+  end
 
-      @logs.clear
-      Puppet::Util::Log.close_all
+  def write_scratch_string(name)
+    fail "you need to supply a block: do |fh| fh.puts 'content' end" unless block_given?
+    fail "name should be a symbol" unless name.is_a? Symbol
+    filename = File.join(@load_path_scratch_dir, 'puppet', 'string', "#{name}.rb")
+    File.open(filename, 'w') do |fh|
+      yield fh
     end
+  end
 end
 
 # We need this because the RAL uses 'should' as a method.  This
 # allows us the same behaviour but with a different method name.
 class Object
-    alias :must :should
+  alias :must :should
 end
diff --git a/spec/unit/string/string_collection_spec.rb b/spec/unit/string/string_collection_spec.rb
index 184299e3c..fab647da0 100755
--- a/spec/unit/string/string_collection_spec.rb
+++ b/spec/unit/string/string_collection_spec.rb
@@ -4,18 +4,22 @@ require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
 require 'tmpdir'
 
 describe Puppet::String::StringCollection do
-  before :all do
-    @strings = subject.instance_variable_get("@strings")
-    @strings_backup = @strings.dup
+  # To avoid cross-pollution we have to save and restore both the hash
+  # containing all the string data, and the array used by require.  Restoring
+  # both means that we don't leak side-effects across the code. --daniel 2011-04-06
+  before :each do
+    @original_strings  = subject.instance_variable_get("@strings").dup
+    @original_required = $".dup
+    subject.instance_variable_get("@strings").clear
   end
 
-  before { @strings.clear }
-
-  after :all do
-    subject.instance_variable_set("@strings", @strings_backup)
+  after :each do
+    subject.instance_variable_set("@strings", @original_strings)
+    $".clear ; @original_required.each do |item| $" << item end
   end
 
   describe "::strings" do
+    it "REVISIT: should have some tests here, if we describe it"
   end
 
   describe "::validate_version" do
@@ -66,7 +70,6 @@ describe Puppet::String::StringCollection do
 
     it "should attempt to load the string if it isn't found" do
       subject.expects(:require).with('puppet/string/bar')
-      subject.expects(:require).with('bar@0.0.1/puppet/string/bar')
       subject["bar", '0.0.1']
     end
 
@@ -88,7 +91,7 @@ describe Puppet::String::StringCollection do
 
     it "should attempt to require the string if it is not registered" do
       subject.expects(:require).with do |file|
-        @strings[:bar]['0.0.1'] = true
+        subject.instance_variable_get("@strings")[:bar]['0.0.1'] = true
         file == 'puppet/string/bar'
       end
       subject.string?("bar", '0.0.1').should == true
@@ -100,55 +103,48 @@ describe Puppet::String::StringCollection do
       end
     end
 
-    it "should require the string by version if the 'current' version isn't it" do
-      subject.expects(:require).with('puppet/string/bar').
-        raises(LoadError, 'no such file to load -- puppet/string/bar')
-      subject.expects(:require).with do |file|
-        @strings[:bar]['0.0.1'] = true
-        file == 'bar@0.0.1/puppet/string/bar'
-      end
-      subject.string?("bar", '0.0.1').should == true
-    end
-
     it "should return false if the string is not registered" do
       subject.stubs(:require).returns(true)
-      subject.string?("bar", '0.0.1').should == false
+      subject.string?("bar", '0.0.1').should be_false
     end
 
-    it "should return false if there is a LoadError requiring the string" do
+    it "should return false if the string file itself is missing" do
       subject.stubs(:require).
-        raises(LoadError, 'no such file to load -- puppet/string/bar').then.
-        raises(LoadError, 'no such file to load -- bar@0.0.1/puppet/string/bar')
-      subject.string?("bar", '0.0.1').should == false
+        raises(LoadError, 'no such file to load -- puppet/string/bar')
+      subject.string?("bar", '0.0.1').should be_false
     end
 
     it "should register the version loaded by `:current` as `:current`" do
       subject.expects(:require).with do |file|
-        @strings[:huzzah]['2.0.1'] = :huzzah_string
+        subject.instance_variable_get("@strings")[:huzzah]['2.0.1'] = :huzzah_string
         file == 'puppet/string/huzzah'
       end
       subject.string?("huzzah", :current)
-      @strings[:huzzah][:current].should == :huzzah_string
+      subject.instance_variable_get("@strings")[:huzzah][:current].should == :huzzah_string
     end
 
-    it "should register the version loaded from `puppet/string/{name}` as `:current`" do
-      subject.expects(:require).with do |file|
-        @strings[:huzzah]['2.0.1'] = :huzzah_string
-        file == 'puppet/string/huzzah'
+    context "with something on disk" do
+      before :each do
+        write_scratch_string :huzzah do |fh|
+          fh.puts <<EOF
+Puppet::String.define(:huzzah, '2.0.1') do
+  action :bar do "is where beer comes from" end
+end
+EOF
+        end
       end
-      subject.string?("huzzah", '2.0.1')
-      @strings[:huzzah][:current].should == :huzzah_string
-    end
 
-    it "should not register the version loaded from `{name}@{version}` as `:current`" do
-      subject.expects(:require).with('puppet/string/huzzah').
-        raises(LoadError, 'no such file to load -- puppet/string/huzzah')
-      subject.expects(:require).with do |file|
-        @strings[:huzzah]['0.0.1'] = true
-        file == 'huzzah@0.0.1/puppet/string/huzzah'
+      it "should register the version loaded from `puppet/string/{name}` as `:current`" do
+        subject.should be_string "huzzah", '2.0.1'
+        subject.should be_string "huzzah", :current
+        Puppet::String[:huzzah, '2.0.1'].should == Puppet::String[:huzzah, :current]
+      end
+
+      it "should index :current when the code was pre-required" do
+        subject.instance_variable_get("@strings")[:huzzah].should_not be_key :current
+        require 'puppet/string/huzzah'
+        subject.string?(:huzzah, :current).should be_true
       end
-      subject.string?("huzzah", '0.0.1')
-      @strings[:huzzah].should_not have_key(:current)
     end
   end
 
diff --git a/spec/unit/string_spec.rb b/spec/unit/string_spec.rb
index 358668f9b..9b7cd887c 100755
--- a/spec/unit/string_spec.rb
+++ b/spec/unit/string_spec.rb
@@ -76,7 +76,6 @@ describe Puppet::String do
 
   it "should try to require strings that are not known" do
     Puppet::String::StringCollection.expects(:require).with "puppet/string/foo"
-    Puppet::String::StringCollection.expects(:require).with "foo@0.0.1/puppet/string/foo"
     Puppet::String[:foo, '0.0.1']
   end