Canonicalisation of Enum[] is not idempotent

[Clebam]

Describe the Bug

When the casing of a value inside an Enum does not match what puppet retrieves on the system, it reapplies the value.

Expected Behavior

Among the casing accepted in the Enum (generally PascalCased and lowercased, ie Enum['MyValue', 'myvalue']), both should be idempotent.

# In the manifest
param = myvalue
param2 = MyValue
param3 = MYVALUE

# In the system, even with the value being MyValue, both param and param2 should result in no change because it matches the enum values and the system values (case insentively)
param 3 should throw because it is not in the Enum even though it matches insensitively

Steps to Reproduce

Steps to reproduce the behavior:

1. Use webadministrationdsc module (or any dsc module in fact)
2. Use a param that is an Enum and put all lowercased
3. Execute several times and see that it reapplies each time the value

Notice: /Stage[main]/Sw_iis::Components::Webapppool/Dsc_webapppool[DefaultAppPool]/dsc_ensure: dsc_ensure changed 'Present' to 'present' (corrective)

Environment

• Version 7.23.0
• Platform Windows Server 2019 Standard

Additional Context

I opened a related issue previously (#254) and had a discussion on slack (here https://puppetcommunity.slack.com/archives/CFD8Z9A4T/p1696342031154069)
I also commented here recently #131 (comment)

I made a few more test and was able to better understand the issue.

First of all the issue comes from this commit: 0dd0714#diff-2ab4c6b5047cd9e5464cf56d34d7953e1aee0facd20a4739cd767dbb335abfcbR82

The behavior before the commit implied that the system could return a value that was not in the Enum (as described here : #127)

So the fix overrides the System value with the manifest value if the type of the current parameter is an Enum.

However this implies another bug. The value in the manifest will be compared to the value in the System after the canonicalization. And the casing of the manifest and the casing of the System value may not match and will produce a change at each run.

I will try to describe step by step how it works

# Before Execution
# System Value for "MyParam" is "MyValue"
# Manifest Value for "MyParam" is "myvalue"
# MyParam is referenced as Type: Enum["MyValue","myvalue","OtherValue","othervalue"]

1. Value is read on manifest ==> `myvalue`
2. Value is read on the system ==> `MyValue`
3. Canonicalized Value is assigned the value read on the system ==> `MyValue` # It is not yet it's definitive value
4. We check if MyParam is an Enum. If it is an Enum, we will assign manifest value to Canonicalized Value. ==> true # it is indeed an Enum
5. Canonicalized Value is assigned the value read on the manifest ==> `myvalue`
6. End of canonicalization
7. The canonicalized value is tested against the type constraint (here, an Enum). The value is in the Enum, it can continue.
8. The canonicalized value is tested against the system by puppet to see if it needs to apply the value
9. Canonicalized Value `myvalue` does not match System Value `MyValue` (the test is case sensitive)
10. Puppet reapplies `myvalue` to replace `MyValue`
11. The system has been updated, but under the hood, it converted by itself `myvalue` to `MyValue`, because itself is case insentive and handle himself the casing.
12. Rinse and repeat

All of what I describe from step 1 to step 5 is in the canonicalize function ine dsc_base_provider.rb
The step 4 and 5 specifically are linked to this piece of code : 0dd0714#diff-2ab4c6b5047cd9e5464cf56d34d7953e1aee0facd20a4739cd767dbb335abfcbR82

I tried a fix that seem to both fix my issue (constant change) and the former (System Value not being in enum) with a few changes

downcased_result.each do |key, value|
  # Canonicalize to the manifest value unless the downcased strings match and the attribute is not an enum:
  # - When the values don't match at all, the manifest value is desired;
  # - When the values match case insensitively but the attribute is an enum, and the casing from invoke_get_method
  #   is not int the enum, prefer the casing of the manifest enum.
  # - When the values match case insensitively and the attribute is not an enum, or is an enum and invoke_get_method casing
  #   is in the enum, prefer the casing from invoke_get_method
  is_enum = enum_attributes(context).include?(key)
  if (is_enum)
    canonicalized_value_in_enum = enum_values(context, key).include?(canonicalized[key])
  else
    canonicalized_value_in_enum = false
  end
  match_insensitively = same?(value, downcased_resource[key])
  canonicalized[key] = r[key] unless match_insensitively && (canonicalized_value_in_enum || !is_enum)
  canonicalized.delete(key) unless downcased_resource.key?(key)
end

[...]

def enum_attributes(context)
    context.type.attributes.select { |_name, properties| properties[:type].include?('Enum[') }.keys
  end

  def enum_values(context, key)
    # Get the attribute's type string for the given key
    type_string = context.type.attributes[key].dig(:type) # Using dig to avoid nil errors

    # Return an empty array if the key doesn't have an Enum type or doesn't exist
    return [] unless type_string&.include?('Enum[')

    # Extract the enum values from the type string
    enum_content = type_string.match(/Enum\[(.*?)\]/)[1] rescue nil # Use match and rescue to avoid nil errors

    # Return an empty array if we couldn't find the enum values
    return [] if enum_content.nil?

    # Return an array of the enum values, stripped of extra whitespace and quote marks
    enum_content.split(',').map { |val| val.strip.gsub("'", "") }
  end

[...]

The whole idea is to check if the value returned by the System is in the Enum or not.
If not ==> use manifest value

This avoids constant change from happening but protects from wrong values being tested against the Enum.
It should be noted that the former issue with value not being in the Enum is an error on the external module itself. The Enum should have listed the values returned by the system

[nickgw]

👍🏼 I've definitely encountered exactly this and it is a huge blocker from adopting dsc resources.

[jordanbreen28]

@Clebam apologies for the delay in response. This might take some time for our team to look at and implement. This repo is open source however, so you can raise a PR with your fix if you wish and we can get it out faster!
Thanks for raising this.

[brajjan]

@jordanbreen28 I just validated the fix for this and the updated code works for me. Just a short test on one Enum on one Windows Server. @Clebam please open a PR for this!