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

Fix a category of confusing scoping related bugs in access policies #4994

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

msullivan
Copy link
Member

The edgeql->IR compiler has a notion of "weak namespaces" in which
scope tree nodes are tagged with namespaces, and those namespaces will
be dropped from a path if it refers to a node bound above that node.

The pg side doesn't understand any of this, though, so we have a
_rewrite_weak_namespaces that rewrites all affected path ids to
reference their targets directly.

Unfortunately we neglect to call it on our type rewrite expressions.
Fix that.

Also refactor fini_expression to try to make this category of bug
less likely by having more things driven by a list of expressions to
process.

The edgeql->IR compiler has a notion of "weak namespaces" in which
scope tree nodes are tagged with namespaces, and those namespaces will
be dropped from a path if it refers to a node bound above that node.

The pg side doesn't understand any of this, though, so we have a
_rewrite_weak_namespaces that rewrites all affected path ids to
reference their targets directly.

Unfortunately we neglect to call it on our type rewrite expressions.
Fix that.

Also refactor `fini_expression` to try to make this category of bug
less likely by having more things driven by a list of expressions to
process.
Comment on lines +145 to +155
extra_exprs = []
extra_exprs += [
rw for rw in ctx.env.type_rewrites.values()
if isinstance(rw, irast.Set)
]
extra_exprs += [
p.sub_params.decoder_ir for p in ctx.env.query_parameters.values()
if p.sub_params and p.sub_params.decoder_ir
]

all_exprs = [ir] + extra_exprs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
extra_exprs = []
extra_exprs += [
rw for rw in ctx.env.type_rewrites.values()
if isinstance(rw, irast.Set)
]
extra_exprs += [
p.sub_params.decoder_ir for p in ctx.env.query_parameters.values()
if p.sub_params and p.sub_params.decoder_ir
]
all_exprs = [ir] + extra_exprs
all_exprs = [ir] + [
rw for rw in ctx.env.type_rewrites.values()
if isinstance(rw, irast.Set)
] + [
p.sub_params.decoder_ir for p in ctx.env.query_parameters.values()
if p.sub_params and p.sub_params.decoder_ir
]

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't get rid of extra_exprs, since we need to use that. We could build extra_exprs in one go, but I kind of like having each case more clearly specified, since it to me emphasizes that more things will be added

@msullivan msullivan merged commit dcdd94f into master Feb 8, 2023
@msullivan msullivan deleted the rewrite-namespaces branch February 8, 2023 19:44
msullivan added a commit that referenced this pull request Feb 8, 2023
More follow up from #4994

 * We didn't call `infer_group_aggregates` on the policy expressions.
 * _rewrite_weak_namespaces actually *already* used a different method
   to iterate over "all sets" (looking at the map of sets to types),
   and only used the passed in set to produce its binding map.
   Avoid redoing the main pass repeatedly.

While trying to test the `infer_group_aggregates` thing, I discovered:
 * Query normalization was busted and would copy a GROUP statement's
   `using` clause to be a `with` binding in front of the GROUP!
msullivan added a commit that referenced this pull request Feb 9, 2023
More follow up from #4994

 * We didn't call `infer_group_aggregates` on the policy expressions.
 * _rewrite_weak_namespaces actually *already* used a different method
   to iterate over "all sets" (looking at the map of sets to types),
   and only used the passed in set to produce its binding map.
   Avoid redoing the main pass repeatedly.

While trying to test the `infer_group_aggregates` thing, I discovered:
 * Query normalization was busted and would copy a GROUP statement's
   `using` clause to be a `with` binding in front of the GROUP!
msullivan added a commit that referenced this pull request Feb 9, 2023
…4994)

The edgeql->IR compiler has a notion of "weak namespaces" in which
scope tree nodes are tagged with namespaces, and those namespaces will
be dropped from a path if it refers to a node bound above that node.

The pg side doesn't understand any of this, though, so we have a
_rewrite_weak_namespaces that rewrites all affected path ids to
reference their targets directly.

Unfortunately we neglect to call it on our type rewrite expressions.
Fix that.

Also refactor `fini_expression` to try to make this category of bug
less likely by having more things driven by a list of expressions to
process.
msullivan added a commit that referenced this pull request Feb 9, 2023
More follow up from #4994

 * We didn't call `infer_group_aggregates` on the policy expressions.
 * _rewrite_weak_namespaces actually *already* used a different method
   to iterate over "all sets" (looking at the map of sets to types),
   and only used the passed in set to produce its binding map.
   Avoid redoing the main pass repeatedly.

While trying to test the `infer_group_aggregates` thing, I discovered:
 * Query normalization was busted and would copy a GROUP statement's
   `using` clause to be a `with` binding in front of the GROUP!
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.

2 participants