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

Jump to bottom

Disable access policy rewrite when compiling constraints #4248

Merged
merged 3 commits into from Aug 31, 2022
Merged

Disable access policy rewrite when compiling constraints #4248

merged 3 commits into from Aug 31, 2022

Conversation

elprans
Copy link
Member

@elprans elprans commented Aug 10, 2022

Access policies are applied before the constraint is checked anyway
and performing rewrites confuses constraint IR analysis. This was
partially fixed in #3892, but not all the way.

Fixes: #4245

@elprans elprans requested a review from msullivan August 10, 2022 00:31
@elprans elprans force-pushed the fix-constraints-and-access-policy branch from 7b52e65 to b5a4dc7 Compare August 10, 2022 00:44
msullivan
msullivan approved these changes Aug 10, 2022
View reviewed changes
Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

This looks good. The error, test_edgeql_migration_eq_collections_25, is somewhat concerning but seems to clearly be an existing flake?

@msullivan
Copy link
Member

Something bizarre is happening. The failure seems to reproduce in CI; I haven't reproed it locally, and I can't understand a change in pgsql could cause a stack overflow during earlier stages of migrations.

@elprans
Copy link
Member Author

elprans commented Aug 10, 2022 •
edited

Looks like a bug to me, unrelated (but somehow uncovered by) the change

@msullivan msullivan force-pushed the fix-constraints-and-access-policy branch from b5a4dc7 to c623512 Compare August 10, 2022 23:57
@elprans elprans force-pushed the fix-constraints-and-access-policy branch 2 times, most recently from f4b95e4 to b28d5fe Compare August 16, 2022 00:30
msullivan and others added 2 commits August 30, 2022 15:28
@msullivan
Remove a lot of tests from Nonisolated
f1be143
@elprans @msullivan
Disable access policy rewrite when compiling constraints
4711df7 
Access policies are applied _before_ the constraint is checked anyway
and performing rewrites confuses constraint IR analysis.  This was
partially fixed in #3892, but not all the way.

Fixes: #4245
@msullivan msullivan force-pushed the fix-constraints-and-access-policy branch from b28d5fe to 4711df7 Compare August 30, 2022 22:28
@msullivan msullivan merged commit 83719c6 into master Aug 31, 2022
@msullivan msullivan deleted the fix-constraints-and-access-policy branch August 31, 2022 00:02
@elprans elprans mentioned this pull request Sep 12, 2022
Closed
elprans added a commit that referenced this pull request Sep 15, 2022
@elprans
Disable access policy rewrite when compiling constraints (#4248)
aa7ebcf 
Access policies are applied _before_ the constraint is checked anyway
and performing rewrites confuses constraint IR analysis.  This was
partially fixed in #3892, but not all the way.

Fixes: #4245
@msullivan msullivan mentioned this pull request Sep 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msullivan msullivan msullivan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrations fail for exclusive constraints on links with access policies
2 participants
@elprans @msullivan