Repository: geldata/gel
Pull request: Cast uuid to object
Closes #4435.

I suggest that the two commits are reviewed separately - first one is just refactoring.

Problem statement:
Support casting from `uuid` to object types
We want to support doing things like `<User><uuid>$0` (and probably just `<User>$0`, with the same meaning?), and have it mean something like
`assert_exists((select detached User filter .id = <uuid>$0))`
(probably with a better error message passed to assert_exists).

This will make it a lot easier to convert ids into objects.

Offline we've discussed a bunch of alternative syntaxes and decided that just supporting casting was the least bad. If anyone wants I guess we can argue more here now.

Hints from issue discussion:
> and probably just `<User>$0`, with the same meaning?

Yes, we want this ^

Commit message: add a test

Patch to review:
diff --git a/edb/edgeql/compiler/casts.py b/edb/edgeql/compiler/casts.py
index 39bfb717e04..39c0dd515d3 100644
--- a/edb/edgeql/compiler/casts.py
+++ b/edb/edgeql/compiler/casts.py
@@ -35,6 +35,7 @@
 from edb.schema import functions as s_func
 from edb.schema import name as sn
 from edb.schema import types as s_types
+from edb.schema import utils as s_utils
 
 from edb.edgeql import ast as qlast
 from edb.edgeql import qltypes as ft
@@ -69,7 +70,7 @@ def compile_cast(
             ctx=ctx,
             srcctx=ir_expr.context)
 
-    elif irutils.is_untyped_empty_array_expr(ir_expr):
+    if irutils.is_untyped_empty_array_expr(ir_expr):
         # Ditto for empty arrays.
         new_typeref = typegen.type_to_typeref(new_stype, ctx.env)
         return setgen.ensure_set(
@@ -81,7 +82,7 @@ def compile_cast(
     if (orig_stype == new_stype and
             cardinality_mod is not qlast.CardinalityModifier.Required):
         return ir_set
-    elif orig_stype.is_object_type() and new_stype.is_object_type():
+    if orig_stype.is_object_type() and new_stype.is_object_type():
         # Object types cannot be cast between themselves,
         # as cast is a _constructor_ operation, and the only
         # valid way to construct an object is to INSERT it.
@@ -92,6 +93,13 @@ def compile_cast(
             f'`...[IS {new_stype.get_displayname(ctx.env.schema)}]` instead',
             context=srcctx)
 
+    uuid_t = ctx.env.get_track_schema_type(sn.QualName('std', 'uuid'))
+    if (
+        orig_stype.issubclass(ctx.env.schema, uuid_t)
+        and new_stype.is_object_type()
+    ):
+        return _find_object_by_id(ir_expr, new_stype, ctx=ctx)
+
     json_t = ctx.env.get_track_schema_type(
         sn.QualName('std', 'json'))
 
@@ -99,27 +107,23 @@ def compile_cast(
         return _cast_array_literal(
             ir_set, orig_stype, new_stype, srcctx=srcctx, ctx=ctx)
 
-    elif orig_stype.is_tuple(ctx.env.schema):
+    if orig_stype.is_tuple(ctx.env.schema):
         return _cast_tuple(
             ir_set, orig_stype, new_stype, srcctx=srcctx, ctx=ctx)
 
-    elif (
-        orig_stype.is_array()
-        and not s_types.is_type_compatible(
-            orig_stype, new_stype, schema=ctx.env.schema)
+    if orig_stype.is_array() and not s_types.is_type_compatible(
+        orig_stype, new_stype, schema=ctx.env.schema
     ):
         return _cast_array(
             ir_set, orig_stype, new_stype, srcctx=srcctx, ctx=ctx)
 
-    elif (
-        orig_stype.is_range()
-        and not s_types.is_type_compatible(
-            orig_stype, new_stype, schema=ctx.env.schema)
+    if orig_stype.is_range() and not s_types.is_type_compatible(
+        orig_stype, new_stype, schema=ctx.env.schema
     ):
         return _cast_range(
             ir_set, orig_stype, new_stype, srcctx=srcctx, ctx=ctx)
 
-    elif orig_stype.issubclass(ctx.env.schema, new_stype):
+    if orig_stype.issubclass(ctx.env.schema, new_stype):
         # The new type is a supertype of the old type,
         # and is always a wider domain, so we simply reassign
         # the stype.
@@ -127,61 +131,96 @@ def compile_cast(
             ir_set, orig_stype, new_stype,
             cardinality_mod=cardinality_mod, ctx=ctx)
 
-    elif new_stype.issubclass(ctx.env.schema, orig_stype):
+    if new_stype.issubclass(ctx.env.schema, orig_stype):
         # The new type is a subtype, so may potentially have
         # a more restrictive domain, generate a cast call.
         return _inheritance_cast_to_ir(
             ir_set, orig_stype, new_stype,
             cardinality_mod=cardinality_mod, ctx=ctx)
 
+    if (
+        new_stype.issubclass(ctx.env.schema, json_t)
+        and ir_set.path_id.is_objtype_path()
+    ):
+        # JSON casts of objects are special: we want the full shape
+        # and not just an identity.
+        with ctx.new() as subctx:
+            subctx.implicit_id_in_shapes = False
+            subctx.implicit_tid_in_shapes = False
+            subctx.implicit_tname_in_shapes = False
+            viewgen.late_compile_view_shapes(ir_set, ctx=subctx)
     else:
-        if (new_stype.issubclass(ctx.env.schema, json_t) and
-                ir_set.path_id.is_objtype_path()):
-            # JSON casts of objects are special: we want the full shape
-            # and not just an identity.
-            with ctx.new() as subctx:
-                subctx.implicit_id_in_shapes = False
-                subctx.implicit_tid_in_shapes = False
-                subctx.implicit_tname_in_shapes = False
-                viewgen.late_compile_view_shapes(ir_set, ctx=subctx)
-        elif (orig_stype.issubclass(ctx.env.schema, json_t)
-              and new_stype.is_enum(ctx.env.schema)):
+        if orig_stype.issubclass(ctx.env.schema, json_t) and new_stype.is_enum(
+            ctx.env.schema
+        ):
             # Casts from json to enums need some special handling
             # here, where we have access to the enum type. Just turn
             # it into json->str and str->enum.
-            str_typ = ctx.env.get_track_schema_type(
-                sn.QualName('std', 'str'))
+            str_typ = ctx.env.get_track_schema_type(sn.QualName('std', 'str'))
             str_ir = compile_cast(ir_expr, str_typ, srcctx=srcctx, ctx=ctx)
-            return compile_cast(str_ir, new_stype,
-                                cardinality_mod=cardinality_mod,
-                                srcctx=srcctx, ctx=ctx)
-        elif (orig_stype.issubclass(ctx.env.schema, json_t)
-              and isinstance(new_stype, s_types.Array)
-              and not new_stype.get_subtypes(ctx.env.schema)[0].issubclass(
-                  ctx.env.schema, json_t)):
+            return compile_cast(
+                str_ir,
+                new_stype,
+                cardinality_mod=cardinality_mod,
+                srcctx=srcctx,
+                ctx=ctx,
+            )
+
+        if (
+            orig_stype.issubclass(ctx.env.schema, json_t)
+            and isinstance(new_stype, s_types.Array)
+            and not new_stype.get_subtypes(ctx.env.schema)[0].issubclass(
+                ctx.env.schema, json_t
+            )
+        ):
             # Turn casts from json->array<T> into json->array<json>
             # and array<json>->array<T>.
             ctx.env.schema, json_array_typ = s_types.Array.from_subtypes(
-                ctx.env.schema, [json_t])
+                ctx.env.schema, [json_t]
+            )
             json_array_ir = compile_cast(
-                ir_expr, json_array_typ, cardinality_mod=cardinality_mod,
-                srcctx=srcctx, ctx=ctx)
+                ir_expr,
+                json_array_typ,
+                cardinality_mod=cardinality_mod,
+                srcctx=srcctx,
+                ctx=ctx,
+            )
             return compile_cast(
-                json_array_ir, new_stype, srcctx=srcctx, ctx=ctx)
-        elif (orig_stype.issubclass(ctx.env.schema, json_t)
-              and isinstance(new_stype, s_types.Tuple)):
+                json_array_ir, new_stype, srcctx=srcctx, ctx=ctx
+            )
+
+        if orig_stype.issubclass(ctx.env.schema, json_t) and isinstance(
+            new_stype, s_types.Tuple
+        ):
             return _cast_json_to_tuple(
-                ir_set, orig_stype, new_stype, cardinality_mod,
-                srcctx=srcctx, ctx=ctx)
-        elif (orig_stype.issubclass(ctx.env.schema, json_t)
-              and isinstance(new_stype, s_types.Range)):
+                ir_set,
+                orig_stype,
+                new_stype,
+                cardinality_mod,
+                srcctx=srcctx,
+                ctx=ctx,
+            )
+
+        if orig_stype.issubclass(ctx.env.schema, json_t) and isinstance(
+            new_stype, s_types.Range
+        ):
             return _cast_json_to_range(
-                ir_set, orig_stype, new_stype, cardinality_mod,
-                srcctx=srcctx, ctx=ctx)
+                ir_set,
+                orig_stype,
+                new_stype,
+                cardinality_mod,
+                srcctx=srcctx,
+                ctx=ctx,
+            )
 
-        return _compile_cast(
-            ir_expr, orig_stype, new_stype, cardinality_mod=cardinality_mod,
-            srcctx=srcctx, ctx=ctx)
+    return _compile_cast(
+        ir_expr,
+        orig_stype,
+        new_stype,
+        cardinality_mod=cardinality_mod,
+        srcctx=srcctx,
+        ctx=ctx,
+    )
 
 
 def _compile_cast(
@@ -888,3 +927,51 @@ def _cast_array_literal(
         )
 
     return setgen.ensure_set(cast_ir, ctx=ctx)
+
+
+def _find_object_by_id(
+    ir_expr: Union[irast.Set, irast.Expr],
+    new_stype: s_types.Type,
+    *,
+    ctx: context.ContextLevel,
+) -> irast.Set:
+    with ctx.new() as subctx:
+        subctx.anchors = subctx.anchors.copy()
+
+        uuid_anchor = subctx.create_anchor(
+            setgen.ensure_set(ir_expr, ctx=ctx), name='a'
+        )
+
+        object_name = s_utils.name_to_ast_ref(
+            new_stype.get_name(ctx.env.schema)
+        )
+
+        select_id = qlast.SelectQuery(
+            result=qlast.DetachedExpr(expr=qlast.Path(steps=[object_name])),
+            where=qlast.BinOp(
+                left=qlast.Path(
+                    steps=[
+                        qlast.Ptr(
+                            ptr=qlast.ObjectRef(name='id'), direction='>'
+                        )
+                    ],
+                    partial=True,
+                ),
+                op='=',
+                right=uuid_anchor,
+            ),
+        )
+        ql = qlast.FunctionCall(
+            func='assert_exists',
+            args=[select_id],
+            kwargs={
+                'message': qlast.StringConstant(
+                    value=(
+                        repr(new_stype.get_displayname(ctx.env.schema))
+                        + ' with this id does not exist'
+                    )
+                )
+            },
+        )
+
+        return dispatch.compile(ql, ctx=subctx)
diff --git a/tests/schemas/casts.esdl b/tests/schemas/casts.esdl
index e1d9782d3da..f4e56c12050 100644
--- a/tests/schemas/casts.esdl
+++ b/tests/schemas/casts.esdl
@@ -76,3 +76,7 @@ type ScalarTest {
     property p_decimal -> decimal;
     property p_json -> json;
 }
+
+type Person {
+    property first_name -> str;
+}
\ No newline at end of file
diff --git a/tests/schemas/casts_setup.edgeql b/tests/schemas/casts_setup.edgeql
index b7c9691a85f..b9ea39881a3 100644
--- a/tests/schemas/casts_setup.edgeql
+++ b/tests/schemas/casts_setup.edgeql
@@ -51,3 +51,11 @@ INSERT JSONTest {
     j_bigint := <json>123456789123456789123456789n,
     j_decimal := <json>123456789123456789123456789.123456789123456789123456789n
 };
+
+INSERT Person {
+    first_name := 'tom',
+};
+
+INSERT Person {
+    first_name := 'kelly',
+};
\ No newline at end of file
diff --git a/tests/test_edgeql_casts.py b/tests/test_edgeql_casts.py
index d59ee0b1228..f256f5871a9 100644
--- a/tests/test_edgeql_casts.py
+++ b/tests/test_edgeql_casts.py
@@ -2631,3 +2631,24 @@ async def test_edgeql_casts_all_null(self):
                 msg=f'{cast.from_type.name} to {cast.to_type.name}',
                 json_only=json_only,
             )
+
+    async def test_edgeql_casts_uuid_to_object(self):
+        persons = await self.con.query('select Person { id }')
+
+        res = await self.con.query('select <Person><uuid>$0', persons[0].id)
+        self.assertEqual(1, len(res))
+
+        async with self.assertRaisesRegexTx(
+            edgedb.CardinalityViolationError, r'with this id does not exist'
+        ):
+            await self.con.query('select <Person><uuid>$0', '1' * 32)
+
+        await self.assert_query_result(
+            '''
+            select (<Person>{<uuid>$0, <uuid>$1}) { first_name }
+            order by .first_name;
+            ''',
+            [{'first_name': 'kelly'}, {'first_name': 'tom'}],
+            json_only=True,
+            variables=(persons[0].id, persons[1].id),
+        )


Code review comments:
diff_hunk: @@ -2631,3 +2631,24 @@ async def test_edgeql_casts_all_null(self):
                 msg=f'{cast.from_type.name} to {cast.to_type.name}',
                 json_only=json_only,
             )
+
+    async def test_edgeql_casts_uuid_to_object(self):
+        persons = await self.con.query('select Person { id }')
+
+        res = await self.con.query('select <Person><uuid>$0', persons[0].id)
+        self.assertEqual(1, len(res))
+
+        async with self.assertRaisesRegexTx(
path: tests/test_edgeql_casts.py
text: @user1:
Add a test where we apply it to a set of ids where *one* of the ids is missing and the rest are there, to make sure that that raises an error.

@author:
Ha. It turns out that this does not raise an error. Did I do something wrong with contexts?

@user1:
Ok, I think we do need to make sure that the set appears in the scope tree, like this:
https://github.com/edgedb/edgedb/blob/master/edb/edgeql/compiler/casts.py#L480-L483

When you test this out, make sure to take a look at the scope tree with and without doing this, to get a feel for what is going on

@user1:
(Interestingly, I bet that if you had `with` bound the set you were casting, and then cast that *name*, it would have worked. This is because compiling that *name* would have put the path in the scope tree)

@author:
Hmmm, after I added UUID into the error message this now passes. Is this the consequence of the strange scope tree rules?

@user1:
Ha, yeah.
In the original take, the reference to the anchor appeared only in a subquery. (And like, triply so: inside of SET OF argument, inside a select statement, inside a filter clause thereof.)
But once the error message was added, it was referred to in the scope *outside* of the SET OF argument to `assert_exists`, so that is where it gets bound...

diff_hunk: @@ -76,3 +76,7 @@ type ScalarTest {
     property p_decimal -> decimal;
     property p_json -> json;
 }
+
+type Person {
+    property first_name -> str;
path: tests/schemas/casts.esdl
text: @user1:
Just call it `name`?

diff_hunk: @@ -961,6 +961,17 @@ def _find_object_by_id(
                 right=uuid_anchor,
             ),
         )
-        ql = qlast.FunctionCall(func='assert_exists', args=[select_id])
+        ql = qlast.FunctionCall(
+            func='assert_exists',
+            args=[select_id],
+            kwargs={
+                'message': qlast.StringConstant(
path: edb/edgeql/compiler/casts.py
text: @user1:
Can we make the error message here actually include the id?

@author:
Yes, but that would compute the error message at runtime, for each id, even if the function does not fail.

Do we want that, thinking from performance standpoint?

@user1:
Yeah, that's a good question. Let's try benchmarking it.

@user1:
Otherwise I would go with something like
```
could not find <typename> with specified id
```
I think that including the id would be pretty nice, though. So if it is slow, it might be worth looking into way to make it not too bad?


@author:
I've tried using this:
```
> time edb cli query "select (for i in _gen_series(1, 10000000000) union (select <Person><uuid>'b90b4f92-4331-11ed-adee-17dbc27f8b02' filter .first_name = 'non-existent'));"
edb cli query   0.52s user 0.06s system 99% cpu 0.589 total
```
and it from 0.49s - 0.55s, no matter if I have ids in error message or not.

So I'm keeping it in.