Fix serialization of undef in to_python()

[smortex]

:undef was used in old Puppet functions, but modern Puppet 4 functions receive undef Puppet values as nil Ruby values. This cause to_python() to serialize undef as nil instead of None, which is not valid Python code.

Fix the to_python() behavior by comparing with nil. Update the test suite accordingly.

While here, also adjust the to_ruby() function which had the same wrong logic but was not causing trouble because in Ruby, we want to serialize nil to nil :-)

[puppet-community-rangefinder]

to_python is a function

that may have no external impact to Forge modules.

to_ruby is a function

that may have no external impact to Forge modules.

This module is declared in 319 of 578 indexed public Puppetfiles.

---

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

[hlindberg]

In general, the to_ruby, and to_python are quite naive in the data types that they handle. Puppet has a rich type system (Pcore). Read about it here: https://github.com/puppetlabs/puppet-specifications/blob/master/language/data-types/pcore-data-representation.md

In order to implement transformations of Pcore data to other representations, the easiest is probably to use the "Generic Pcore Data Format" in JSON and then transform it to the target encoding. That format is described here: https://github.com/puppetlabs/puppet-specifications/blob/master/language/data-types/pcore-generic-data.md (this format is used for the catalog compilation result sent to agents).

For examples using the built in serializer in Puppet see https://github.com/hlindberg/tahu/blob/master/lib/puppet/functions/tahu/convert_to_rich_data.rb

Example of stuff the naive implementation will probably choke on (or produce some strange string representation of):

$x = [default, Timestamp(), /.*/, Car(regnbr => 'abc123'), Integer[0,10], Deferred('f', [1,2,3]), Resource['file', '/tmp/tmp.txt'],]

[CLAassistant]

All committers have signed the CLA.

[adrianiurca]

So could this be considered a bug fix or it's just an improvement?

[smortex]

This is clearly fixing a bug: the currently generated code is invalid (example).

But I also think the input data type is wrong: Any allows to pass anything while we shall only accept Data.

[smortex]

@hlindberg may I ask you to check the last commit to confirm it is what you expected? It looks a bit useless to me to have support for this so I might have missed something? Maybe accepting Data instead of Any would make more sense?

romain@zappy ~/Projects/puppetlabs/puppetlabs-stdlib % cat tset.pp 
type Car = Object[attributes => {regnbr => String}]

$x = [default, Timestamp(), /.*/, Car(regnbr => 'abc123'), Integer[0,10], Deferred('f', [1,2,3]), Resource['file', '/tmp/tmp.txt'],3]

warning($x.to_python)
warning($x.to_ruby)
romain@zappy ~/Projects/puppetlabs/puppetlabs-stdlib % puppet apply -t --modulepath spec/fixtures/modules tset.pp 
Info: Loading facts
Warning: Scope(Class[main]): [{"__ptype": "Default"}, {"__ptype": "Timestamp", "__pvalue": "2021-10-11T20:15:28.626940753 UTC"}, {"__ptype": "Regexp", "__pvalue": ".*"}, {"__ptype": "Car", "regnbr": "abc123"}, {"__ptype": "Pcore::IntegerType", "from": 0, "to": 10}, {"__ptype": "Deferred", "name": "f", "arguments": [1, 2, 3]}, {"__ptype": "Pcore::ResourceType", "type_name": "File", "title": "/tmp/tmp.txt"}, 3]
Warning: Scope(Class[main]): [{"__ptype" => "Default"}, {"__ptype" => "Timestamp", "__pvalue" => "2021-10-11T20:15:28.626940753 UTC"}, {"__ptype" => "Regexp", "__pvalue" => ".*"}, {"__ptype" => "Car", "regnbr" => "abc123"}, {"__ptype" => "Pcore::IntegerType", "from" => 0, "to" => 10}, {"__ptype" => "Deferred", "name" => "f", "arguments" => [1, 2, 3]}, {"__ptype" => "Pcore::ResourceType", "type_name" => "File", "title" => "/tmp/tmp.txt"}, 3]
Notice: Compiled catalog for zappy.blogreen.org in environment production in 0.02 seconds
Info: Using environment 'production'
Info: Applying configuration version '1633983328'
Notice: Applied catalog in 0.02 seconds
romain@zappy ~/Projects/puppetlabs/puppetlabs-stdlib %

I will then rework the branch to take your feedback into account and split the to_ruby changes in another PR.

Thanks!

[github-actions]

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

[smortex]

Dear 🤖, this is still relevant:

1. the current code is broken;
2. this PR fix the issue a way it was asked me to update it;
3. I do not feel this is the right fix so I asked for advice in my last comment.

[chelnak]

Hey @smortex , thanks for letting us know.

Am I right that we are waiting on @hlindberg 's response to your question?

[hlindberg]

By using Any and Pcore serialization all data types have a specified representation. In contrast if only accepting Data the non standard datatypes would lead to an error if an attempt is made to transform them "to_ruby/to_python". With the Pcore serialization the result for non standard datatypes is a standard data type with an encoding that can be used in ruby/python but requires either pcore support or "manual" inspection of the type information.

Clearly, offering the full support for Any is more versatile.

[smortex]

Thanks @hlindberg for the clarification.

I moved the commits for Pcore serialization to #1237 and rebased the fix on top of the main branch.

[david22swan]

Everything look's good to me so I'm gonna go ahead and merge.
Thank you @smortex for the work and @hlindberg for sharing your wise advice.