Jira (PUP-8615) sort_formats does not sort consistently with MRI Ruby which causes issues when find .find processes the hash to select a choice

Eric Delaney (JIRA)

unread,

Mar 29, 2018, 3:02:04 PM3/29/18

to puppe...@googlegroups.com

Eric Delaney updated an issue

Puppet /

PUP-8615

sort_formats does not sort consistently with MRI Ruby which causes issues when find .find processes the hash to select a choice

Change By:	Eric Delaney

get_format relies on the order that .find processes the hash which causes it to fail on JRuby

This test fails under JRuby when the wrong formatter is selected:

{noformat}
  1) The string converter when converting array multiple rules selects most specific
     Failure/Error: expect(converter.convert([1, 2], string_formats)).to eq('(1, 2)')
       expected: "(1, 2)"
            got: "[1, 2]"
       (compared using ==)
     # ./spec/unit/pops/types/string_converter_spec.rb:691:in `block in (root)'
     # util/rspec_runner:44:in `run'
     # util/rspec_runner:59:in `<main>'
{noformat}

The underlying issue is in get_format():

https://github.com/puppetlabs/puppet/blob/master/lib/puppet/pops/types/string_converter.rb#L1114

We call .find on the hash of the formatPoptions trying to find an assignable one to use:

{code: ruby}
    fmt = format_options.find {|k,_| k.assignable?(val_t) }
{code}

So I added code to walk the list as .find would:

{code}
    fmt_false = format_options.find {|k,_|
      $stderr.puts("k: #{k.class} #{k.assignable?(val_t)}")
      $stderr.puts("k: #{k.inspect}")
      $stderr.puts("")
      false
    }
{code}

On MRI Ruby this ends up with a list like this, where the 2nd entry is the formater that we expected to be used, we'd find and use that one:

{noformat}
k: Puppet::Pops::Types::PHashType false
k: #<Puppet::Pops::Types::PHashType:0x007fbdc5d2cbb8 @size_type=nil, @key_type=#<Puppet::Pops::Types::PAnyType:0x007fbdc5d2f570>, @value_type=#<Puppet::Pops::Types::PAnyType:0x007fbdc5d2f570>>

k: Puppet::Pops::Types::PArrayType true
k: #<Puppet::Pops::Types::PArrayType:0x007fbdc50c5cf8 @size_type=#<Puppet::Pops::Types::PIntegerType:0x007fbdc50c5d20 @from=1, @to=2>, @element_type=#<Puppet::Pops::Types::PIntegerType:0x007fbdc5d2e1c0 @from=-Infinity, @to=Infinity>>

k: Puppet::Pops::Types::PArrayType true
k: #<Puppet::Pops::Types::PArrayType:0x007fbdc5d2ce60 @size_type=nil, @element_type=#<Puppet::Pops::Types::PAnyType:0x007fbdc5d2f570>>

k: Puppet::Pops::Types::PBinaryType false
k: #<Puppet::Pops::Types::PBinaryType:0x007fbdc3a27f50>

k: Puppet::Pops::Types::PFloatType false
k: #<Puppet::Pops::Types::PFloatType:0x007fbdc5d2e080 @from=-Infinity, @to=Infinity>

k: Puppet::Pops::Types::PNumericType false
k: #<Puppet::Pops::Types::PNumericType:0x007fbdc5d2e2d8 @from=-Infinity, @to=Infinity>

k: Puppet::Pops::Types::PObjectType false
k: #<Puppet::Pops::Types::PObjectType:0x007fbdc5943650 @type_parameters={}, @attributes={}, @functions={}, @name=nil, @parent=nil, @equality_include_type=true, @equality=nil, @checks=nil, @annotations=nil>

k: Puppet::Pops::Types::PAnyType true
k: #<Puppet::Pops::Types::PAnyType:0x007fbdc5d2f570>
{noformat}

However on JRuby (jruby-9.1.15.0), running the same test the order that the .find will process the hash is different:

{noformat}
k: Puppet::Pops::Types::PArrayType true
k: #<Puppet::Pops::Types::PArrayType:0x687a0e40 @size_type=nil, @element_type=#<Puppet::Pops::Types::PAnyType:0x2db6d68d>>

k: Puppet::Pops::Types::PBinaryType false
k: #<Puppet::Pops::Types::PBinaryType:0x33a47707>

k: Puppet::Pops::Types::PFloatType false
k: #<Puppet::Pops::Types::PFloatType:0x720a1fd0 @to=Infinity, @from=-Infinity>

k: Puppet::Pops::Types::PHashType false
k: #<Puppet::Pops::Types::PHashType:0x4abbe41c @key_type=#<Puppet::Pops::Types::PAnyType:0x2db6d68d>, @size_type=nil, @value_type=#<Puppet::Pops::Types::PAnyType:0x2db6d68d>>

k: Puppet::Pops::Types::PArrayType true
k: #<Puppet::Pops::Types::PArrayType:0x2ef041bb @size_type=#<Puppet::Pops::Types::PIntegerType:0x45e7bb79 @to=2, @from=1>, @element_type=#<Puppet::Pops::Types::PIntegerType:0xedb83f8 @to=Infinity, @from=-Infinity>>

k: Puppet::Pops::Types::PNumericType false
k: #<Puppet::Pops::Types::PNumericType:0xacb5508 @to=Infinity, @from=-Infinity>

k: Puppet::Pops::Types::PObjectType false
k: #<Puppet::Pops::Types::PObjectType:0x3ac0a14b @functions={}, @checks=nil, @attributes={}, @equality_include_type=true, @name=nil, @annotations=nil, @parent=nil, @type_parameters={}, @equality=nil>

k: Puppet::Pops::Types::PAnyType true
k: #<Puppet::Pops::Types::PAnyType:0x2db6d68d>
{noformat}

Note that the first entry is an assignable? version, but its the 5th one that we're actually expected to be selected for the test to pass.

We attempt to order this list in Puppet::Pops::Type::Converter.sort_formats(), but the returned Hash is different between JRuby and MRI.

Add Comment

This message was sent by Atlassian JIRA (v7.7.1#77002-sha1:e75ca93)

Eric Delaney (JIRA)

unread,

Mar 29, 2018, 3:02:06 PM3/29/18

to puppe...@googlegroups.com

Eric Delaney updated an issue

Puppet /

PUP-8615

sort_formats does not sort consistently with MRI Ruby which causes issues when find .find processes the hash to select a choice

Change By:	Eric Delaney
Summary:	get_format relies on the order that sort_formats does not sort consistently with MRI Ruby which causes issues when find .find processes the hash which causes it to fail on JRuby select a choice

Add Comment

Eric Delaney (JIRA)

unread,

Mar 29, 2018, 3:36:03 PM3/29/18

to puppe...@googlegroups.com

Eric Delaney updated an issue

Puppet /

PUP-8615

sort_formats does not sort consistently with MRI Ruby which causes issues when find .find processes the hash to select a choice

Change By:	Eric Delaney

get_format relies on the order that .find processes the hash to find the first entry to use. So the order of the hash which is insertion order matters.

There is a problem with {{Puppet::Pops::Type::StringConverter.sort_formats}} its passed an array which it sorts based on assignablity and rank order. This method on Ruby is not consistent in the order of the hash it ends up creating.

I had it process 3 entries from in the array, in the normal order and reverse order and then printed out the values:

{code: ruby}
   sort_formats(merged[-6..-4])
   sort_formats(merged[-6..-4].reverse)
{code}

The array's after sorting look like this. These arrays should be sorted in the same order:

{noformat}
format_map:
key: Hash rank: 2
key: Array rank: 4
key: Binary rank: 0

format_map:
key: Binary rank: 0
key: Hash rank: 2
key: Array rank: 4
{noformat}

I think there are a couple issues with this part of the code:

{code}
        else
          # arbitrary order if disjunct (based on name of type)
          rank_a = type_rank(a)
          rank_b = type_rank(b)
          if rank_a == 0 || rank_b == 0
            a.to_s <=> b.to_s
          else
            rank_a <=> rank_b
          end
{code}

Why do we ignore the rank if any one of the key's has a rank of 0?

Do we want a sort of this instead? where we use the rank if one of them is set and reverse the rank comparison so that high ranked items are higher?
Or should we be only using the {{a.to_s <=> b.to_s}} check when the ranks match

{code}
          if rank_a == 0 && rank_b == 0
            a.to_s <=> b.to_s
          else
            rank_b <=> rank_a
          end
{code}