Repository: geldata/gel
Pull request: Better error messages on schema type mismatches
Make `Schema.get` produce its own error on a type mismatch instead of
using `get_by_id`, since it has access to the name and so can produce
a better message. Also pass in the sourcectx when resolving names as
part of shell construction, so that the errors will have source
contexts.

In `declarative`, replace some type asserts with type checks and error messages, 
and add some infrastructure for producing the error messages.

Fixes #1016.

Problem statement:
Bad error reporting on create type
```
CREATE TYPE MyStr EXTENDING str;                                     
ERROR: InternalServerError: 'NoneType' object has no attribute 'is_schema_field'
```
Version `1.0-alpha.2+dev.391.g2276144b`

I obviously needed `CREATE SCALAR TYPE` and would expect it to be grammar error (i.e. something like `TYPE is unexpected, expected one of SCALAR, DATABASE, ...`

Hints from issue discussion:
Paul, do you think you can look at this one yourself? This needs a relatively simple & quick fix in `edb/schema/` package.
Yes, I can take a look.
> would expect it to be grammar error

`CREATE TYPE` is valid syntax and is used to define object types. The crash in this case is caused by the fact that you're trying to create an object type that inherits from a scalar type.  There has to be a check to prevent mixing schema object classes like that. [Here](https://github.com/edgedb/edgedb/blob/36e86d56cff905b66e865dfb64b0ed3b421ca2ed/edb/schema/inheriting.py#L401) and [here](https://github.com/edgedb/edgedb/blob/36e86d56cff905b66e865dfb64b0ed3b421ca2ed/edb/schema/inheriting.py#L623) are good spots to place the checks in (check `bases`).
@tailhook Please take a look at this one.
Well I've managed to fix exact this error in #1425. I couldn't follow @elprans's hint as execution flow doesn't reach that points.

Wanted to fix similar errors on `ALTER`. But that takes too much time at this moment...
It looks like we produce an error message now (instead of an ISE), but not a good one:
`ERROR: InvalidReferenceError: schema object UUID('00000000-0000-0000-0000-000000000101') exists, but is a 'scalar type' not a 'object type'`

We can do better

Commit message: Handle it in declarative also

Fixes #2556.

Patch to review:
diff --git a/edb/edgeql/declarative.py b/edb/edgeql/declarative.py
index c70b7af91f1..cd0530517ae 100644
--- a/edb/edgeql/declarative.py
+++ b/edb/edgeql/declarative.py
@@ -50,8 +50,10 @@
 from edb.schema import links as s_links
 from edb.schema import name as s_name
 from edb.schema import objects as s_obj
+from edb.schema import objtypes as s_objtypes
 from edb.schema import properties as s_props
 from edb.schema import pseudo as s_pseudo
+from edb.schema import scalars as s_scalars
 from edb.schema import schema as s_schema
 from edb.schema import sources as s_sources
 from edb.schema import types as s_types
@@ -1200,6 +1202,39 @@ def _resolve_type_expr(
         )
 
 
+TRACER_TO_REAL_TYPE_MAP = {
+    qltracer.ObjectType: s_objtypes.ObjectType,
+    qltracer.ScalarType: s_scalars.ScalarType,
+    qltracer.Constraint: s_constr.Constraint,
+    qltracer.Annotation: s_anno.Annotation,
+    qltracer.Property: s_props.Property,
+    qltracer.Link: s_links.Link,
+}
+
+
+def _get_local_obj(
+    refname: s_name.QualName,
+    tracer_type: Type[qltracer.NamedObject],
+    sourcectx: Optional[parsing.ParserContext],
+    *,
+    ctx: LayoutTraceContext,
+) -> Optional[qltracer.NamedObject]:
+
+    obj = ctx.objects.get(refname)
+
+    if obj is not None and not isinstance(obj, tracer_type):
+        obj_type = TRACER_TO_REAL_TYPE_MAP[type(obj)]
+        real_type = TRACER_TO_REAL_TYPE_MAP[tracer_type]
+        raise errors.InvalidReferenceError(
+            f'{str(refname)!r} exists, but is a '
+            f'{obj_type.get_schema_class_displayname()!r}, '
+            f'not a {real_type.get_schema_class_displayname()!r}',
+            context=sourcectx,
+        )
+
+    return obj
+
+
 def _resolve_type_name(
     ref: qlast.BaseObjectRef,
     *,
@@ -1209,10 +1244,9 @@ def _resolve_type_name(
 ) -> qltracer.ObjectLike:
 
     refname = ctx.get_ref_name(ref)
-    local_obj = ctx.objects.get(refname)
+    local_obj = _get_local_obj(refname, tracer_type, ref.context, ctx=ctx)
     obj: qltracer.ObjectLike
     if local_obj is not None:
-        assert isinstance(local_obj, tracer_type)
         obj = local_obj
     else:
         obj = _resolve_schema_ref(
@@ -1235,26 +1269,23 @@ def _get_tracer_and_real_type(
 
     if isinstance(decl, qlast.CreateObjectType):
         tracer_type = qltracer.ObjectType
-        real_type = s_types.Type
     elif isinstance(decl, qlast.CreateScalarType):
         tracer_type = qltracer.ScalarType
-        real_type = s_types.Type
     elif isinstance(decl, (qlast.CreateConstraint,
                            qlast.CreateConcreteConstraint)):
         tracer_type = qltracer.Constraint
-        real_type = s_constr.Constraint
     elif isinstance(decl, (qlast.CreateAnnotation,
                            qlast.CreateAnnotationValue)):
         tracer_type = qltracer.Annotation
-        real_type = s_anno.Annotation
     elif isinstance(decl, (qlast.CreateProperty,
                            qlast.CreateConcreteProperty)):
         tracer_type = qltracer.Property
-        real_type = s_props.Property
     elif isinstance(decl, (qlast.CreateLink,
                            qlast.CreateConcreteLink)):
         tracer_type = qltracer.Link
-        real_type = s_links.Link
+
+    if tracer_type:
+        real_type = TRACER_TO_REAL_TYPE_MAP[tracer_type]
 
     return tracer_type, real_type
 
@@ -1270,11 +1301,9 @@ def _validate_schema_ref(
         # Bail out and rely on some other validation mechanism
         return
 
-    local_obj = ctx.objects.get(refname)
+    local_obj = _get_local_obj(refname, tracer_type, decl.context, ctx=ctx)
 
-    if local_obj is not None:
-        assert isinstance(local_obj, tracer_type)
-    else:
+    if local_obj is None:
         assert real_type is not None
         _resolve_schema_ref(
             refname,
diff --git a/edb/schema/schema.py b/edb/schema/schema.py
index 5d5d5d895e8..1729c51f536 100644
--- a/edb/schema/schema.py
+++ b/edb/schema/schema.py
@@ -1248,7 +1248,7 @@ def _get_by_id(  # NoQA: F811
             if type is not None and not isinstance(obj, type):
                 raise errors.InvalidReferenceError(
                     f'schema object {obj_id!r} exists, but is a '
-                    f'{obj.__class__.get_schema_class_displayname()!r} '
+                    f'{obj.__class__.get_schema_class_displayname()!r}, '
                     f'not a {type.get_schema_class_displayname()!r}'
                 )
 
@@ -1287,7 +1287,7 @@ def getter(schema: FlatSchema, name: sn.Name) -> Optional[so.Object]:
             if obj_id is None:
                 return None
 
-            obj = schema.get_by_id(obj_id, type=type, default=None)
+            obj = schema.get_by_id(obj_id, default=None)
             if obj is not None and condition is not None:
                 if not condition(obj):
                     obj = None
@@ -1301,6 +1301,17 @@ def getter(schema: FlatSchema, name: sn.Name) -> Optional[so.Object]:
         )
 
         if obj is not so.NoDefault:
+            # We do our own type check, instead of using get_by_id's, so
+            # we can produce a user-facing error message.
+            if obj and type is not None and not isinstance(obj, type):
+                refname = str(name)
+                raise errors.InvalidReferenceError(
+                    f'{refname!r} exists, but is a '
+                    f'{obj.__class__.get_schema_class_displayname()!r}, '
+                    f'not a {type.get_schema_class_displayname()!r}',
+                    context=sourcectx,
+                )
+
             return obj  # type: ignore
         else:
             self._raise_bad_reference(
diff --git a/edb/schema/utils.py b/edb/schema/utils.py
index e98332cfdfe..d1392e3a5a2 100644
--- a/edb/schema/utils.py
+++ b/edb/schema/utils.py
@@ -36,6 +36,7 @@
     from . import objtypes as s_objtypes
     from . import schema as s_schema
     from . import types as s_types
+    from edb.common import parsing
 
     T = TypeVar('T')
 
@@ -73,6 +74,7 @@ def resolve_name(
     lname: sn.Name,
     *,
     metaclass: Optional[Type[so.Object]] = None,
+    sourcectx: Optional[parsing.ParserContext] = None,
     modaliases: Mapping[Optional[str], str],
     schema: s_schema.Schema,
 ) -> sn.Name:
@@ -81,6 +83,7 @@ def resolve_name(
         type=metaclass,
         module_aliases=modaliases,
         default=None,
+        sourcectx=sourcectx,
     )
     if obj is not None:
         name = obj.get_name(schema)
@@ -116,6 +119,7 @@ def ast_objref_to_object_shell(
         metaclass=metaclass,
         modaliases=modaliases,
         schema=schema,
+        sourcectx=ref.context,
     )
 
     return so.ObjectShell(
@@ -146,6 +150,7 @@ def ast_objref_to_type_shell(
         metaclass=mcls,
         modaliases=modaliases,
         schema=schema,
+        sourcectx=ref.context,
     )
 
     return s_types.TypeShell(
diff --git a/tests/test_edgeql_ddl.py b/tests/test_edgeql_ddl.py
index a1a92911a97..be0c504c97a 100644
--- a/tests/test_edgeql_ddl.py
+++ b/tests/test_edgeql_ddl.py
@@ -11511,6 +11511,16 @@ async def test_edgeql_ddl_alter_union_03(self):
                 [],
             )
 
+    async def test_edgeql_ddl_extending_scalar_wrongly(self):
+        async with self.assertRaisesRegexTx(
+            edgedb.QueryError,
+            "'str' exists, but is a 'scalar type', not a 'object type'",
+            _line=1, _col=29
+        ):
+            await self.con.execute(
+                r'''CREATE TYPE MyStr EXTENDING str;''',
+            )
+
 
 class TestConsecutiveMigrations(tb.DDLTestCase):
     TRANSACTION_ISOLATION = False
diff --git a/tests/test_schema.py b/tests/test_schema.py
index 389fbd8e245..28e94448d9d 100644
--- a/tests/test_schema.py
+++ b/tests/test_schema.py
@@ -606,6 +606,26 @@ def test_schema_target_consistency_check_03(self):
             }
         """
 
+    @tb.must_fail(
+        errors.InvalidReferenceError,
+        "'test::X' exists, but is a 'scalar type', not a 'object type'",
+        line=3, col=30)
+    def test_schema_wrong_type_01(self):
+        """
+            scalar type X extending enum<TEST>;
+            type Y extending X {}
+        """
+
+    @tb.must_fail(
+        errors.InvalidReferenceError,
+        "'test::X' exists, but is a 'object type', not a 'constraint'",
+        line=3, col=22)
+    def test_schema_wrong_type_02(self):
+        """
+            type X;
+            type Y { constraint X; }
+        """
+
     def test_schema_refs_01(self):
         schema = self.load_schema("""
             type Object1;


Code review comments:
diff_hunk: @@ -1200,6 +1202,39 @@ def _resolve_type_expr(
         )
 
 
+TRACER_TO_REAL_TYPE_MAP = {
+    qltracer.ObjectType: s_objtypes.ObjectType,
+    qltracer.ScalarType: s_scalars.ScalarType,
+    qltracer.Constraint: s_constr.Constraint,
+    qltracer.Annotation: s_anno.Annotation,
+    qltracer.Property: s_props.Property,
+    qltracer.Link: s_links.Link,
+}
+
+
+def _get_local_obj(
+    refname: s_name.QualName,
+    tracer_type: Type[qltracer.NamedObject],
+    sourcectx: Optional[parsing.ParserContext],
+    *,
+    ctx: LayoutTraceContext,
+) -> Optional[qltracer.NamedObject]:
+
+    obj = ctx.objects.get(refname)
+
+    if obj is not None and not isinstance(obj, tracer_type):
+        obj_type = TRACER_TO_REAL_TYPE_MAP[type(obj)]
+        real_type = TRACER_TO_REAL_TYPE_MAP[tracer_type]
+        raise errors.InvalidReferenceError(
+            f'{str(refname)!r} exists, but is a '
+            f'{obj_type.get_schema_class_displayname()!r}, '
path: edb/edgeql/declarative.py
text: @user1:
Hm, perhaps we could lose the quotes here.  `'test::X' exists, but is a scalar type, not a object type` reads better to me (minus a more obvious article error).

@author:
Sure

@author:
I fixed the articles too