Skip to content

Commit

Permalink
Fix two issues directly reading pointers from a group (#7130)
Browse files Browse the repository at this point in the history
The first is that `select (group Card by .element).key.element`
produces an ISE. The issue here is that the path_id was registered at
the *use site* already in the path_scope. We already suppress the
path_scope for these things when compiling the group itself, and we
need to do it when compiling the result.
(Another scope tree related bug!)

The second is that in some cases, projecting a multi property out of a
shape, the the array that the multi property is serialized into could
escape and be returned from the query as the final result, which can
lead to an array being interpreted as a string, for example.

I actually fixed this in two ways:
 * First, shape compilation preents the shape elements from leaking out.
 * Second, we set `expr_exposed = False` when compiling the LHS
   of pointer dereferences,so we skip pointlessly compiling the
   shape also.

Fixes #5651. Fixes #6557.
  • Loading branch information
msullivan authored Mar 28, 2024
1 parent d67682a commit 0fa82ae
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 6 deletions.
3 changes: 3 additions & 0 deletions edb/pgsql/compiler/expr.py
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,9 @@ def _compile_shape(
name=el.name,
val=ser_val
))
# Don't let the elements themselves leak out, since they may
# be wrapped in arrays.
pathctx.put_path_id_mask(ctx.rel, el.path_id)

ser_result = pgast.TupleVar(elements=ser_elements, named=True)
sval = output.serialize_expr(
Expand Down
7 changes: 7 additions & 0 deletions edb/pgsql/compiler/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,8 @@ def _compile_group(
# Now we compile the bindings
groupctx.path_scope = subjctx.path_scope.new_child()
groupctx.path_scope[stmt.group_binding.path_id] = None
if stmt.grouping_binding:
groupctx.path_scope[stmt.grouping_binding.path_id] = None

# Compile all the 'using' items
for _alias, (value, using_card) in stmt.using.items():
Expand Down Expand Up @@ -359,6 +361,9 @@ def _get_volatility_ref() -> Optional[pgast.BaseExpr]:
return vol_ref

with ctx.new() as outctx:
# Inherit the path_scope we made earlier (with the GROUP bindings
# removed), so that we'll always look for those in the right place.
outctx.path_scope = groupctx.path_scope

outctx.volatility_ref += (lambda stmt, xctx: _get_volatility_ref(),)

Expand All @@ -368,6 +373,8 @@ def _get_volatility_ref() -> Optional[pgast.BaseExpr]:
clauses.compile_output(stmt.result, ctx=outctx)

with ctx.new() as ictx:
ictx.path_scope = groupctx.path_scope

# FILTER and ORDER BY need to have the base result as a
# volatility ref.
clauses.setup_iterator_volatility(stmt.result, ctx=ictx)
Expand Down
16 changes: 10 additions & 6 deletions edb/pgsql/compiler/relgen.py
Original file line number Diff line number Diff line change
Expand Up @@ -1132,6 +1132,8 @@ def process_set_as_path(

elif not source_is_visible:
with ctx.subrel() as srcctx:
srcctx.expr_exposed = False

get_set_rvar(ir_source, ctx=srcctx)

if is_inline_primitive_ref:
Expand Down Expand Up @@ -1341,12 +1343,14 @@ def process_set_as_subquery(
if source_is_visible and (
ir_source.path_id not in ctx.skippable_sources
):
source_set_rvar = get_set_rvar(ir_source, ctx=ctx)
# Force a source rvar so that trivial computed pointers
# on erroneous objects (like a bad array deref) fail.
# (Most sensible computables will end up requiring the
# source rvar anyway.)
ensure_source_rvar(ir_source, stmt, ctx=ctx)
with ctx.new() as sctx:
sctx.expr_exposed = False
source_set_rvar = get_set_rvar(ir_source, ctx=sctx)
# Force a source rvar so that trivial computed pointers
# on erroneous objects (like a bad array deref) fail.
# (Most sensible computables will end up requiring the
# source rvar anyway.)
ensure_source_rvar(ir_source, stmt, ctx=sctx)
else:
ir_source = None
source_is_visible = False
Expand Down
18 changes: 18 additions & 0 deletions tests/test_edgeql_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -1381,3 +1381,21 @@ async def test_edgeql_group_backlink(self):
''',
[{"a": {}}, {"a": {}}],
)

async def test_edgeql_group_destruct_immediately_01(self):
await self.assert_query_result(
r'''
WITH MODULE cards
select (group Card by .element).key.element
''',
{"Fire", "Earth", "Water", "Air"},
)

async def test_edgeql_group_destruct_immediately_02(self):
await self.assert_query_result(
r'''
WITH MODULE cards
select (group Card by .element).grouping
''',
["element", "element", "element", "element"],
)
8 changes: 8 additions & 0 deletions tests/test_edgeql_select.py
Original file line number Diff line number Diff line change
Expand Up @@ -5930,6 +5930,14 @@ async def test_edgeql_select_bigint_index_03(self):
['[]'],
)

async def test_edgeql_select_multi_property_shape_01(self):
await self.assert_query_result(
r"""
select (BooleanTest { tags }).tags
""",
tb.bag(['red', 'black', 'red', 'green', 'red']),
)

async def test_edgeql_select_tuple_01(self):
await self.assert_query_result(
r"""
Expand Down

0 comments on commit 0fa82ae

Please sign in to comment.