(PDK-988) restrain mungify from non-puppet resource workflows

lib/puppet/resource_api.rb

@@ -68,7 +68,7 @@ def type_definition
           @title = attributes.title
           attributes = attributes.to_hash
         else
-          @called_from_resource = true
+          @ral_find_absent = true
         end
         # $stderr.puts "B: #{attributes.inspect}"
         if type_definition.feature?('canonicalize')
@@ -92,6 +92,10 @@ def type_definition
         # enforce mandatory attributes
         @missing_attrs = []
         @missing_params = []
+
+        # do not validate on known-absent instances
+        return if @ral_find_absent
+
         definition[:attributes].each do |name, options|
           type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type])
 
@@ -109,9 +113,9 @@ def type_definition
           end
         end
 
-        @missing_attrs -= [:ensure] if @called_from_resource
+        @missing_attrs -= [:ensure]
 
-        raise_missing_params if @missing_params.any? && @called_from_resource != true
+        raise_missing_params if @missing_params.any?
       end
 
       definition[:attributes].each do |name, options|
@@ -206,16 +210,16 @@ def insync?(is)
             end
           end
 
-          if type.instance_of? Puppet::Pops::Types::POptionalType
-            type = type.type
-          end
-
           # puppet symbolizes some values through puppet/parameter/value.rb (see .convert()), but (especially) Enums
           # are strings. specifying a munge block here skips the value_collection fallback in puppet/parameter.rb's
           # default .unsafe_munge() implementation.
           munge { |v| v }
 
           # provide hints to `puppet type generate` for better parsing
+          if type.instance_of? Puppet::Pops::Types::POptionalType
+            type = type.type
+          end
+
           case type
           when Puppet::Pops::Types::PStringType
             # require any string value
@@ -439,18 +443,32 @@ def self.def_newvalues(this, param_or_property, *values)
     end
   end
 
-  # Validates and munges values coming from puppet into a shape palatable to the provider.
-  # This includes parsing values from strings, e.g. when running in `puppet resource`.
+  def self.caller_is_resource_app?
+    caller.any? { |c| c.match(%r{application/resource.rb:}) }
+  end
+
+  # This method handles translating values from the runtime environment to the expected types for the provider.
+  # When being called from `puppet resource`, it tries to transform the strings from the command line into their
+  # expected ruby representations, e.g. `"2"` (a string), will be transformed to `2` (the number) if (and only if)
+  # the target `type` is `Integer`.
+  # Additionally this function also validates that the passed in (and optionally transformed) value matches the
+  # specified type.
   # @param type[Puppet::Pops::Types::TypedModelObject] the type to check/clean against
   # @param value the value to clean
   # @param error_msg_prefix[String] a prefix for the error messages
   # @return [type] the cleaned value
   # @raise [Puppet::ResourceError] if `value` could not be parsed into `type`
   def self.mungify(type, value, error_msg_prefix)
-    cleaned_value, error = try_mungify(type, value, error_msg_prefix)
-
-    raise Puppet::ResourceError, error if error
-
+    if caller_is_resource_app?
+      # When the provider is exercised from the `puppet resource` CLI, we need to unpack strings into
+      # the correct types, e.g. "1" (a string) to 1 (an integer)
+      cleaned_value, error_msg = try_mungify(type, value, error_msg_prefix)
+      raise Puppet::ResourceError, error_msg if error_msg
+    else
+      # Every other time, we can use the values as is
+      cleaned_value = value
+    end
+    Puppet::ResourceApi.validate(type, cleaned_value, error_msg_prefix)
     cleaned_value
   end
 
@@ -513,13 +531,41 @@ def self.try_mungify(type, value, error_msg_prefix)
       # fall through to default handling
     end
 
-    # a match!
-    return [value, nil] if type.instance?(value)
+    error_msg = try_validate(type, value, error_msg_prefix)
+    if error_msg
+      # an error :-(
+      [nil, error_msg]
+    else
+      # a match!
+      [value, nil]
+    end
+  end
+
+  # Validates the `value` against the specified `type`.
+  # @param type[Puppet::Pops::Types::TypedModelObject] the type to check against
+  # @param value the value to clean
+  # @param error_msg_prefix[String] a prefix for the error messages
+  # @raise [Puppet::ResourceError] if `value` is not of type `type`
+  # @private
+  def self.validate(type, value, error_msg_prefix)
+    error_msg = try_validate(type, value, error_msg_prefix)
+
+    raise Puppet::ResourceError, error_msg if error_msg
+  end
+
+  # Tries to validate the `value` against the specified `type`.
+  # @param type[Puppet::Pops::Types::TypedModelObject] the type to check against
+  # @param value the value to clean
+  # @param error_msg_prefix[String] a prefix for the error messages
+  # @return [String, nil] a error message indicating the problem, or `nil` if the value was valid.
+  # @private
+  def self.try_validate(type, value, error_msg_prefix)
+    return nil if type.instance?(value)
 
     # an error :-(
     inferred_type = Puppet::Pops::Types::TypeCalculator.infer_set(value)
     error_msg = Puppet::Pops::Types::TypeMismatchDescriber.new.describe_mismatch(error_msg_prefix, type, inferred_type)
-    [nil, error_msg]
+    error_msg
   end
 
   def self.validate_ensure(definition)

spec/acceptance/validation_spec.rb

@@ -78,6 +78,12 @@
       expect(status.exitstatus).to eq 0
     end
 
+    it 'rejects stringified integers' do
+      output, status = Open3.capture2e("puppet apply #{common_args} -e \"test_validation{ foo: prop => '2', param => '3' }\"")
+      expect(output.strip).to match %r{Parameter prop failed on Test_validation\[foo\]: test_validation.prop expect.* an Integer value, got String}i
+      expect(status.exitstatus).to eq 1
+    end
+
     it 'allows creating' do
       output, status = Open3.capture2e("puppet apply #{common_args} -e \"test_validation{ new: prop => 2, param => 3 }\"")
       expect(output.strip).to match %r{Test_validation\[new\]/ensure: defined 'ensure' as 'present'}

spec/puppet/resource_api_spec.rb

@@ -185,63 +185,21 @@ def extract_values(function)
         it('the test_url value is set correctly') { expect(instance[:test_url]).to eq('hkp://example.com') }
       end
 
-      context 'when setting string values for the attributes' do
-        let(:params) do
-          {
-            title: 'test',
-            test_string: 'somevalue',
-            test_boolean: 'true',
-            test_integer: '-1',
-            test_float: '-1.5',
-            test_enum: 'a',
-            test_variant_pattern: 'a' * 8,
-            test_url: 'hkp://example.com',
-          }
-        end
-
-        it('the test_string value is set correctly') { expect(instance[:test_string]).to eq 'somevalue' }
-        it('the test_integer value is set correctly') { expect(instance[:test_integer]).to eq(-1) }
-        it('the test_float value is set correctly') { expect(instance[:test_float]).to eq(-1.5) }
-        it('the test_enum value is set correctly') { expect(instance[:test_enum]).to eq('a') }
-        it('the test_variant_pattern value is set correctly') { expect(instance[:test_variant_pattern]).to eq('a' * 8) }
-        it('the test_url value is set correctly') { expect(instance[:test_url]).to eq('hkp://example.com') }
-      end
-
       describe 'different boolean values' do
         let(:params) do
           {
             title: 'test',
             test_string: 'somevalue',
             test_boolean: the_boolean,
-            test_integer: '-1',
-            test_float: '-1.5',
+            test_integer: -1,
+            test_float: -1.5,
             test_enum: 'a',
             test_variant_pattern: 'a' * 8,
             test_url: 'http://example.com',
           }
         end
 
         # rubocop:disable Lint/BooleanSymbol
-        context 'when using :true' do
-          let(:the_boolean) { :true }
-
-          it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :true }
-        end
-        context 'when using :false' do
-          let(:the_boolean) { :false }
-
-          it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :false }
-        end
-        context 'when using "true"' do
-          let(:the_boolean) { 'true' }
-
-          it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :true }
-        end
-        context 'when using "false"' do
-          let(:the_boolean) { 'false' }
-
-          it('the test_boolean value is set correctly') { expect(instance[:test_boolean]).to eq :false }
-        end
         context 'when using true' do
           let(:the_boolean) { true }
 
@@ -255,7 +213,12 @@ def extract_values(function)
         context 'when using an unparsable value' do
           let(:the_boolean) { 'flubb' }
 
-          it('the test_boolean value is set correctly') { expect { instance }.to raise_error Puppet::ResourceError, %r{test_boolean expect.* Boolean .* got String} }
+          it('an error is raised') { expect { instance }.to raise_error Puppet::ResourceError, %r{test_boolean expect.* Boolean .* got String} }
+        end
+        context 'when using a legacy symbol' do
+          let(:the_boolean) { :true }
+
+          it('an error is raised') { expect { instance }.to raise_error Puppet::ResourceError, %r{test_boolean expect.* Boolean .* got Runtime} }
         end
         # rubocop:enable Lint/BooleanSymbol
       end
@@ -369,7 +332,11 @@ def set(_context, _changes); end
       end
 
       describe 'an instance of this type' do
-        subject(:instance) { Puppet::Type.type(:with_ensure).new(params) }
+        subject(:instance) do
+          type = Puppet::Type.type(:with_ensure)
+          resource = Puppet::Resource.new(type, params[:title], parameters: params)
+          type.new(resource)
+        end
 
         context 'when mandatory attributes are missing, but ensure is present' do
           let(:params) do
@@ -531,9 +498,9 @@ def set(_context, _changes); end
           { parameters: {
             name: 'test',
             test_string: 'somevalue',
-            test_boolean: 'true',
-            test_integer: '-1',
-            test_float: '-1.5',
+            test_boolean: true,
+            test_integer: -1,
+            test_float: -1.5,
             test_variant_pattern: 'a' * 8,
             test_url: 'hkp://example.com',
           } }
@@ -1496,24 +1463,88 @@ def set(_context, changes) end
   end
 
   describe '#mungify(type, value)' do
-    before(:each) do
-      allow(described_class).to receive(:try_mungify).with('type', 'input', 'error prefix').and_return(result)
-    end
-    context 'when the munge succeeds' do
-      let(:result) { ['result', nil] }
+    context 'when called from `puppet resource`' do
+      before(:each) do
+        allow(described_class).to receive(:caller_is_resource_app?).with(no_args).and_return(true)
+        allow(described_class).to receive(:try_mungify).with('type', 'input', 'error prefix').and_return(result)
+        allow(described_class).to receive(:validate)
+      end
+
+      let(:caller_is_resource_app) { true }
+
+      context 'when the munge succeeds' do
+        let(:result) { ['result', nil] }
+
+        it('returns the cleaned result') { expect(described_class.mungify('type', 'input', 'error prefix')).to eq 'result' }
+        it('validates the cleaned result') do
+          described_class.mungify('type', 'input', 'error prefix')
+          expect(described_class).to have_received(:validate).with('type', 'result', 'error prefix').once
+        end
+      end
+
+      context 'when the munge fails' do
+        let(:result) { [nil, 'some error'] }
 
-      it('returns the cleaned result') { expect(described_class.mungify('type', 'input', 'error prefix')).to eq 'result' }
+        it('raises the error') { expect { described_class.mungify('type', 'input', 'error prefix') }.to raise_error Puppet::ResourceError, %r{\Asome error\Z} }
+      end
     end
 
-    context 'when the munge fails' do
-      let(:result) { [nil, 'some error'] }
+    context 'when called from something else' do
+      before(:each) do
+        allow(described_class).to receive(:caller_is_resource_app?).with(no_args).and_return(false)
+        allow(described_class).to receive(:try_mungify).never
+        allow(described_class).to receive(:validate)
+      end
 
-      it('raises the error') { expect { described_class.mungify('type', 'input', 'error prefix') }.to raise_error Puppet::ResourceError, %r{\Asome error\Z} }
+      it('returns the value') { expect(described_class.mungify('type', 'input', 'error prefix')).to eq 'input' }
+      it('validates the value') do
+        described_class.mungify('type', 'input', 'error prefix')
+        expect(described_class).to have_received(:validate).with('type', 'input', 'error prefix')
+      end
     end
   end
 
   # keep test data consistent # rubocop:disable Style/WordArray
   # run try_mungify only once to get both value, and error # rubocop:disable RSpec/InstanceVariable
+  describe '#try_validate(type, value)' do
+    let(:error_msg) do
+      pops_type = Puppet::Pops::Types::TypeParser.singleton.parse(type)
+      described_class.try_validate(pops_type, value, 'error prefix')
+    end
+
+    [
+      {
+        type: 'String',
+        valid: ['a', 'true'],
+        invalid: [1, true],
+      },
+      {
+        type: 'Integer',
+        valid: [1, -1, 0],
+        invalid: ['a', :a, 'true', 1.0],
+      },
+    ].each do |testcase|
+      context "when validating '#{testcase[:type]}" do
+        let(:type) { testcase[:type] }
+
+        testcase[:valid].each do |valid_value|
+          context "when validating #{valid_value.inspect}" do
+            let(:value) { valid_value }
+
+            it { expect(error_msg).to be nil }
+          end
+        end
+        testcase[:invalid].each do |invalid_value|
+          context "when validating #{invalid_value.inspect}" do
+            let(:value) { invalid_value }
+
+            it { expect(error_msg).to match %r{^error prefix } }
+          end
+        end
+      end
+    end
+  end
+
   describe '#try_mungify(type, value)' do
     before(:each) do
       @value, @error = described_class.try_mungify(type, input, 'error prefix')