(PUP-11663) Enable more types in max/min functions

Prior to this commit, the max and min core functions would compare
SemVer, Timespan, and Timestamp types as Strings, which would lead
to often incorrect results (e.g. max would state that a SemVer of
2.0.0 was greater than 10.0.0).

This commit updates max and min to recognize SemVer, Timespan, and
Timestamp types properly.

Author: mhashizume

lib/puppet/functions/max.rb

@@ -82,6 +82,18 @@
     repeated_param 'String', :values
   end
 
+  dispatch :on_semver do
+    repeated_param 'Semver', :values
+  end
+
+  dispatch :on_timespan do
+    repeated_param 'Timespan', :values
+  end
+
+  dispatch :on_timestamp do
+    repeated_param 'Timestamp', :values
+  end
+
   dispatch :on_single_numeric_array do
     param 'Array[Numeric]', :values
     optional_block_param 'Callable[2,2]', :block
@@ -92,6 +104,21 @@
     optional_block_param 'Callable[2,2]', :block
   end
 
+  dispatch :on_single_semver_array do
+    param 'Array[Semver]', :values
+    optional_block_param 'Callable[2,2]', :block
+  end
+
+  dispatch :on_single_timespan_array do
+    param 'Array[Timespan]', :values
+    optional_block_param 'Callable[2,2]', :block
+  end
+
+  dispatch :on_single_timestamp_array do
+    param 'Array[Timestamp]', :values
+    optional_block_param 'Callable[2,2]', :block
+  end
+
   dispatch :on_single_any_array do
     param 'Array', :values
     optional_block_param 'Callable[2,2]', :block
@@ -129,6 +156,21 @@ def on_string(*args)
     end
   end
 
+  def on_semver(*args)
+    assert_arg_count(args)
+    args.max
+  end
+
+  def on_timespan(*args)
+    assert_arg_count(args)
+    args.max
+  end
+
+  def on_timestamp(*args)
+    assert_arg_count(args)
+    args.max
+  end
+
   def on_any_with_block(*args, &block)
     args.max {|x,y| block.call(x,y) }
   end
@@ -149,6 +191,30 @@ def on_single_string_array(array, &block)
     end
   end
 
+  def on_single_semver_array(array, &block)
+    if block_given?
+      on_any_with_block(*array, &block)
+    else
+      on_semver(*array)
+    end
+  end
+
+  def on_single_timespan_array(array, &block)
+    if block_given?
+      on_any_with_block(*array, &block)
+    else
+      on_timespan(*array)
+    end
+  end
+
+  def on_single_timestamp_array(array, &block)
+    if block_given?
+      on_any_with_block(*array, &block)
+    else
+      on_timestamp(*array)
+    end
+  end
+
   def on_single_any_array(array, &block)
     if block_given?
       on_any_with_block(*array, &block)

lib/puppet/functions/min.rb

@@ -81,11 +81,38 @@
     repeated_param 'String', :values
   end
 
+  dispatch :on_semver do
+    repeated_param 'Semver', :values
+  end
+
+  dispatch :on_timespan do
+    repeated_param 'Timespan', :values
+  end
+
+  dispatch :on_timestamp do
+    repeated_param 'Timestamp', :values
+  end
+
   dispatch :on_single_numeric_array do
     param 'Array[Numeric]', :values
     optional_block_param 'Callable[2,2]', :block
   end
 
+  dispatch :on_single_semver_array do
+    param 'Array[Semver]', :values
+    optional_block_param 'Callable[2,2]', :block
+  end
+
+  dispatch :on_single_timespan_array do
+    param 'Array[Timespan]', :values
+    optional_block_param 'Callable[2,2]', :block
+  end
+
+  dispatch :on_single_timestamp_array do
+    param 'Array[Timestamp]', :values
+    optional_block_param 'Callable[2,2]', :block
+  end
+
   dispatch :on_single_string_array do
     param 'Array[String]', :values
     optional_block_param 'Callable[2,2]', :block
@@ -128,6 +155,21 @@ def on_string(*args)
     end
   end
 
+  def on_semver(*args)
+    assert_arg_count(args)
+    args.min
+  end
+
+  def on_timespan(*args)
+    assert_arg_count(args)
+    args.min
+  end
+
+  def on_timestamp(*args)
+    assert_arg_count(args)
+    args.min
+  end
+
   def on_any_with_block(*args, &block)
     args.min {|x,y| block.call(x,y) }
   end
@@ -148,6 +190,30 @@ def on_single_string_array(array, &block)
     end
   end
 
+  def on_single_semver_array(array, &block)
+    if block_given?
+      on_any_with_block(*array, &block)
+    else
+      on_semver(*array)
+    end
+  end
+
+  def on_single_timespan_array(array, &block)
+    if block_given?
+      on_any_with_block(*array, &block)
+    else
+      on_timespan(*array)
+    end
+  end
+
+  def on_single_timestamp_array(array, &block)
+    if block_given?
+      on_any_with_block(*array, &block)
+    else
+      on_timestamp(*array)
+    end
+  end
+
   def on_single_any_array(array, &block)
     if block_given?
       on_any_with_block(*array, &block)

spec/unit/functions/max_spec.rb

@@ -67,14 +67,43 @@
 
   end
 
+  context 'compares semver' do
+    { ["Semver('2.0.0')", "Semver('10.0.0')"] => "Semver('10.0.0')",
+      ["Semver('5.5.5')", "Semver('5.6.7')"]  => "Semver('5.6.7')",
+    }.each_pair do |values, expected|
+      it "called as max(#{values[0]}, #{values[1]}) results in the value #{expected}" do
+        expect(compile_to_catalog("notify { String( max(#{values[0]}, #{values[1]}) == #{expected}): }")).to have_resource("Notify[true]")
+      end
+    end
+  end
+
+  context 'compares timespans' do
+    { ["Timespan(2)", "Timespan(77.3)"] => "Timespan(77.3)",
+      ["Timespan('1-00:00:00')", "Timespan('2-00:00:00')"]  => "Timespan('2-00:00:00')",
+    }.each_pair do |values, expected|
+      it "called as max(#{values[0]}, #{values[1]}) results in the value #{expected}" do
+        expect(compile_to_catalog("notify { String( max(#{values[0]}, #{values[1]}) == #{expected}): }")).to have_resource("Notify[true]")
+      end
+    end
+  end
+
+  context 'compares timestamps' do
+    { ["Timestamp(0)", "Timestamp(298922400)"] => "Timestamp(298922400)",
+      ["Timestamp('1970-01-01T12:00:00.000')", "Timestamp('1979-06-22T18:00:00.000')"]  => "Timestamp('1979-06-22T18:00:00.000')",
+    }.each_pair do |values, expected|
+      it "called as max(#{values[0]}, #{values[1]}) results in the value #{expected}" do
+        expect(compile_to_catalog("notify { String( max(#{values[0]}, #{values[1]}) == #{expected}): }")).to have_resource("Notify[true]")
+      end
+    end
+  end
+
   context 'compares all except numeric and string by conversion to string and issues deprecation warning' do
     {
       [[20], "'a'"]                  => "'a'",            # after since '[' is before 'a'
       ["{'a' => 10}", "'a'"]         => "{'a' => 10}",    # after since '{' is after 'a'
       [false, 'fal']                 => "false",          # the boolean since text 'false' is longer
       ['/b/', "'(?-mix:c)'"]         => "'(?-mix:c)'",    # because regexp to_s is a (?-mix:b) string
       ["Timestamp(1)", "'1980 a.d'"] => "'1980 a.d'",     # because timestamp to_s is a date-time string here starting with 1970
-      ["Semver('2.0.0')", "Semver('10.0.0')"] => "Semver('2.0.0')", # "10.0.0" is lexicographically before "2.0.0"
     }.each_pair do |values, expected|
       it "called as max(#{values[0]}, #{values[1]}) results in the value #{expected} and issues deprecation warning" do
         Puppet::Util::Log.with_destination(Puppet::Test::LogCollector.new(logs)) do

spec/unit/functions/min_spec.rb

@@ -67,14 +67,43 @@
 
   end
 
+  context 'compares semver' do
+    { ["Semver('2.0.0')", "Semver('10.0.0')"] => "Semver('2.0.0')",
+      ["Semver('5.5.5')", "Semver('5.6.7')"]  => "Semver('5.5.5')",
+    }.each_pair do |values, expected|
+      it "called as min(#{values[0]}, #{values[1]}) results in the value #{expected}" do
+        expect(compile_to_catalog("notify { String( min(#{values[0]}, #{values[1]}) == #{expected}): }")).to have_resource("Notify[true]")
+      end
+    end
+  end
+
+  context 'compares timespans' do
+    { ["Timespan(2)", "Timespan(77.3)"] => "Timespan(2)",
+      ["Timespan('1-00:00:00')", "Timespan('2-00:00:00')"]  => "Timespan('1-00:00:00')",
+    }.each_pair do |values, expected|
+      it "called as min(#{values[0]}, #{values[1]}) results in the value #{expected}" do
+        expect(compile_to_catalog("notify { String( min(#{values[0]}, #{values[1]}) == #{expected}): }")).to have_resource("Notify[true]")
+      end
+    end
+  end
+
+  context 'compares timestamps' do
+    { ["Timestamp(0)", "Timestamp(298922400)"] => "Timestamp(0)",
+      ["Timestamp('1970-01-01T12:00:00.000')", "Timestamp('1979-06-22T18:00:00.000')"]  => "Timestamp('1970-01-01T12:00:00.000')",
+    }.each_pair do |values, expected|
+      it "called as min(#{values[0]}, #{values[1]}) results in the value #{expected}" do
+        expect(compile_to_catalog("notify { String( min(#{values[0]}, #{values[1]}) == #{expected}): }")).to have_resource("Notify[true]")
+      end
+    end
+  end
+
   context 'compares all except numeric and string by conversion to string (and issues deprecation warning)' do
     {
       [[20], "'a'"]                  => [20],             # before since '[' is before 'a'
       ["{'a' => 10}", "'|a'"]         => "{'a' => 10}",   # before since '{' is before '|'
       [false, 'fal']                 => "'fal'",          # 'fal' before since shorter than 'false'
       ['/b/', "'(?-mix:a)'"]         => "'(?-mix:a)'",    # because regexp to_s is a (?-mix:b) string
       ["Timestamp(1)", "'1556 a.d'"] => "'1556 a.d'",     # because timestamp to_s is a date-time string here starting with 1970
-      ["Semver('2.0.0')", "Semver('10.0.0')"] => "Semver('10.0.0')", # "10.0.0" is lexicographically before "2.0.0"
     }.each_pair do |values, expected|
       it "called as min(#{values[0]}, #{values[1]}) results in the value #{expected} and issues deprecation warning" do
         Puppet::Util::Log.with_destination(Puppet::Test::LogCollector.new(logs)) do