From bf1d023e7887c45e4fcaa4dd3b80641a0a94602b Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Wed, 12 Feb 2025 10:29:33 -0500 Subject: [PATCH] internal/fwschemadata: Rewrite `SetValue` semantic equality logic to ignore order (#1064) * quick rewrite of semantic equality for sets * add unit tests for fix * Revert "quick rewrite of semantic equality for sets" This reverts commit 37749fd91ce93bc9490f52ab6a1c6ab96b2cc52e. * fix the semantic equal custom type bump * Revert "Revert "quick rewrite of semantic equality for sets"" This reverts commit b37dde16679cbbcd2e1927dfc31cf999195d6213. * changelog * small change * comment * comment updates * remove unnecessary check on prior element length --- .../unreleased/BUG FIXES-20250117-110109.yaml | 6 + .../value_semantic_equality_set.go | 59 +-- .../value_semantic_equality_set_test.go | 368 ++++++++++++++++++ .../testtypes/stringwithsemanticequals.go | 19 +- 4 files changed, 424 insertions(+), 28 deletions(-) create mode 100644 .changes/unreleased/BUG FIXES-20250117-110109.yaml diff --git a/.changes/unreleased/BUG FIXES-20250117-110109.yaml b/.changes/unreleased/BUG FIXES-20250117-110109.yaml new file mode 100644 index 000000000..8ce6b651d --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20250117-110109.yaml @@ -0,0 +1,6 @@ +kind: BUG FIXES +body: 'internal/fwschemadata: Set semantic equality logic has been adjusted and will + now ignore order of elements during comparison.' +time: 2025-01-17T11:01:09.848503-05:00 +custom: + Issue: "1061" diff --git a/internal/fwschemadata/value_semantic_equality_set.go b/internal/fwschemadata/value_semantic_equality_set.go index 1afe626f4..4ad2ea946 100644 --- a/internal/fwschemadata/value_semantic_equality_set.go +++ b/internal/fwschemadata/value_semantic_equality_set.go @@ -136,33 +136,40 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua // Ensure new value always contains all of proposed new value newValueElements[idx] = proposedNewValueElement - if idx >= len(priorValueElements) { - continue + // Loop through all prior value elements and see if there are any semantically equal elements + for pIdx, priorValueElement := range priorValueElements { + elementReq := ValueSemanticEqualityRequest{ + Path: req.Path.AtSetValue(proposedNewValueElement), + PriorValue: priorValueElement, + 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) { + // This prior value element didn't match, but there could be other elements that do + continue + } + + // Prior state was kept, meaning that we found a semantically equal element + updatedElements = true + + // Remove the semantically equal element from the slice of candidates + priorValueElements = append(priorValueElements[:pIdx], priorValueElements[pIdx+1:]...) + + // Order doesn't matter, so we can just set the prior state element to this index + newValueElements[idx] = elementResp.NewValue + break } - - 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 } // No changes required if the elements were not updated. diff --git a/internal/fwschemadata/value_semantic_equality_set_test.go b/internal/fwschemadata/value_semantic_equality_set_test.go index 59b34856b..d5fa25c85 100644 --- a/internal/fwschemadata/value_semantic_equality_set_test.go +++ b/internal/fwschemadata/value_semantic_equality_set_test.go @@ -50,6 +50,34 @@ func TestValueSemanticEqualitySet(t *testing.T) { ), }, }, + "SetValue-diff-order": { + request: fwschemadata.ValueSemanticEqualityRequest{ + Path: path.Root("test"), + PriorValue: types.SetValueMust( + types.StringType, + []attr.Value{ + types.StringValue("prior"), + types.StringValue("value"), + }, + ), + ProposedNewValue: types.SetValueMust( + types.StringType, + []attr.Value{ + types.StringValue("value"), + types.StringValue("new"), + }, + ), + }, + expected: &fwschemadata.ValueSemanticEqualityResponse{ + NewValue: types.SetValueMust( + types.StringType, + []attr.Value{ + types.StringValue("value"), + types.StringValue("new"), + }, + ), + }, + }, // ElementType with semantic equality "SetValue-StringValuableWithSemanticEquals-true": { request: fwschemadata.ValueSemanticEqualityRequest{ @@ -91,6 +119,64 @@ func TestValueSemanticEqualitySet(t *testing.T) { ), }, }, + "SetValue-StringValuableWithSemanticEquals-true-diff-order": { + request: fwschemadata.ValueSemanticEqualityRequest{ + Path: path.Root("test"), + PriorValue: types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{}, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-123"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-123"), + }, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-456"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-456"), + }, + }, + }, + ), + ProposedNewValue: types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{}, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-456"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-456"), + }, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-123"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-123"), + }, + }, + }, + ), + }, + expected: &fwschemadata.ValueSemanticEqualityResponse{ + NewValue: types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{}, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-123"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-123"), + }, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-456"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-456"), + }, + }, + }, + ), + }, + }, "SetValue-StringValuableWithSemanticEquals-false": { request: fwschemadata.ValueSemanticEqualityRequest{ Path: path.Root("test"), @@ -131,6 +217,58 @@ func TestValueSemanticEqualitySet(t *testing.T) { ), }, }, + "SetValue-StringValuableWithSemanticEquals-false-diff-order": { + request: fwschemadata.ValueSemanticEqualityRequest{ + Path: path.Root("test"), + PriorValue: types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("prior"), + SemanticEquals: false, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("value"), + SemanticEquals: false, + }, + }, + ), + ProposedNewValue: types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("value"), + SemanticEquals: false, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("new"), + SemanticEquals: false, + }, + }, + ), + }, + expected: &fwschemadata.ValueSemanticEqualityResponse{ + NewValue: types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("value"), + SemanticEquals: false, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("new"), + SemanticEquals: false, + }, + }, + ), + }, + }, "SetValue-StringValuableWithSemanticEquals-diagnostics": { request: fwschemadata.ValueSemanticEqualityRequest{ Path: path.Root("test"), @@ -267,6 +405,136 @@ func TestValueSemanticEqualitySet(t *testing.T) { ), }, }, + "SetValue-SetValue-StringValuableWithSemanticEquals-true-diff-order": { + request: fwschemadata.ValueSemanticEqualityRequest{ + Path: path.Root("test"), + PriorValue: types.SetValueMust( + types.SetType{ + ElemType: testtypes.StringTypeWithSemanticEquals{}, + }, + []attr.Value{ + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{}, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-123"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-123"), + }, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-456"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-456"), + }, + }, + }, + ), + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{}, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-789"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-789"), + }, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-012"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-012"), + }, + }, + }, + ), + }, + ), + ProposedNewValue: types.SetValueMust( + types.SetType{ + ElemType: testtypes.StringTypeWithSemanticEquals{}, + }, + []attr.Value{ + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{}, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-012"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-012"), + }, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-789"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-789"), + }, + }, + }, + ), + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{}, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-456"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-456"), + }, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-123"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-123"), + }, + }, + }, + ), + }, + ), + }, + expected: &fwschemadata.ValueSemanticEqualityResponse{ + NewValue: types.SetValueMust( + types.SetType{ + ElemType: testtypes.StringTypeWithSemanticEquals{}, + }, + []attr.Value{ + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{}, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-123"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-123"), + }, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-456"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-456"), + }, + }, + }, + ), + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{}, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-789"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-789"), + }, + }, + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("keep-lowercase-012"), + SemanticallyEqualTo: testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("KEEP-LOWERCASE-012"), + }, + }, + }, + ), + }, + ), + }, + }, "SetValue-SetValue-StringValuableWithSemanticEquals-false": { request: fwschemadata.ValueSemanticEqualityRequest{ Path: path.Root("test"), @@ -334,6 +602,106 @@ func TestValueSemanticEqualitySet(t *testing.T) { ), }, }, + "SetValue-SetValue-StringValuableWithSemanticEquals-false-diff-order": { + request: fwschemadata.ValueSemanticEqualityRequest{ + Path: path.Root("test"), + PriorValue: types.SetValueMust( + types.SetType{ + ElemType: testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + }, + []attr.Value{ + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("prior"), + SemanticEquals: false, + }, + }, + ), + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("value"), + SemanticEquals: false, + }, + }, + ), + }, + ), + ProposedNewValue: types.SetValueMust( + types.SetType{ + ElemType: testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + }, + []attr.Value{ + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("value"), + SemanticEquals: false, + }, + }, + ), + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("new"), + SemanticEquals: false, + }, + }, + ), + }, + ), + }, + expected: &fwschemadata.ValueSemanticEqualityResponse{ + NewValue: types.SetValueMust( + types.SetType{ + ElemType: testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + }, + []attr.Value{ + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("value"), + SemanticEquals: false, + }, + }, + ), + types.SetValueMust( + testtypes.StringTypeWithSemanticEquals{ + SemanticEquals: false, + }, + []attr.Value{ + testtypes.StringValueWithSemanticEquals{ + StringValue: types.StringValue("new"), + SemanticEquals: false, + }, + }, + ), + }, + ), + }, + }, "SetValue-SetValue-StringValuableWithSemanticEquals-NewValueElementsGreaterThanPriorValueElements": { request: fwschemadata.ValueSemanticEqualityRequest{ Path: path.Root("test"), diff --git a/internal/testing/testtypes/stringwithsemanticequals.go b/internal/testing/testtypes/stringwithsemanticequals.go index 4baf39d4c..a98c06a24 100644 --- a/internal/testing/testtypes/stringwithsemanticequals.go +++ b/internal/testing/testtypes/stringwithsemanticequals.go @@ -24,7 +24,12 @@ var ( type StringTypeWithSemanticEquals struct { basetypes.StringType - SemanticEquals bool + // Will always return this boolean for semantic equality + SemanticEquals bool + + // Will only return semantic equality as true if the attr.Value matches this + SemanticallyEqualTo attr.Value + SemanticEqualsDiagnostics diag.Diagnostics } @@ -52,6 +57,7 @@ func (t StringTypeWithSemanticEquals) ValueFromString(ctx context.Context, in ba value := StringValueWithSemanticEquals{ StringValue: in, SemanticEquals: t.SemanticEquals, + SemanticallyEqualTo: t.SemanticallyEqualTo, SemanticEqualsDiagnostics: t.SemanticEqualsDiagnostics, } @@ -83,6 +89,7 @@ func (t StringTypeWithSemanticEquals) ValueFromTerraform(ctx context.Context, in func (t StringTypeWithSemanticEquals) ValueType(ctx context.Context) attr.Value { return StringValueWithSemanticEquals{ SemanticEquals: t.SemanticEquals, + SemanticallyEqualTo: t.SemanticallyEqualTo, SemanticEqualsDiagnostics: t.SemanticEqualsDiagnostics, } } @@ -90,7 +97,12 @@ func (t StringTypeWithSemanticEquals) ValueType(ctx context.Context) attr.Value type StringValueWithSemanticEquals struct { basetypes.StringValue - SemanticEquals bool + // Will always return this boolean for semantic equality + SemanticEquals bool + + // Will only return semantic equality as true if the attr.Value matches this + SemanticallyEqualTo attr.Value + SemanticEqualsDiagnostics diag.Diagnostics } @@ -105,6 +117,9 @@ func (v StringValueWithSemanticEquals) Equal(o attr.Value) bool { } func (v StringValueWithSemanticEquals) StringSemanticEquals(ctx context.Context, otherV basetypes.StringValuable) (bool, diag.Diagnostics) { + if v.SemanticallyEqualTo != nil && !v.SemanticallyEqualTo.IsNull() { + return v.SemanticallyEqualTo.Equal(otherV), v.SemanticEqualsDiagnostics + } return v.SemanticEquals, v.SemanticEqualsDiagnostics }