Semantic Equal on Sets expects identical order of elements

[briankassouf]

Module version

% go list -m github.com/hashicorp/terraform-plugin-framework
github.com/hashicorp/terraform-plugin-framework v1.13.0

Relevant provider source code

Schema with SetNestedAttribute with a custom type CaseInsensitiveStringType that implements StringSemanticEquals.

	resp.Schema = schema.Schema{
		Attributes: map[string]schema.Attribute{
			"id": schema.StringAttribute{
				Computed:    true,
				PlanModifiers: []planmodifier.String{
					stringplanmodifier.UseStateForUnknown(),
				},
			},
			"set_attribute": schema.SetNestedAttribute{
				Optional:    true,
				NestedObject: schema.NestedAttributeObject{
					Attributes: map[string]schema.Attribute{
						"resource_id": schema.StringAttribute{
							Required:    true,
						},
						"value": schema.StringAttribute{
							CustomType:  internaltypes.CaseInsensitiveStringType{},
							Description: "(case-insensitive) custom type",
							Required:    true,
						},
					},
				},
			},
		},
	}

Terraform Configuration Files

resource "..." "..." {
  set_attribute = [
    {
      resource_id = "id1"
      value   = "Value"
    },
    {
      resource_id = "id2"
      value   = "Value2"
    }
  ]
}

Expected Behavior

Sets should not care about the order of the elements contained within. This is how the SetValue.Equals function behaves here:

terraform-plugin-framework/types/basetypes/set_value.go

Lines 282 to 286 in 4260861

	for _, elem := range s.elements {
	if !other.contains(elem) {
	return false
	}
	}

This property should hold for both Equals and Semantic Equals

Actual Behavior

Semantic Equals compares elements based on their index in the elements array and will result in plan changes if the order of elements and case of value change during Read on the Resource:

terraform-plugin-framework/internal/fwschemadata/value_semantic_equality_set.go

Lines 131 to 166 in 4260861

	// Loop through proposed elements by delegating to the recursive semantic
	// equality logic. This ensures that recursion will catch a further
	// underlying element type has its semantic equality logic checked, even if
	// the current element type does not implement the interface.
	for idx, proposedNewValueElement := range proposedNewValueElements {
	// Ensure new value always contains all of proposed new value
	newValueElements[idx] = proposedNewValueElement
	if idx >= len(priorValueElements) {
	continue
	}
	elementReq := ValueSemanticEqualityRequest{
	Path: req.Path.AtSetValue(proposedNewValueElement),
	PriorValue: priorValueElements[idx],
	ProposedNewValue: proposedNewValueElement,
	}
	elementResp := &ValueSemanticEqualityResponse{
	NewValue: elementReq.ProposedNewValue,
	}
	ValueSemanticEquality(ctx, elementReq, elementResp)
	resp.Diagnostics.Append(elementResp.Diagnostics...)
	if resp.Diagnostics.HasError() {
	return
	}
	if elementResp.NewValue.Equal(elementReq.ProposedNewValue) {
	continue
	}
	updatedElements = true
	newValueElements[idx] = elementResp.NewValue
	}

This results in a plan output like:

  ~ resource "..." "..." {
        id                 = "..."
      ~ set_attribute = [
          - {
              - resource_id = "id1" -> null
              - value   = "value" -> null
            },
          - {
              - resource_id = "id2" -> null
              - value   = "value2" -> null
            },
          + {
              + resource_id = "id1"
              + value   = "Value"
            },
          + {
              + resource_id = "id2"
              + value   = "Value2"
            },
        ]
    }

[austinvalle]

Hey there @briankassouf 👋🏻 , thanks for reporting the bug and sorry you're running into trouble here.

That definitely doesn't look correct and I think we should be able to resolve this with a tweak to that logic. There are a good number of places where sets can cause issues, although this one seems easily avoidable.

I quickly drafted a fix in #1064 that you can try out if you'd like:

go get -u github.com/hashicorp/terraform-plugin-framework@37749fd91ce93bc9490f52ab6a1c6ab96b2cc52e

[briankassouf]

Thanks @austinvalle, I tried that patch out and it appears to fix the issue!

[briankassouf]

@austinvalle Curious if there are any updates on the timeline for merging your patch?

[austinvalle]

Hey there @briankassouf 👋🏻 , apologies but I haven't had a chance to return to this bug yet, but I'm hoping to spend some time on it in the next few weeks.

Unforuntately, it's going to have a negative impact on all set performance in general, although there isn't really a way to avoid doing comparisons like this, since I believe that patch is the correct way to do it 🤷🏻. (also sets in general will always perform poorly with a large amount of elements in Terraform, because the same problem we have here is also repeated on the Terraform CLI side of things as well. If you can avoid using sets it's almost always preferable 😆 )

I'll ping back here with any updates!