Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

types: Sort any elements during construction #3805

Merged
merged 3 commits into from
Sep 20, 2021
Merged

types: Sort any elements during construction #3805

merged 3 commits into from
Sep 20, 2021

Conversation

tsandall
Copy link
Member

This changes updates the implementation of the any type to sort
elements during construction. This way the Compare() function does not
have to sort elements before recursing (which can result in data races
if global type instances from the built-in function declarations or
elsewhere are compared.)

Fixes #3793

Signed-off-by: Torin Sandall [email protected]

srenatus
srenatus previously approved these changes Sep 18, 2021
@srenatus
Copy link
Contributor

Something in CI got confused here, it seems. Could be related to my recent change, or have something to do with the branch not being in a fork ...? I'll have a look later

@srenatus
Copy link
Contributor

srenatus commented Sep 18, 2021

Looks like the test failures are genuine -- the ordering of the types has changed. Could you have another look, please, @tsandall?

As a side note, we should add a git diff here, this output isn't useful: https://github.com/open-policy-agent/opa/pull/3805/checks?check_run_id=3639675427#step:3:145

tsandall and others added 3 commits September 20, 2021 11:23
This changes updates the implementation of the any type to sort
elements during construction. This way the Compare() function does not
have to sort elements before recursing (which can result in data races
if global type instances from the built-in function declarations or
elsewhere are compared.)

Fixes #3793

Signed-off-by: Torin Sandall <[email protected]>
@srenatus
Copy link
Contributor

@tsandall I've picked this up -- by chance it was pushed to this origin, so I've taken care of the tests and rebased it.

@srenatus srenatus merged commit f4d9c22 into main Sep 20, 2021
@srenatus srenatus deleted the fix-3793 branch September 20, 2021 09:46
dolevf pushed a commit to dolevf/opa that referenced this pull request Nov 4, 2021
* types: Sort any elements during construction

This changes updates the implementation of the any type to sort
elements during construction. This way the Compare() function does not
have to sort elements before recursing (which can result in data races
if global type instances from the built-in function declarations or
elsewhere are compared.)

Fixes open-policy-agent#3793

* capabilities.json: changed ordering
* internal/presentation: fix json error output

Co-authored-by: Torin Sandall <[email protected]>
Co-authored-by: Stephan Renatus <[email protected]>
Signed-off-by: Torin Sandall <[email protected]>
Signed-off-by: Dolev Farhi <[email protected]>
@philipaconrad philipaconrad mentioned this pull request Jul 1, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

concurrency issues in types.Compare()
2 participants