From 7ad0d4660cc4a868824c4f314a32b1b1112d6b01 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Fri, 6 Dec 2024 17:42:47 -0500 Subject: [PATCH 01/10] quick rewrite of semantic equality for sets --- .../value_semantic_equality_set.go | 53 ++++++++++++------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/internal/fwschemadata/value_semantic_equality_set.go b/internal/fwschemadata/value_semantic_equality_set.go index 1afe626f4..6ba878d73 100644 --- a/internal/fwschemadata/value_semantic_equality_set.go +++ b/internal/fwschemadata/value_semantic_equality_set.go @@ -128,6 +128,10 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua // Short circuit flag updatedElements := false + // The underlying loop will mutate priorValueElements to avoid keeping + // duplicate semantically equal elements. Need the original length to avoid panicks + originalPriorElementsLength := len(priorValueElements) + // 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 @@ -136,33 +140,44 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua // Ensure new value always contains all of proposed new value newValueElements[idx] = proposedNewValueElement - if idx >= len(priorValueElements) { + if idx >= originalPriorElementsLength { continue } - elementReq := ValueSemanticEqualityRequest{ - Path: req.Path.AtSetValue(proposedNewValueElement), - PriorValue: priorValueElements[idx], - ProposedNewValue: proposedNewValueElement, - } - elementResp := &ValueSemanticEqualityResponse{ - NewValue: elementReq.ProposedNewValue, - } + // Loop through all prior value elements and see if there are any semantically equal elements + for pIdx, priorValue := range priorValueElements { + elementReq := ValueSemanticEqualityRequest{ + Path: req.Path.AtSetValue(proposedNewValueElement), + PriorValue: priorValue, + ProposedNewValue: proposedNewValueElement, + } + elementResp := &ValueSemanticEqualityResponse{ + NewValue: elementReq.ProposedNewValue, + } - ValueSemanticEquality(ctx, elementReq, elementResp) + ValueSemanticEquality(ctx, elementReq, elementResp) - resp.Diagnostics.Append(elementResp.Diagnostics...) + resp.Diagnostics.Append(elementResp.Diagnostics...) - if resp.Diagnostics.HasError() { - return - } + if resp.Diagnostics.HasError() { + return + } - if elementResp.NewValue.Equal(elementReq.ProposedNewValue) { - continue - } + 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 - updatedElements = true - newValueElements[idx] = elementResp.NewValue + // 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 + } } // No changes required if the elements were not updated. From 5ab079b938b7f95b4ab1826376b408dfe059ae91 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Fri, 17 Jan 2025 10:39:38 -0500 Subject: [PATCH 02/10] add unit tests for fix --- .../value_semantic_equality_set_test.go | 368 ++++++++++++++++++ .../testtypes/stringwithsemanticequals.go | 20 +- 2 files changed, 386 insertions(+), 2 deletions(-) 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..a317f3dd3 100644 --- a/internal/testing/testtypes/stringwithsemanticequals.go +++ b/internal/testing/testtypes/stringwithsemanticequals.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" "github.com/hashicorp/terraform-plugin-go/tftypes" ) @@ -24,7 +25,12 @@ var ( type StringTypeWithSemanticEquals struct { basetypes.StringType - SemanticEquals bool + // Will always return true for semantic equality + SemanticEquals bool + + // Will only return semantic equality as true if the string matches this + SemanticallyEqualTo types.String + SemanticEqualsDiagnostics diag.Diagnostics } @@ -52,6 +58,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 +90,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 +98,12 @@ func (t StringTypeWithSemanticEquals) ValueType(ctx context.Context) attr.Value type StringValueWithSemanticEquals struct { basetypes.StringValue - SemanticEquals bool + // Will always return true for semantic equality + SemanticEquals bool + + // Will only return semantic equality as true if the string matches this + SemanticallyEqualTo attr.Value + SemanticEqualsDiagnostics diag.Diagnostics } @@ -105,6 +118,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 { + return v.SemanticallyEqualTo.Equal(otherV), v.SemanticEqualsDiagnostics + } return v.SemanticEquals, v.SemanticEqualsDiagnostics } From 92d4d2e83d50925c932ae54fc96e09b5de307209 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Fri, 17 Jan 2025 10:40:33 -0500 Subject: [PATCH 03/10] Revert "quick rewrite of semantic equality for sets" This reverts commit 37749fd91ce93bc9490f52ab6a1c6ab96b2cc52e. --- .../value_semantic_equality_set.go | 53 +++++++------------ 1 file changed, 19 insertions(+), 34 deletions(-) diff --git a/internal/fwschemadata/value_semantic_equality_set.go b/internal/fwschemadata/value_semantic_equality_set.go index 6ba878d73..1afe626f4 100644 --- a/internal/fwschemadata/value_semantic_equality_set.go +++ b/internal/fwschemadata/value_semantic_equality_set.go @@ -128,10 +128,6 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua // Short circuit flag updatedElements := false - // The underlying loop will mutate priorValueElements to avoid keeping - // duplicate semantically equal elements. Need the original length to avoid panicks - originalPriorElementsLength := len(priorValueElements) - // 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 @@ -140,44 +136,33 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua // Ensure new value always contains all of proposed new value newValueElements[idx] = proposedNewValueElement - if idx >= originalPriorElementsLength { + if idx >= len(priorValueElements) { continue } - // Loop through all prior value elements and see if there are any semantically equal elements - for pIdx, priorValue := range priorValueElements { - elementReq := ValueSemanticEqualityRequest{ - Path: req.Path.AtSetValue(proposedNewValueElement), - PriorValue: priorValue, - ProposedNewValue: proposedNewValueElement, - } - elementResp := &ValueSemanticEqualityResponse{ - NewValue: elementReq.ProposedNewValue, - } - - ValueSemanticEquality(ctx, elementReq, elementResp) - - resp.Diagnostics.Append(elementResp.Diagnostics...) - - if resp.Diagnostics.HasError() { - return - } + elementReq := ValueSemanticEqualityRequest{ + Path: req.Path.AtSetValue(proposedNewValueElement), + PriorValue: priorValueElements[idx], + ProposedNewValue: proposedNewValueElement, + } + elementResp := &ValueSemanticEqualityResponse{ + NewValue: elementReq.ProposedNewValue, + } - if elementResp.NewValue.Equal(elementReq.ProposedNewValue) { - // This prior value element didn't match, but there could be other elements that do - continue - } + ValueSemanticEquality(ctx, elementReq, elementResp) - // Prior state was kept, meaning that we found a semantically equal element - updatedElements = true + resp.Diagnostics.Append(elementResp.Diagnostics...) - // Remove the semantically equal element from the slice of candidates - priorValueElements = append(priorValueElements[:pIdx], priorValueElements[pIdx+1:]...) + if resp.Diagnostics.HasError() { + return + } - // Order doesn't matter, so we can just set the prior state element to this index - newValueElements[idx] = elementResp.NewValue - break + if elementResp.NewValue.Equal(elementReq.ProposedNewValue) { + continue } + + updatedElements = true + newValueElements[idx] = elementResp.NewValue } // No changes required if the elements were not updated. From 982a40796b73f27be0c697b316bbb46e932d835e Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Fri, 17 Jan 2025 10:45:36 -0500 Subject: [PATCH 04/10] fix the semantic equal custom type bump --- internal/testing/testtypes/stringwithsemanticequals.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/testing/testtypes/stringwithsemanticequals.go b/internal/testing/testtypes/stringwithsemanticequals.go index a317f3dd3..9ae0d6bf2 100644 --- a/internal/testing/testtypes/stringwithsemanticequals.go +++ b/internal/testing/testtypes/stringwithsemanticequals.go @@ -118,7 +118,7 @@ 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 { + if v.SemanticallyEqualTo != nil && !v.SemanticallyEqualTo.IsNull() { return v.SemanticallyEqualTo.Equal(otherV), v.SemanticEqualsDiagnostics } return v.SemanticEquals, v.SemanticEqualsDiagnostics From 61e7bb94424f7b21792e3be3e535a24bf224cde4 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Fri, 17 Jan 2025 10:46:09 -0500 Subject: [PATCH 05/10] Revert "Revert "quick rewrite of semantic equality for sets"" This reverts commit b37dde16679cbbcd2e1927dfc31cf999195d6213. --- .../value_semantic_equality_set.go | 53 ++++++++++++------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/internal/fwschemadata/value_semantic_equality_set.go b/internal/fwschemadata/value_semantic_equality_set.go index 1afe626f4..6ba878d73 100644 --- a/internal/fwschemadata/value_semantic_equality_set.go +++ b/internal/fwschemadata/value_semantic_equality_set.go @@ -128,6 +128,10 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua // Short circuit flag updatedElements := false + // The underlying loop will mutate priorValueElements to avoid keeping + // duplicate semantically equal elements. Need the original length to avoid panicks + originalPriorElementsLength := len(priorValueElements) + // 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 @@ -136,33 +140,44 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua // Ensure new value always contains all of proposed new value newValueElements[idx] = proposedNewValueElement - if idx >= len(priorValueElements) { + if idx >= originalPriorElementsLength { continue } - elementReq := ValueSemanticEqualityRequest{ - Path: req.Path.AtSetValue(proposedNewValueElement), - PriorValue: priorValueElements[idx], - ProposedNewValue: proposedNewValueElement, - } - elementResp := &ValueSemanticEqualityResponse{ - NewValue: elementReq.ProposedNewValue, - } + // Loop through all prior value elements and see if there are any semantically equal elements + for pIdx, priorValue := range priorValueElements { + elementReq := ValueSemanticEqualityRequest{ + Path: req.Path.AtSetValue(proposedNewValueElement), + PriorValue: priorValue, + ProposedNewValue: proposedNewValueElement, + } + elementResp := &ValueSemanticEqualityResponse{ + NewValue: elementReq.ProposedNewValue, + } - ValueSemanticEquality(ctx, elementReq, elementResp) + ValueSemanticEquality(ctx, elementReq, elementResp) - resp.Diagnostics.Append(elementResp.Diagnostics...) + resp.Diagnostics.Append(elementResp.Diagnostics...) - if resp.Diagnostics.HasError() { - return - } + if resp.Diagnostics.HasError() { + return + } - if elementResp.NewValue.Equal(elementReq.ProposedNewValue) { - continue - } + 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 - updatedElements = true - newValueElements[idx] = elementResp.NewValue + // 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 + } } // No changes required if the elements were not updated. From fdfe1af5da48b4ac04f1d434e9c7e374bca144bd Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Fri, 17 Jan 2025 11:01:14 -0500 Subject: [PATCH 06/10] changelog --- .changes/unreleased/BUG FIXES-20250117-110109.yaml | 6 ++++++ 1 file changed, 6 insertions(+) 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" From acad0cd51bab970250ad99f539e773fe52ac0156 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Fri, 17 Jan 2025 14:47:07 -0500 Subject: [PATCH 07/10] small change --- internal/fwschemadata/value_semantic_equality_set.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/fwschemadata/value_semantic_equality_set.go b/internal/fwschemadata/value_semantic_equality_set.go index 6ba878d73..1e39dad61 100644 --- a/internal/fwschemadata/value_semantic_equality_set.go +++ b/internal/fwschemadata/value_semantic_equality_set.go @@ -145,10 +145,10 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua } // Loop through all prior value elements and see if there are any semantically equal elements - for pIdx, priorValue := range priorValueElements { + for pIdx, priorValueElement := range priorValueElements { elementReq := ValueSemanticEqualityRequest{ Path: req.Path.AtSetValue(proposedNewValueElement), - PriorValue: priorValue, + PriorValue: priorValueElement, ProposedNewValue: proposedNewValueElement, } elementResp := &ValueSemanticEqualityResponse{ From b5b62cb20471ebc5e9245a2455318cdb473722d8 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Fri, 17 Jan 2025 14:48:44 -0500 Subject: [PATCH 08/10] comment --- internal/fwschemadata/value_semantic_equality_set.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/fwschemadata/value_semantic_equality_set.go b/internal/fwschemadata/value_semantic_equality_set.go index 1e39dad61..097f7aa20 100644 --- a/internal/fwschemadata/value_semantic_equality_set.go +++ b/internal/fwschemadata/value_semantic_equality_set.go @@ -129,7 +129,7 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua updatedElements := false // The underlying loop will mutate priorValueElements to avoid keeping - // duplicate semantically equal elements. Need the original length to avoid panicks + // duplicate semantically equal elements. Need the original length to avoid a panic originalPriorElementsLength := len(priorValueElements) // Loop through proposed elements by delegating to the recursive semantic From fef2d6a087a2e3456d361f41faf0dbfea8eafef4 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Fri, 17 Jan 2025 14:51:14 -0500 Subject: [PATCH 09/10] comment updates --- .../testing/testtypes/stringwithsemanticequals.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/internal/testing/testtypes/stringwithsemanticequals.go b/internal/testing/testtypes/stringwithsemanticequals.go index 9ae0d6bf2..a98c06a24 100644 --- a/internal/testing/testtypes/stringwithsemanticequals.go +++ b/internal/testing/testtypes/stringwithsemanticequals.go @@ -9,7 +9,6 @@ import ( "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/diag" - "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" "github.com/hashicorp/terraform-plugin-go/tftypes" ) @@ -25,11 +24,11 @@ var ( type StringTypeWithSemanticEquals struct { basetypes.StringType - // Will always return true for semantic equality + // Will always return this boolean for semantic equality SemanticEquals bool - // Will only return semantic equality as true if the string matches this - SemanticallyEqualTo types.String + // Will only return semantic equality as true if the attr.Value matches this + SemanticallyEqualTo attr.Value SemanticEqualsDiagnostics diag.Diagnostics } @@ -98,10 +97,10 @@ func (t StringTypeWithSemanticEquals) ValueType(ctx context.Context) attr.Value type StringValueWithSemanticEquals struct { basetypes.StringValue - // Will always return true for semantic equality + // Will always return this boolean for semantic equality SemanticEquals bool - // Will only return semantic equality as true if the string matches this + // Will only return semantic equality as true if the attr.Value matches this SemanticallyEqualTo attr.Value SemanticEqualsDiagnostics diag.Diagnostics From c11ad07180bfb18ee7f506d3aa0c0e92bc51f69a Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Wed, 12 Feb 2025 10:19:12 -0500 Subject: [PATCH 10/10] remove unnecessary check on prior element length --- internal/fwschemadata/value_semantic_equality_set.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/internal/fwschemadata/value_semantic_equality_set.go b/internal/fwschemadata/value_semantic_equality_set.go index 097f7aa20..4ad2ea946 100644 --- a/internal/fwschemadata/value_semantic_equality_set.go +++ b/internal/fwschemadata/value_semantic_equality_set.go @@ -128,10 +128,6 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua // Short circuit flag updatedElements := false - // The underlying loop will mutate priorValueElements to avoid keeping - // duplicate semantically equal elements. Need the original length to avoid a panic - originalPriorElementsLength := len(priorValueElements) - // 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 @@ -140,10 +136,6 @@ func ValueSemanticEqualitySetElements(ctx context.Context, req ValueSemanticEqua // Ensure new value always contains all of proposed new value newValueElements[idx] = proposedNewValueElement - if idx >= originalPriorElementsLength { - continue - } - // Loop through all prior value elements and see if there are any semantically equal elements for pIdx, priorValueElement := range priorValueElements { elementReq := ValueSemanticEqualityRequest{