(PDK-988) Remove mungify from non-puppet resource calls

The mungify introduced in puppetlabs#59 would also affect strings passed in as
parameters through manifests (e.g. using `puppet apply` or the agent),
masking input type errors. This change fixes the behaviour so that
munging is only happening when the runtime environment is actually
puppet resource.

Of course, long-term it would be better to have the CLI itself mungify
inputs from the CLI. Such an implementation could provider more specific
error messages, and reduce code complexity here. For now, this solution
works across all puppet versions, and provides the same functionality.
versions.

lib/puppet/resource_api.rb

@@ -443,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
 
@@ -527,6 +541,18 @@ def self.try_mungify(type, value, error_msg_prefix)
     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)
+    is_valid, error_msg = try_validate(type, value, error_msg_prefix)
+
+    raise Puppet::ResourceError, error_msg unless is_valid
+  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

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 expects 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,20 @@ 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,9 +212,13 @@ 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 } # rubocop:disable Lint/BooleanSymbol
+
+          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
 
       context 'with a basic provider', agent_test: true do
@@ -535,9 +496,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',
           } }
@@ -1500,19 +1461,39 @@ 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
+        expect(described_class).to receive(:caller_is_resource_app?).with(no_args).and_return(true)
+        expect(described_class).to receive(:try_mungify).with('type', 'input', 'error prefix').and_return(result)
+      end
+
+      let(:caller_is_resource_app) { true }
+
+      context 'when the munge succeeds' do
+        let(:result) { ['result', nil] }
+
+        before(:each) do
+          expect(described_class).to receive(:validate).with('type', result[0], 'error prefix')
+        end
 
-      it('returns the cleaned result') { expect(described_class.mungify('type', 'input', 'error prefix')).to eq 'result' }
+        it('returns the cleaned result') { expect(described_class.mungify('type', 'input', 'error prefix')).to eq 'result' }
+      end
+
+      context 'when the munge fails' do
+        let(:result) { [nil, 'some error'] }
+
+        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
+        expect(described_class).to receive(:caller_is_resource_app?).with(no_args).and_return(false)
+        expect(described_class).to receive(:try_mungify).never
+        expect(described_class).to receive(:validate).with('type', 'input', 'error prefix')
+      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' }
     end
   end