(IAC-1657) Fix for invalid DateTime value error in `invoke_get_method` #169

david22swan · 2021-06-23T14:43:00Z

Fix has been put in place so that if the returned DateTime is nil it is not parsed and is instead returned as is.
In addition, if the parse ever fails the error is caught and outputted into the context, with nil being returned.

michaeltlombardi

Can we get a minimal unit test to validate this behavior? Otherwise, looks good!

spec/unit/puppet/provider/dsc_base_provider/dsc_base_provider_spec.rb

lib/puppet/provider/dsc_base_provider/dsc_base_provider.rb

michaeltlombardi · 2021-06-24T13:37:23Z

lib/puppet/provider/dsc_base_provider/dsc_base_provider.rb

+        begin
+          data[type_key] = Puppet::Pops::Time::Timestamp.parse(data[type_key]) if context.type.attributes[type_key][:mof_type] =~ /DateTime/i && !data[type_key].nil?
+        rescue ArgumentError, TypeError => e
+          # Catch any failures in the parse, output them to the context and then return nil
+          context.err("Value returned for DateTime (#{data[type_key].inspect}) failed to parse: #{e}")
+          return nil
+        end


I think we want the data[type_key] = assignment to the begin/rescue block itself;

As is, this implementation will attempt to assign the parsed output to the key and, if that errors, return nil for the entire resource.

Ah, thought that was the desired result, have changed it now

michaeltlombardi · 2021-06-24T13:38:11Z

spec/unit/puppet/provider/dsc_base_provider/dsc_base_provider_spec.rb

+      end
+
+      context 'When the DateTime is an invalid string' do
+        let(:name_hash) { { name: 'foo', dsc_name: 'foo', dsc_time: '21000101' } }


Let's make it more obvious this is a bad string and make it foo or something

michaeltlombardi · 2021-06-24T14:22:31Z

spec/unit/puppet/provider/dsc_base_provider/dsc_base_provider_spec.rb

+
+        context 'When a'
+        it 'casts the formatted timestamp string to DateTime in the property hash' do
+          expect(date_time).to receive(:format).and_return('2100-01-01')
+          expect(result).to match(/datetime = \[DateTime\]'2100-01-01'/)
+        end


This looks like it ended up in the wrong place?

Fix has been put in place so that if the returned DateTime is nil it is not parsed and is instead returned as is. In addition, if the parse ever fails the error is caught and outputted into the context, with nil being returned.

david22swan added the bugfix label Jun 23, 2021

david22swan requested a review from a team as a code owner June 23, 2021 14:43