Merge pull request #14 from jeffmccune/maint/master-pr13/pr14_fix_aut…

…orequire_case_sensitivity

Fix autorequire case sensitivity

acceptance/tests/resource/registry/should_tolerate_mixed_case.rb

@@ -0,0 +1,161 @@
+require 'pathname'
+require Pathname.new(__FILE__).dirname.dirname.dirname.dirname + 'lib/systest/util/registry'
+# Include our utility methods in the singleton class of the test case instance.
+class << self
+  include Systest::Util::Registry
+end
+
+test_name "Registry Value Management (Mixed Case)"
+
+# JJM - The whole purpose of this test is to exercise situations where the user
+# specifies registry valies in a case-insensitive but case-preserving way.
+# This has particular "gotchas" with regard to autorequire.
+#
+# Note how SUBKEY1 is capitalized in some resources but downcased in other
+# resources.  On windows these refer to the same thing, and case is preserved.
+# In puppet, however, resource namevars are case sensisitve unless otherwise
+# noted.
+
+# Generate a unique key name
+keyname = "PuppetLabsTest_MixedCase_#{randomstring(8)}"
+# This is the keypath we'll use for this entire test.  We will actually create this key and delete it.
+vendor_path = "HKLM\\Software\\Vendor"
+keypath = "#{vendor_path}\\#{keyname}"
+
+master_manifest_content = <<HERE
+notify { fact_phase: message => "fact_phase: $fact_phase" }
+
+registry_key { '#{vendor_path}': ensure => present }
+registry_key { '32:#{vendor_path}': ensure => present }
+
+class phase1 {
+  Registry_key { ensure => present }
+  registry_key { '#{keypath}': }
+  registry_key { '#{keypath}\\SUBKEY1': }
+  # NOTE THE DIFFERENCE IN CASE IN SubKey1 and SUBKEY1 above
+  registry_key { '#{keypath}\\SubKey1\\SUBKEY2': }
+  registry_key { '32:#{keypath}': }
+  registry_key { '32:#{keypath}\\SUBKEY1': }
+  registry_key { '32:#{keypath}\\SubKey1\\SUBKEY2': }
+
+  # The Default Value
+  # NOTE THE DIFFERENCE IN CASE IN SubKey1 and SUBKEY1 above
+  registry_value { '#{keypath}\\SubKey1\\\\':
+    data => "Default Data",
+  }
+
+  # String Values
+  registry_value { '#{keypath}\\SubKey1\\ValueString1':
+    data => "Should be a string",
+  }
+  registry_value { '#{keypath}\\SubKey1\\ValueString2':
+    type => string,
+    data => "Should be a string",
+  }
+  registry_value { '#{keypath}\\SubKey1\\ValueString3':
+    ensure => present,
+    type   => string,
+    data   => "Should be a string",
+  }
+  registry_value { '#{keypath}\\SubKey1\\ValueString4':
+    data   => "Should be a string",
+    type   => string,
+    ensure => present,
+  }
+  registry_value { '#{keypath}\\SubKey1\\SubKey2\\ValueString1':
+    data => "Should be a string",
+  }
+  registry_value { '#{keypath}\\SubKey1\\SubKey2\\ValueString2':
+    type => string,
+    data => "Should be a string",
+  }
+  registry_value { '#{keypath}\\SubKey1\\SubKey2\\ValueString3':
+    ensure => present,
+    type   => string,
+    data   => "Should be a string",
+  }
+  registry_value { '#{keypath}\\SubKey1\\SubKey2\\ValueString4':
+    data   => "Should be a string",
+    type   => string,
+    ensure => present,
+  }
+
+  # The Default Value
+  # NOTE THE DIFFERENCE IN CASE IN SubKey1 and SUBKEY1 above
+  registry_value { '32:#{keypath}\\SubKey1\\\\':
+    data => "Default Data",
+  }
+
+  # String Values
+  registry_value { '32:#{keypath}\\SubKey1\\ValueString1':
+    data => "Should be a string",
+  }
+  registry_value { '32:#{keypath}\\SubKey1\\ValueString2':
+    type => string,
+    data => "Should be a string",
+  }
+  registry_value { '32:#{keypath}\\SubKey1\\ValueString3':
+    ensure => present,
+    type   => string,
+    data   => "Should be a string",
+  }
+  registry_value { '32:#{keypath}\\SubKey1\\ValueString4':
+    data   => "Should be a string",
+    type   => string,
+    ensure => present,
+  }
+  registry_value { '32:#{keypath}\\SubKey1\\SubKey2\\ValueString1':
+    data => "Should be a string",
+  }
+  registry_value { '32:#{keypath}\\SubKey1\\SubKey2\\ValueString2':
+    type => string,
+    data => "Should be a string",
+  }
+  registry_value { '32:#{keypath}\\SubKey1\\SubKey2\\ValueString3':
+    ensure => present,
+    type   => string,
+    data   => "Should be a string",
+  }
+  registry_value { '32:#{keypath}\\SubKey1\\SubKey2\\ValueString4':
+    data   => "Should be a string",
+    type   => string,
+    ensure => present,
+  }
+}
+
+case $fact_phase {
+  default: { include phase1 }
+}
+HERE
+
+# Setup the master to use the modules specified in the --modules option
+setup_master master_manifest_content
+
+step "Start the master" do
+  with_master_running_on(master, master_options) do
+    # A set of keys we expect Puppet to create
+    phase1_resources_created = [
+      /Registry_key\[HKLM.Software.Vendor.PuppetLabsTest\w+\].ensure: created/,
+      /Registry_key\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SUBKEY1\].ensure: created/,
+      /Registry_key\[32:HKLM.Software.Vendor.PuppetLabsTest\w+\].ensure: created/,
+      /Registry_key\[32:HKLM.Software.Vendor.PuppetLabsTest\w+\\SUBKEY1\].ensure: created/,
+      /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\\\\].ensure: created/,
+      /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\ValueString1\].ensure: created/,
+      /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\ValueString2\].ensure: created/,
+      /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\ValueString3\].ensure: created/,
+      /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\ValueString4\].ensure: created/,
+    ]
+
+    windows_agents.each do |agent|
+      this_agent_args = agent_args % get_test_file_path(agent, agent_lib_dir)
+
+      step "Registry Tolerate Mixed Case Values - Phase 1.a - Create some values"
+      on agent, puppet_agent(this_agent_args, :environment => { 'FACTER_FACT_PHASE' => '1' }), :acceptable_exit_codes => agent_exit_codes do
+        phase1_resources_created.each do |val_re|
+          assert_match(val_re, result.stdout, "Expected output to contain #{val_re.inspect}.")
+        end
+        assert_no_match(/err:/, result.stdout, "Expected no error messages.")
+      end
+    end
+  end
+end

lib/puppet/type/registry_key.rb

@@ -20,7 +20,12 @@ def self.title_patterns
       Puppet::Modules::Registry::RegistryKeyPath.new(path).valid?
     end
     munge do |path|
-      Puppet::Modules::Registry::RegistryKeyPath.new(path).canonical
+      canonical = Puppet::Modules::Registry::RegistryKeyPath.new(path).canonical
+      # Windows is case insensitive and case preserving.  We deal with this by
+      # aliasing resources to their downcase values.  This is inspired by the
+      # munge block in the alias metaparameter.
+      @resource.catalog.alias(@resource, canonical.downcase) if @resource.catalog.respond_to? :alias
+      canonical
     end
   end
 
@@ -59,8 +64,11 @@ def self.title_patterns
   autorequire(:registry_key) do
     req = []
     path = Puppet::Modules::Registry::RegistryKeyPath.new(value(:path))
-    if found = path.enum_for(:ascend).find { |p| catalog.resource(:registry_key, p.to_s) }
-      req << found.to_s
+    # It is important to match against the downcase value of the path because
+    # other resources are expected to alias themselves to the downcase value so
+    # that we respect the case insensitive and preserving nature of Windows.
+    if found = path.enum_for(:ascend).find { |p| catalog.resource(:registry_key, p.to_s.downcase) }
+      req << found.to_s.downcase
     end
     req
   end

lib/puppet/type/registry_value.rb

@@ -20,7 +20,12 @@ def self.title_patterns
       Puppet::Modules::Registry::RegistryValuePath.new(path).valid?
     end
     munge do |path|
-      Puppet::Modules::Registry::RegistryValuePath.new(path).canonical
+      canonical = Puppet::Modules::Registry::RegistryValuePath.new(path).canonical
+      # Windows is case insensitive and case preserving.  We deal with this by
+      # aliasing resources to their downcase values.  This is inspired by the
+      # munge block in the alias metaparameter.
+      @resource.catalog.alias(@resource, canonical.downcase) if @resource.catalog.respond_to? :alias
+      canonical
     end
   end
 
@@ -97,8 +102,11 @@ def change_to_s(currentvalue, newvalue)
   autorequire(:registry_key) do
     req = []
     path = Puppet::Modules::Registry::RegistryKeyPath.new(value(:path))
-    if found = path.enum_for(:ascend).find { |p| catalog.resource(:registry_key, p.to_s) }
-      req << found.to_s
+    # It is important to match against the downcase value of the path because
+    # other resources are expected to alias themselves to the downcase value so
+    # that we respect the case insensitive and preserving nature of Windows.
+    if found = path.enum_for(:ascend).find { |p| catalog.resource(:registry_key, p.to_s.downcase) }
+      req << found.to_s.downcase
     end
     req
   end

spec/unit/puppet/type/registry_key_spec.rb

@@ -66,11 +66,17 @@
     context "purging" do
       let (:catalog) do Puppet::Resource::Catalog.new end
 
+      # This is overridden here so we get a consistent association with the key
+      # and a catalog using memoized let methods.
+      let (:key) do
+        Puppet::Type.type(:registry_key).new(:name => 'HKLM\Software', :catalog => catalog)
+      end
+
       before :each do
         key[:purge_values] = true
         catalog.add_resource(key)
-        catalog.add_resource(Puppet::Type.type(:registry_value).new(:path => "#{key[:path]}\\val1"))
-        catalog.add_resource(Puppet::Type.type(:registry_value).new(:path => "#{key[:path]}\\val2"))
+        catalog.add_resource(Puppet::Type.type(:registry_value).new(:path => "#{key[:path]}\\val1", :catalog => catalog))
+        catalog.add_resource(Puppet::Type.type(:registry_value).new(:path => "#{key[:path]}\\val2", :catalog => catalog))
       end
 
       it "should return an empty array if the key doesn't have any values" do
@@ -92,4 +98,36 @@
       end
     end
   end
+
+  describe "#autorequire" do
+    let :the_catalog do
+      Puppet::Resource::Catalog.new
+    end
+
+    let(:the_resource_name) { 'HKLM\Software\Vendor\PuppetLabs' }
+
+    let :the_resource do
+      # JJM Holy cow this is an intertangled mess.  ;)
+      resource = Puppet::Type.type(:registry_key).new(:name => the_resource_name, :catalog => the_catalog)
+      the_catalog.add_resource resource
+      resource
+    end
+
+    it 'Should initialize the catalog instance variable' do
+      the_resource.catalog.must be the_catalog
+    end
+
+    it 'Should allow case insensitive lookup using the downcase path' do
+      the_resource.must be the_catalog.resource(:registry_key, the_resource_name.downcase)
+    end
+
+    it 'Should preserve the case of the user specified path' do
+      the_resource.must be the_catalog.resource(:registry_key, the_resource_name)
+    end
+
+    it 'Should return the same resource regardless of the alias used' do
+      the_resource.must be the_catalog.resource(:registry_key, the_resource_name)
+      the_resource.must be the_catalog.resource(:registry_key, the_resource_name.downcase)
+    end
+  end
 end