Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

internal/fwschemadata: Rewrite SetValue semantic equality logic to ignore order #1064

Merged
merged 10 commits into from
Feb 12, 2025

Conversation

austinvalle
Copy link
Member

@austinvalle austinvalle commented Dec 6, 2024

Closes #1061

This PR fixes the set semantic equality logic, which prior to this change, was written equivalent to list semantic equality.

Sets are unordered, so to correctly compare the semantic equality of two sets, we must compare each element in the set against all other elements to determine if there is a semantically equivalent element in the set. Once we find a semantically equal element, we remove it from the slice of candidates and continue.


As an example, consider a set of case insensitive strings.

With the previous logic, the following sets would be considered semantically equal:

["value-a", "value-b", "value-c"]
["VALUE-A", "VALUE-B", "VALUE-C"]

However the following set would be incorrectly considered semantically not equal, due to the difference in order:

["value-a", "value-b", "value-c"]
["VALUE-B", "VALUE-C", "VALUE-A"]

This change will result in both of these examples being considered semantically equal.

Corner tests

I also wrote some acceptance tests to make it a little easier to understand the impact (using IPv6 address strings as an example)

hashicorp/terraform-provider-corner#297

Unit test failures before the fix

https://github.com/hashicorp/terraform-plugin-framework/actions/runs/12832297072/job/35784696031?pr=1064

--- FAIL: TestValueSemanticEqualitySet (0.00s)
    --- FAIL: TestValueSemanticEqualitySet/SetValue-SetValue-StringValuableWithSemanticEquals-true-diff-order (0.00s)
        value_semantic_equality_set_test.go:1021: unexpected difference:   &fwschemadata.ValueSemanticEqualityResponse{
            - 	NewValue:    s`[["KEEP-LOWERCASE-012","KEEP-LOWERCASE-789"],["KEEP-LOWERCASE-456","KEEP-LOWERCASE-123"]]`,
            + 	NewValue:    s`[["keep-lowercase-123","keep-lowercase-456"],["keep-lowercase-789","keep-lowercase-012"]]`,
              	Diagnostics: nil,
              }
    --- FAIL: TestValueSemanticEqualitySet/SetValue-StringValuableWithSemanticEquals-true-diff-order (0.00s)
        value_semantic_equality_set_test.go:10[21](https://github.com/hashicorp/terraform-plugin-framework/actions/runs/12832297072/job/35784696031?pr=1064#step:5:22): unexpected difference:   &fwschemadata.ValueSemanticEqualityResponse{
            - 	NewValue:    s`["KEEP-LOWERCASE-456","KEEP-LOWERCASE-123"]`,
            + 	NewValue:    s`["keep-lowercase-123","keep-lowercase-456"]`,
              	Diagnostics: nil,
              }

@austinvalle austinvalle added the bug Something isn't working label Dec 6, 2024
@austinvalle austinvalle marked this pull request as ready for review January 17, 2025 16:01
@austinvalle austinvalle requested a review from a team as a code owner January 17, 2025 16:01
@austinvalle austinvalle added this to the v1.14.1 milestone Jan 17, 2025
@austinvalle
Copy link
Member Author

austinvalle commented Jan 17, 2025

̶S̶i̶n̶c̶e̶ ̶w̶e̶ ̶h̶a̶v̶e̶ ̶w̶r̶i̶t̶e̶-̶o̶n̶l̶y̶ ̶a̶t̶t̶r̶i̶b̶u̶t̶e̶s̶ ̶l̶a̶n̶d̶i̶n̶g̶ ̶s̶o̶o̶n̶,̶ ̶t̶h̶i̶s̶ ̶b̶u̶g̶ ̶f̶i̶x̶ ̶w̶i̶l̶l̶ ̶g̶o̶ ̶i̶n̶ ̶a̶f̶t̶e̶r̶ ̶i̶n̶ ̶̶v̶1̶.̶1̶4̶.̶1̶̶

Now planning on going with write-only attributes in v1.14.0

@bbasata bbasata self-requested a review February 6, 2025 20:03
@bbasata
Copy link

bbasata commented Feb 10, 2025

I read line-by-line through internal/fwschemadata/value_semantic_equality_set.go a couple times until it tangibly clicked with me what semantic equality is about -- that was my first challenge. "Why does equality modify values?" Oh, ok! Now I want to call it "prior state prevails when the difference is cosmetic" 😃

With that, I think I have unlocked the power-up needed to review the PR 😃

@bbasata
Copy link

bbasata commented Feb 10, 2025

Once we find a semantically equal element, we remove it from the slice of candidates and continue.

Non-blocking for merge, simply a consideration: is this for optimization or correctness? I'm guessing optimization -- I think the behavior is still correct without doing this.

Related: A set might contain the same value more than once. Or multiple semantically-equal values. And I imagine it's a Hard (and unnecessary) Problem to make any promise about the behavior in that case. If the behavior mattered in that scenario, we'd likely advise folks to use a List.

@austinvalle austinvalle force-pushed the av/set-semantic-equality branch from e28c810 to fef2d6a Compare February 10, 2025 19:58
bbasata
bbasata previously approved these changes Feb 10, 2025
Copy link

@bbasata bbasata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@austinvalle austinvalle requested a review from bbasata February 12, 2025 15:19
@austinvalle austinvalle modified the milestones: v1.14.1, v1.14.0 Feb 12, 2025
Copy link

@bbasata bbasata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

I see the CI / lint Action wants something from us -- I'm happy to stamp this again if/when needed.

@austinvalle
Copy link
Member Author

Will create a new PR for lint deprecations since it's gonna be noisy and unrelated to these changes 👍🏻

@austinvalle austinvalle merged commit bf1d023 into main Feb 12, 2025
33 of 34 checks passed
@austinvalle austinvalle deleted the av/set-semantic-equality branch February 12, 2025 15:29
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Semantic Equal on Sets expects identical order of elements
2 participants