From d4012dbf2e7e041e8e24eda6cd896b6f6e4fac4d Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Wed, 6 Apr 2011 16:28:56 -0700 Subject: (#6995) Fix indexing of :current on string load. We do this by implementing a standard mechanism for finding the current version out of the default file, and only supporting that one file. This implements our decision to lazy-evaluate the extra version support stuff as much as possible. Reviewed-By: Dan Bode --- lib/puppet/string/string_collection.rb | 86 +++++++++++++++++++++++------- spec/unit/string/string_collection_spec.rb | 54 ++++++++----------- spec/unit/string_spec.rb | 1 - 3 files changed, 89 insertions(+), 52 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/unit/string/string_collection_spec.rb b/spec/unit/string/string_collection_spec.rb index fa63f18e3..fab647da0 100755 --- a/spec/unit/string/string_collection_spec.rb +++ b/spec/unit/string/string_collection_spec.rb @@ -70,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 @@ -104,26 +103,15 @@ 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| - subject.instance_variable_get("@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 @@ -135,24 +123,28 @@ describe Puppet::String::StringCollection do 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| - subject.instance_variable_get("@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 <