Repository: geldata/gel
Pull request: Catch more errors in constraints and report them better
Disallow using EXISTS and other SET OF operations on MULTI pointers
(which was previously accepted but didn't work).

Produce a better error than "ISE: unexpectedly long path in simple
expr" for constraints that try to traverse links.

Turn the "multi-table constraints not supported" ISE into a real error
with a source context.

Fixes: #2207

Problem statement:
Constraint expressions with backlinks give ISE
<!-- Please search existing issues to avoid creating duplicates. -->
- EdgeDB Version: 1.0a7-2021011304
- OS Version: macOS 11.2

A type with a constraint expression containing a backlink causes an `edb.errors.InternalServerError: unexpectedly long path in simple expr` to be raised. SDL that reproduces is provided, but from testing, equivalent DDL statements cause the same issue, so it's not purely an issue with SDLs.

Steps to Reproduce:

1. Prepare the schema for migration.

2. Populate and commit the transaction.

Schema:
```
module default {
    abstract type User {
        required property name -> str;
        multi link groups -> MemberGroup;
    }

    type MemberGroup {
        required property name -> str;

        # Only Bob can have groups.
        constraint expression on (
            __subject__.<groups[IS User].name = "Bob"
        );
    }
}
```

Hints from issue discussion:
This shouldn't be an ISE, but it's a valid error: we don't support reaching across links in constraints yet, only immediate properties/links can be used in a constraint expression.

Commit message: Catch more errors in constraints and report them better

Disallow using EXISTS and other SET OF operations on MULTI pointers
(which was previously accepted but didn't work).

Produce a better error than "ISE: unexpectedly long path in simple
expr" for constraints that try to traverse links.

Turn the "multi-table constraints not supported" ISE into a real error
with a source context.

Patch to review:
diff --git a/edb/ir/utils.py b/edb/ir/utils.py
index e468dba731e..b5e225db7d4 100644
--- a/edb/ir/utils.py
+++ b/edb/ir/utils.py
@@ -326,3 +326,10 @@ def contains_dml(stmt: irast.Base, *, skip_bindings: bool=False) -> bool:
     visitor = ContainsDMLVisitor(skip_bindings=skip_bindings)
     res = visitor.visit(stmt) is True
     return res
+
+
+def contains_set_of_op(ir: irast.Base) -> bool:
+    flt = (lambda n: isinstance(n, irast.Call)
+           and any(x == ft.TypeModifier.SetOfType
+                   for x in n.params_typemods))
+    return bool(ast.find_children(ir, flt, terminate_early=True))
diff --git a/edb/pgsql/delta.py b/edb/pgsql/delta.py
index fe224fdc0cb..a73e890ee78 100644
--- a/edb/pgsql/delta.py
+++ b/edb/pgsql/delta.py
@@ -1326,7 +1326,8 @@ def apply(
             schemac_to_backendc = \
                 schemamech.ConstraintMech.\
                 schema_constraint_to_backend_constraint
-            bconstr = schemac_to_backendc(subject, constraint, schema, context)
+            bconstr = schemac_to_backendc(
+                subject, constraint, schema, context, self.source_context)
 
             op = dbops.CommandGroup(priority=1)
             op.add_command(bconstr.create_ops())
@@ -1352,11 +1353,13 @@ def apply(self, schema, context):
                 schemamech.ConstraintMech.\
                 schema_constraint_to_backend_constraint
 
-            bconstr = schemac_to_backendc(subject, constraint, schema, context)
+            bconstr = schemac_to_backendc(
+                subject, constraint, schema, context, self.source_context)
 
             orig_subject = constraint.get_subject(orig_schema)
             orig_bconstr = schemac_to_backendc(
-                orig_subject, constraint, orig_schema, context
+                orig_subject, constraint, orig_schema, context,
+                self.source_context,
             )
 
             op = dbops.CommandGroup(priority=1)
@@ -1403,13 +1406,15 @@ def apply(self, schema, context):
                 schemamech.ConstraintMech.\
                 schema_constraint_to_backend_constraint
 
-            bconstr = schemac_to_backendc(subject, constraint, schema, context)
+            bconstr = schemac_to_backendc(
+                subject, constraint, schema, context, self.source_context)
 
             orig_bconstr = schemac_to_backendc(
                 constraint.get_subject(orig_schema),
                 constraint,
                 orig_schema,
                 context,
+                self.source_context,
             )
 
             op = dbops.CommandGroup(priority=1)
@@ -1422,12 +1427,14 @@ def apply(self, schema, context):
                         child,
                         orig_schema,
                         context,
+                        self.source_context,
                     )
                     cbconstr = schemac_to_backendc(
                         child.get_subject(schema),
                         child,
                         schema,
                         context,
+                        self.source_context,
                     )
                     op.add_command(cbconstr.alter_ops(orig_cbconstr))
             elif not self.constraint_is_effective(schema, constraint):
@@ -1439,12 +1446,14 @@ def apply(self, schema, context):
                         child,
                         orig_schema,
                         context,
+                        self.source_context,
                     )
                     cbconstr = schemac_to_backendc(
                         child.get_subject(schema),
                         child,
                         schema,
                         context,
+                        self.source_context,
                     )
                     op.add_command(cbconstr.alter_ops(orig_cbconstr))
             else:
@@ -1473,7 +1482,8 @@ def apply(
                     schemamech.ConstraintMech.\
                     schema_constraint_to_backend_constraint
                 bconstr = schemac_to_backendc(
-                    subject, constraint, orig_schema, context)
+                    subject, constraint, orig_schema, context,
+                    self.source_context)
 
                 op = dbops.CommandGroup()
                 op.add_command(bconstr.delete_ops())
diff --git a/edb/pgsql/schemamech.py b/edb/pgsql/schemamech.py
index bb7a05caedd..1cc21117046 100644
--- a/edb/pgsql/schemamech.py
+++ b/edb/pgsql/schemamech.py
@@ -160,7 +160,7 @@ def _get_constraint_origin(cls, schema, constraint):
 
     @classmethod
     def schema_constraint_to_backend_constraint(
-            cls, subject, constraint, schema, context):
+            cls, subject, constraint, schema, context, source_context):
         assert constraint.get_subject(schema) is not None
 
         constraint_origin = cls._get_constraint_origin(schema, constraint)
@@ -188,8 +188,12 @@ def schema_constraint_to_backend_constraint(
         ref_tables = get_ref_storage_info(ir.schema, terminal_refs)
 
         if len(ref_tables) > 1:
-            raise ValueError(
-                'backend: multi-table constraints are not currently supported')
+            raise errors.InvalidConstraintDefinitionError(
+                f'Constraint {constraint.get_displayname(schema)} on '
+                f'{subject.get_displayname(schema)} is not supported '
+                f'because it would depend on multiple tables',
+                context=source_context,
+            )
         elif ref_tables:
             subject_db_name, _ = next(iter(ref_tables.items()))
         else:
diff --git a/edb/schema/constraints.py b/edb/schema/constraints.py
index f217e0b55bb..96d930ca9cd 100644
--- a/edb/schema/constraints.py
+++ b/edb/schema/constraints.py
@@ -918,28 +918,57 @@ def _populate_concrete_constraint_attrs(
                 context=expr_context
             )
 
-        if (subjectexpr is not None and isinstance(subject_obj, s_types.Type)
-                and subject_obj.is_object_type()):
+        if subjectexpr is not None:
+            if (isinstance(subject_obj, s_types.Type)
+                    and subject_obj.is_object_type()):
+                singletons = frozenset({subject_obj})
+            else:
+                singletons = frozenset()
+
             final_subjectexpr = s_expr.Expression.compiled(
                 subjectexpr,
                 schema=schema,
                 options=qlcompiler.CompilerOptions(
                     anchors={qlast.Subject().name: subject},
                     path_prefix_anchor=path_prefix_anchor,
-                    singletons=frozenset({subject_obj}),
+                    singletons=singletons,
                     apply_query_rewrites=not context.stdmode,
                 ),
             )
             assert isinstance(final_subjectexpr.irast, ir_ast.Statement)
 
-            if final_subjectexpr.irast.cardinality.is_multi():
-                refs = ir_utils.get_longest_paths(final_expr.irast)
-                if len(refs) > 1:
-                    raise errors.InvalidConstraintDefinitionError(
-                        "Constraint with multi cardinality may not "
-                        "reference multiple links or properties",
-                        context=expr_context
-                    )
+            refs = ir_utils.get_longest_paths(final_expr.irast)
+            has_multi = False
+            for ref in refs:
+                while ref.rptr:
+                    rptr = ref.rptr
+                    if rptr.ptrref.dir_cardinality.is_multi():
+                        has_multi = True
+                    if (not isinstance(rptr.ptrref,
+                                       ir_ast.TupleIndirectionPointerRef)
+                            and rptr.ptrref.source_ptr is None
+                            and rptr.source.rptr is not None):
+                        raise errors.InvalidConstraintDefinitionError(
+                            "Constraints may not traverse links",
+                            context=ref.context,
+                        )
+
+                    ref = rptr.source
+
+            if has_multi and len(refs) > 1:
+                raise errors.InvalidConstraintDefinitionError(
+                    "Constraint referencing MULTI links or properties "
+                    "may not reference multiple ones",
+                    context=expr_context
+                )
+
+            if has_multi and ir_utils.contains_set_of_op(
+                    final_subjectexpr.irast):
+                raise errors.InvalidConstraintDefinitionError(
+                    "Constraint referencing MULTI links or properties "
+                    "may not use SET OF operations",
+                    context=expr_context
+                )
 
         attrs['return_type'] = constr_base.get_return_type(schema)
         attrs['return_typemod'] = constr_base.get_return_typemod(schema)
diff --git a/tests/test_constraints.py b/tests/test_constraints.py
index 3cdc3bfd0e1..81cc5197b87 100644
--- a/tests/test_constraints.py
+++ b/tests/test_constraints.py
@@ -1051,14 +1051,17 @@ async def test_constraints_ddl_08(self):
             CREATE TYPE test::ObjCnstr2 {
                 CREATE MULTI PROPERTY first_name -> str;
                 CREATE MULTI PROPERTY last_name -> str;
+                CREATE LINK foo -> Object {
+                    CREATE PROPERTY p -> str;
+                };
                 CREATE CONSTRAINT exclusive on (__subject__.first_name);
             };
         """)
 
         async with self.assertRaisesRegexTx(
             edgedb.InvalidConstraintDefinitionError,
-            "Constraint with multi cardinality may not "
-            "reference multiple links or properties",
+            "Constraint referencing MULTI links or properties "
+            "may not reference multiple ones",
         ):
             await self.con.execute("""
                 ALTER TYPE test::ObjCnstr2 {
@@ -1067,6 +1070,63 @@ async def test_constraints_ddl_08(self):
                 };
             """)
 
+        async with self.assertRaisesRegexTx(
+            edgedb.InvalidConstraintDefinitionError,
+            "Constraint referencing MULTI links or properties "
+            "may not use SET OF operations",
+        ):
+            await self.con.execute("""
+                ALTER TYPE test::ObjCnstr2 {
+                    CREATE CONSTRAINT expression on (EXISTS .first_name);
+                };
+            """)
+
+        async with self.assertRaisesRegexTx(
+            edgedb.InvalidConstraintDefinitionError,
+            "Constraint referencing MULTI links or properties "
+            "may not use SET OF operations",
+        ):
+            await self.con.execute("""
+                ALTER TYPE test::ObjCnstr2 {
+                    ALTER PROPERTY first_name {
+                        CREATE CONSTRAINT expression on (EXISTS __subject__);
+                    }
+                };
+            """)
+
+        async with self.assertRaisesRegexTx(
+            edgedb.InvalidConstraintDefinitionError,
+            "Constraints may not traverse link",
+        ):
+            await self.con.execute("""
+                ALTER TYPE test::ObjCnstr2 {
+                    CREATE CONSTRAINT expression on (<str>.foo.id != 'lol');
+                };
+            """)
+
+        async with self.assertRaisesRegexTx(
+            edgedb.InvalidConstraintDefinitionError,
+            "Constraints may not traverse link",
+        ):
+            await self.con.execute("""
+                ALTER TYPE test::ObjCnstr2 {
+                    ALTER LINK foo {
+                        CREATE CONSTRAINT expression on (
+                            <str>__subject__.id != 'lol');
+                    }
+                };
+            """)
+
+        async with self.assertRaisesRegexTx(
+            edgedb.InvalidConstraintDefinitionError,
+            "not supported because it would depend on multiple tables",
+        ):
+            await self.con.execute("""
+                ALTER TYPE test::ObjCnstr2 {
+                    CREATE CONSTRAINT expression on (<str>.id != .foo@p);
+                };
+            """)
+
     async def test_constraints_ddl_09(self):
         await self.con.execute("""
             CREATE TYPE test::Label {
diff --git a/tests/test_edgeql_ddl.py b/tests/test_edgeql_ddl.py
index 28c750876e2..d2847126f57 100644
--- a/tests/test_edgeql_ddl.py
+++ b/tests/test_edgeql_ddl.py
@@ -10574,45 +10574,6 @@ async def test_edgeql_ddl_link_policy_11(self):
             DELETE Tgt;
         """)
 
-    @test.xfail('''
-        We can't properly compile the constraint here in the basic case,
-        let alone combined with DELETE ALLOW.
-
-        We probably should be rejecting the constraint.
-    ''')
-    async def test_edgeql_ddl_link_policy_12(self):
-        # Should we even be allowing (EXISTS .tgt) as a constraint?
-        await self.con.execute(r"""
-            SET MODULE test;
-
-            CREATE TYPE Tgt;
-            CREATE TYPE Foo {
-                CREATE MULTI LINK tgt -> Tgt {
-                    ON TARGET DELETE ALLOW;
-                };
-                CREATE CONSTRAINT expression on (EXISTS .tgt);
-
-            };
-            CREATE TYPE Bar EXTENDING Foo;
-        """)
-
-        await self.con.execute(r"""
-            INSERT Bar { tgt := (INSERT Tgt) };
-        """)
-
-        async with self.assertRaisesRegexTx(
-            edgedb.ConstraintViolationError,
-            'invalid Bar',
-        ):
-            await self.con.execute("""
-                DELETE Tgt;
-            """)
-
-        await self.con.execute("""
-            DELETE Bar;
-            DELETE Tgt;
-        """)
-
 
 class TestConsecutiveMigrations(tb.DDLTestCase):
     TRANSACTION_ISOLATION = False


Code review comments:
diff_hunk: @@ -188,8 +188,12 @@ def schema_constraint_to_backend_constraint(
         ref_tables = get_ref_storage_info(ir.schema, terminal_refs)
 
         if len(ref_tables) > 1:
-            raise ValueError(
-                'backend: multi-table constraints are not currently supported')
+            raise errors.InvalidConstraintDefinitionError(
+                f'Constraint {constraint.get_displayname(schema)} on '
+                f'{subject.get_displayname(schema)} is not supported '
+                f'because it would depend on multiple tables',
path: edb/pgsql/schemamech.py
text: @user1:
```suggestion
                f'because it would depend on multiple objects',
```

diff_hunk: @@ -918,28 +918,57 @@ def _populate_concrete_constraint_attrs(
                 context=expr_context
             )
 
-        if (subjectexpr is not None and isinstance(subject_obj, s_types.Type)
-                and subject_obj.is_object_type()):
+        if subjectexpr is not None:
+            if (isinstance(subject_obj, s_types.Type)
+                    and subject_obj.is_object_type()):
+                singletons = frozenset({subject_obj})
+            else:
+                singletons = frozenset()
+
             final_subjectexpr = s_expr.Expression.compiled(
                 subjectexpr,
                 schema=schema,
                 options=qlcompiler.CompilerOptions(
                     anchors={qlast.Subject().name: subject},
                     path_prefix_anchor=path_prefix_anchor,
-                    singletons=frozenset({subject_obj}),
+                    singletons=singletons,
                     apply_query_rewrites=not context.stdmode,
                 ),
             )
             assert isinstance(final_subjectexpr.irast, ir_ast.Statement)
 
-            if final_subjectexpr.irast.cardinality.is_multi():
-                refs = ir_utils.get_longest_paths(final_expr.irast)
-                if len(refs) > 1:
-                    raise errors.InvalidConstraintDefinitionError(
-                        "Constraint with multi cardinality may not "
-                        "reference multiple links or properties",
-                        context=expr_context
-                    )
+            refs = ir_utils.get_longest_paths(final_expr.irast)
+            has_multi = False
+            for ref in refs:
+                while ref.rptr:
+                    rptr = ref.rptr
+                    if rptr.ptrref.dir_cardinality.is_multi():
+                        has_multi = True
+                    if (not isinstance(rptr.ptrref,
+                                       ir_ast.TupleIndirectionPointerRef)
+                            and rptr.ptrref.source_ptr is None
+                            and rptr.source.rptr is not None):
+                        raise errors.InvalidConstraintDefinitionError(
+                            "Constraints may not traverse links",
+                            context=ref.context,
+                        )
+
+                    ref = rptr.source
+
+            if has_multi and len(refs) > 1:
+                raise errors.InvalidConstraintDefinitionError(
+                    "Constraint referencing MULTI links or properties "
+                    "may not reference multiple ones",
+                    context=expr_context
+                )
+
+            if has_multi and ir_utils.contains_set_of_op(
+                    final_subjectexpr.irast):
+                raise errors.InvalidConstraintDefinitionError(
+                    "Constraint referencing MULTI links or properties "
+                    "may not use SET OF operations",
path: edb/schema/constraints.py
text: @user1:
```suggestion
                    "cannot use aggregate functions or operators "
                    "in a non-aggregating constraint",
```

diff_hunk: @@ -918,28 +918,57 @@ def _populate_concrete_constraint_attrs(
                 context=expr_context
             )
 
-        if (subjectexpr is not None and isinstance(subject_obj, s_types.Type)
-                and subject_obj.is_object_type()):
+        if subjectexpr is not None:
+            if (isinstance(subject_obj, s_types.Type)
+                    and subject_obj.is_object_type()):
+                singletons = frozenset({subject_obj})
+            else:
+                singletons = frozenset()
+
             final_subjectexpr = s_expr.Expression.compiled(
                 subjectexpr,
                 schema=schema,
                 options=qlcompiler.CompilerOptions(
                     anchors={qlast.Subject().name: subject},
                     path_prefix_anchor=path_prefix_anchor,
-                    singletons=frozenset({subject_obj}),
+                    singletons=singletons,
                     apply_query_rewrites=not context.stdmode,
                 ),
             )
             assert isinstance(final_subjectexpr.irast, ir_ast.Statement)
 
-            if final_subjectexpr.irast.cardinality.is_multi():
-                refs = ir_utils.get_longest_paths(final_expr.irast)
-                if len(refs) > 1:
-                    raise errors.InvalidConstraintDefinitionError(
-                        "Constraint with multi cardinality may not "
-                        "reference multiple links or properties",
-                        context=expr_context
-                    )
+            refs = ir_utils.get_longest_paths(final_expr.irast)
+            has_multi = False
+            for ref in refs:
+                while ref.rptr:
+                    rptr = ref.rptr
+                    if rptr.ptrref.dir_cardinality.is_multi():
+                        has_multi = True
+                    if (not isinstance(rptr.ptrref,
+                                       ir_ast.TupleIndirectionPointerRef)
+                            and rptr.ptrref.source_ptr is None
+                            and rptr.source.rptr is not None):
+                        raise errors.InvalidConstraintDefinitionError(
+                            "Constraints may not traverse links",
path: edb/schema/constraints.py
text: @user1:
```suggestion
                            "constraints cannot contain paths with more than one hop",
```

diff_hunk: @@ -918,28 +918,57 @@ def _populate_concrete_constraint_attrs(
                 context=expr_context
             )
 
-        if (subjectexpr is not None and isinstance(subject_obj, s_types.Type)
-                and subject_obj.is_object_type()):
+        if subjectexpr is not None:
+            if (isinstance(subject_obj, s_types.Type)
+                    and subject_obj.is_object_type()):
+                singletons = frozenset({subject_obj})
+            else:
+                singletons = frozenset()
+
             final_subjectexpr = s_expr.Expression.compiled(
                 subjectexpr,
                 schema=schema,
                 options=qlcompiler.CompilerOptions(
                     anchors={qlast.Subject().name: subject},
                     path_prefix_anchor=path_prefix_anchor,
-                    singletons=frozenset({subject_obj}),
+                    singletons=singletons,
                     apply_query_rewrites=not context.stdmode,
                 ),
             )
             assert isinstance(final_subjectexpr.irast, ir_ast.Statement)
 
-            if final_subjectexpr.irast.cardinality.is_multi():
-                refs = ir_utils.get_longest_paths(final_expr.irast)
-                if len(refs) > 1:
-                    raise errors.InvalidConstraintDefinitionError(
-                        "Constraint with multi cardinality may not "
-                        "reference multiple links or properties",
-                        context=expr_context
-                    )
+            refs = ir_utils.get_longest_paths(final_expr.irast)
+            has_multi = False
+            for ref in refs:
+                while ref.rptr:
+                    rptr = ref.rptr
+                    if rptr.ptrref.dir_cardinality.is_multi():
+                        has_multi = True
+                    if (not isinstance(rptr.ptrref,
+                                       ir_ast.TupleIndirectionPointerRef)
+                            and rptr.ptrref.source_ptr is None
+                            and rptr.source.rptr is not None):
+                        raise errors.InvalidConstraintDefinitionError(
+                            "Constraints may not traverse links",
+                            context=ref.context,
+                        )
+
+                    ref = rptr.source
+
+            if has_multi and len(refs) > 1:
+                raise errors.InvalidConstraintDefinitionError(
+                    "Constraint referencing MULTI links or properties "
+                    "may not reference multiple ones",
path: edb/schema/constraints.py
text: @user1:
```suggestion
                    "cannot reference multiple link or properties in an aggregate constraint",
```

Additionally, only constraints with `is_aggregate = True` can reference `MULTI`.  The only `is_aggregate` constraint is `exclusive`.



@author:
Hm. Having constraints on the individual values in a multi property, for example, works, and is useful, though.

@user1:
Good point

```suggestion
                    "cannot reference multiple links or properties in a constraint where at least one link or property is MULTI",
```