Repository: geldata/gel
Pull request: Disallow using std::exclusive on scalar types
Fixes #1182.

Problem statement:
InternalServerError error creating scalar type
<!-- Please search existing issues to avoid creating duplicates. -->
- EdgeDB Version: `1.0-alpha.2+dev.505.g5b68ec24`

Steps to Reproduce:

```
START TRANSACTION;
CREATE MIGRATION init TO {
    module default {
        scalar type unique_name extending str {
            constraint exclusive;
        }
    }
};
COMMIT MIGRATION init;
```
Error: `InternalServerError: column "78096142-4760-11ea-ad7d-c3dbbf5ced49" does not exist`

I found another InternalServerError while playing with this:
```
START TRANSACTION;
CREATE MIGRATION init TO {
    module default {
        scalar type unique_name extending str {
            constraint exclusive on (str_lower(__subject__));
        }
    }
};
COMMIT MIGRATION init;
```
Error: `InternalServerError: argument of CHECK must be type boolean, not type text`

I'm not sure if the latter is a bug, I'm using it wrong, but `InternalServerError` is scary.

Is it possible to declare `exclusive` constraint in scalar so that it checks lowercase value?
My plan to use it is:
```
type User {
    required property nickname -> unique_name;
    # ...
    index on (str_lower(__subject__.nickname));
}
```
`unique_name` has other constraints, but I removed them to simplify my report.

Hints from issue discussion:
Ah. Right. `exclusive` constraints are not supported on scalar types. The bug is that there's no explicit check for that.  

You can only declare such constraints on links and properties (support for object types is being added), so:

```
type User {
    required property nickname -> str {
        constraint exclusive on (str_lower(__subject__));
    }
}
```

You don't need a separate index for the property, since the constraint would normally generate an implicit unique index.
@vpetrovykh, please look into adding an explicit check, prohibiting exclusive constraints on scalars.  I'm thinking this should be done by marking a constraint as an "aggregate" constraint, i.e. a `set of` function.
"Aggregate" constraint sounds interesting, but the issue here is that any constraint that refers to the pointer source will have issues when applied to a scalar type, regardless of whether it's aggregate or not. Incidentally, is it possible to make a constraint referring to a link property and the link target? Because that type of constraint could not be applied to a scalar either.

To make a more concrete example, consider this:
```
type Foo {
  proeprty val -> int64;
}

type Bar {
  link foo -> Foo {
    property mult -> int64;
  }
}
```
The question is whether I can create a constraint on link `foo` that says that the link target's `val` must be divisible by `mult`? 

So it looks to me that there are constraints that apply to scalars (which can be used on scalar types or properties) and constraints that apply to pointers (which can only apply to links and properties). Whether they are aggregate or not is an orthogonal parameter here.


> The question is whether I can create a constraint on link `foo` that says that the link target's `val` must be divisible by `mult`?

Going "up" like this is not supported for all pointer constraints regardless of whether they're scalar or not.  If you need to define a constraint on multiple properties or links, you must use an `object type` level constraint.

> "Aggregate" constraint sounds interesting, but the issue here is that any constraint that refers to the pointer source will have issues when applied to a scalar type

> Whether they are aggregate or not is an orthogonal parameter here.

No. The original report had an `exclusive` constraint defined on  `scalar type` *definition*.  It's perfectly fine to declare `exclusive` on a scalar *property*, though.

This is invalid:

```
        scalar type unique_name extending str {
            constraint exclusive;
        }
```

This is valid:

```
        type Foo {
            link name -> unique_name {
                constraint exclusive;
            }
        }
```

To summarize a slack discussion, it appears that a more reasonable flag name is "pointer-only" (maybe just "pointer") rather than "aggregate". The reasoning is as follows:
- `exclusive` constraint can only be applied to pointers.
- One could interpret `exclusive` constraint applied to an object type to be a NOP, because all objects are already unique. However, currently we forbid applying this constraint to object types (it's illegal to do `type Foo {constraint exclusive}`).

Basically if we forbid the `exclusive` constraint for scalar type definitions and object type definitions, then it's more appropriate to label it "pointer-only". On the other hand, if we allow it to appear on object types (even though it has no effect), then it would make sense to label it as "aggregate" and use that as the feature that makes it illegal for scalar types.

Proposed syntax (for internal usage only, so valid only during EdgeDB initialization):
```
CREATE ABSTRACT CONSTRAINT
std::exclusive EXTENDING std::constraint
{
    SET is_pointer_only := true;
    SET errmessage := '{__subject__} violates exclusivity constraint';
    USING (std::_is_exclusive(__subject__));
};
```
Object-level constraints are coming (see #1164), so `is_aggregate` would be a more appropriate flag here.

Commit message: Disallow using std::exclusive on scalar types

Fixes #1182.

Patch to review:
diff --git a/edb/lib/std/50-constraints.edgeql b/edb/lib/std/50-constraints.edgeql
index 684d7eb0113..069e2ca0b20 100644
--- a/edb/lib/std/50-constraints.edgeql
+++ b/edb/lib/std/50-constraints.edgeql
@@ -46,6 +46,7 @@ std::expression EXTENDING std::constraint
 CREATE ABSTRACT CONSTRAINT
 std::exclusive EXTENDING std::constraint
 {
+    SET is_aggregate := true;
     SET errmessage := '{__subject__} violates exclusivity constraint';
     USING (std::_is_exclusive(__subject__));
 };
diff --git a/edb/schema/constraints.py b/edb/schema/constraints.py
index ec44f17e2eb..0fffea5ac10 100644
--- a/edb/schema/constraints.py
+++ b/edb/schema/constraints.py
@@ -28,6 +28,7 @@
 from edb.edgeql import qltypes as ft
 from edb.edgeql import parser as qlparser
 from edb.edgeql import utils as qlutils
+from edb.schema import scalars as s_scalars
 
 from . import abc as s_abc
 from . import annos as s_anno
@@ -44,7 +45,6 @@
 
 if TYPE_CHECKING:
     from edb.common import parsing as c_parsing
-    from edb.schema import scalars as s_scalars
     from edb.schema import schema as s_schema
 
 
@@ -103,6 +103,9 @@ class Constraint(referencing.ReferencedInheritingObject,
     errmessage = so.SchemaField(
         str, default=None, compcoef=0.971, allow_ddl_set=True)
 
+    is_aggregate = so.SchemaField(
+        bool, default=False, compcoef=0.971, allow_ddl_set=False)
+
     def get_verbosename(
         self,
         schema: s_schema.Schema,
@@ -733,6 +736,12 @@ def _populate_concrete_constraint_attrs(
                 f'subjectexpr is already defined for {name!r}'
             )
 
+        if (isinstance(subject_obj, s_scalars.ScalarType)
+                and constr_base.get_is_aggregate(schema)):
+            raise errors.InvalidConstraintDefinitionError(
+                f'Constraint {name} may not be used on scalar types'
+            )
+
         if subjectexpr is not None:
             subject_ql = subjectexpr.qlast
             subject = subject_ql
diff --git a/edb/server/defines.py b/edb/server/defines.py
index a9206326f54..bce621d5fa4 100644
--- a/edb/server/defines.py
+++ b/edb/server/defines.py
@@ -27,7 +27,7 @@
 EDGEDB_VISIBLE_METADATA_PREFIX = r'EdgeDB metadata follows, do not modify.\n'
 
 # Increment this whenever the database layout or stdlib changes.
-EDGEDB_CATALOG_VERSION = 2020_07_23_00_00
+EDGEDB_CATALOG_VERSION = 2020_07_23_13_00
 
 # Resource limit on open FDs for the server process.
 # By default, at least on macOS, the max number of open FDs
diff --git a/tests/test_constraints.py b/tests/test_constraints.py
index 68abe3009eb..4375812d83e 100644
--- a/tests/test_constraints.py
+++ b/tests/test_constraints.py
@@ -1025,6 +1025,17 @@ async def test_constraints_ddl_error_05(self):
                 "got scalar type 'std::str'"):
             await self.con.execute(qry)
 
+        qry = """
+            CREATE SCALAR TYPE wrong extending str {
+                CREATE CONSTRAINT exclusive;
+            };
+        """
+
+        with self.assertRaisesRegex(
+                edgedb.SchemaDefinitionError,
+                "Constraint std::exclusive may not be used on scalar types"):
+            await self.con.execute(qry)
+
     async def test_constraints_ddl_error_06(self):
         async with self._run_and_rollback():
             with self.assertRaisesRegex(


Code review comments:
diff_hunk: @@ -733,6 +736,12 @@ def _populate_concrete_constraint_attrs(
                 f'subjectexpr is already defined for {name!r}'
             )
 
+        if (isinstance(subject_obj, s_scalars.ScalarType)
+                and constr_base.get_is_aggregate(schema)):
+            raise errors.InvalidConstraintDefinitionError(
+                f'Constraint {name} may not be used on scalar types'
path: edb/schema/constraints.py
text: @user1:
```suggestion
                f'{constr_base.get_verbosename(schema)} may not be used on scalar types'
```

@user1:
Hm. Sorry, `abstract constraint 'std::exclusive'` is a tad misleading and irrelevant in this context, so let's do
`f'constraint {constr_base.get_displayname(schema)} may not ...` (to elide the word "abstract")