Repository: geldata/gel
Pull request: Disallow scalar types with more than one concrete base
Fixes #1790.

Problem statement:
Prohibit inheriting scalar types from more than one non-abstract scalar type
Currently:

```
select> CREATE SCALAR TYPE test::myint EXTENDING std::int64, std::str;
OK: CREATE
```

We have to prohibit this by enforcing that there must be exactly one base non-abstract scalar type in the ancestry of a new non-abstract scalar type.

Hints from issue discussion:
Why that rule instead of requiring a single root scalar type?
> Why that rule instead of requiring a single root scalar type?

Because multiple inheritance from different abstract scalar types is useful.  For example:

```
CREATE SCALAR TYPE std::bigint EXTENDING std::anynumeric, std::anyint;
```

Commit message: Disallow scalar types with more than one concrete base

Fixes #1790.

Patch to review:
diff --git a/edb/schema/scalars.py b/edb/schema/scalars.py
index 53ffdf84f5c..6bf2b94a4a7 100644
--- a/edb/schema/scalars.py
+++ b/edb/schema/scalars.py
@@ -222,7 +222,45 @@ class ScalarTypeCommand(
     schema_metaclass=ScalarType,
     context_class=ScalarTypeCommandContext,
 ):
-    pass
+    def apply(
+        self,
+        schema: s_schema.Schema,
+        context: sd.CommandContext,
+    ) -> s_schema.Schema:
+        v_ancestors = self.get_attribute_value('ancestors')
+        if v_ancestors:
+            if isinstance(v_ancestors, so.ObjectCollection):
+                rancestors = list(v_ancestors.objects(schema))
+            else:
+                ancestors = (
+                    v_ancestors.items
+                    if isinstance(v_ancestors, so.ObjectCollectionShell)
+                    else v_ancestors)
+                rancestors = [x.resolve(schema) for x in ancestors]
+            concrete_ancestors = {
+                ancestor for ancestor in rancestors
+                if not ancestor.get_is_abstract(schema)
+            }
+            # Filter out anything that has a subclass relation with
+            # every other concrete ancestor. This lets us allow chains
+            # of concrete scalar types while prohibiting diamonds (for
+            # example if X <: A, B <: int64 where A, B are concrete).
+            # (If we wanted to allow diamonds, we could instead filter out
+            # anything that has concrete bases.)
+            concrete_ancestors = {
+                c1 for c1 in concrete_ancestors
+                if not all(c1 == c2 or c1.issubclass(schema, c2)
+                           or c2.issubclass(schema, c1)
+                           for c2 in concrete_ancestors)
+            }
+
+            if len(concrete_ancestors) > 1:
+                raise errors.SchemaError(
+                    f'scalar type may not have more than '
+                    f'one concrete base type',
+                )
+
+        return super().apply(schema, context)
 
 
 class CreateScalarType(
diff --git a/tests/test_edgeql_data_migration.py b/tests/test_edgeql_data_migration.py
index 7ba688de43b..795319a344a 100644
--- a/tests/test_edgeql_data_migration.py
+++ b/tests/test_edgeql_data_migration.py
@@ -7508,3 +7508,15 @@ async def test_edgeql_migration_rename_type_03(self):
         """)
 
         await self.migrate("")
+
+    async def test_edgeql_migration_invalid_scalar_01(self):
+        with self.assertRaisesRegex(
+                edgedb.SchemaError,
+                r"may not have more than one concrete base type"):
+            await self.con.execute(r"""
+                START MIGRATION TO {
+                    abstract scalar type test::lol extending str;
+                    scalar type test::myint extending int64, test::lol;
+                };
+                POPULATE MIGRATION;
+            """)
diff --git a/tests/test_edgeql_ddl.py b/tests/test_edgeql_ddl.py
index 65741bbb7d7..48cd35ef777 100644
--- a/tests/test_edgeql_ddl.py
+++ b/tests/test_edgeql_ddl.py
@@ -2967,6 +2967,81 @@ async def test_edgeql_ddl_operator_11(self):
                 };
             ''')
 
+    async def test_edgeql_ddl_scalar_01(self):
+        with self.assertRaisesRegex(
+                edgedb.SchemaError,
+                r'may not have more than one concrete base type'):
+            await self.con.execute('''
+                CREATE SCALAR TYPE test::myint EXTENDING std::int64, std::str;
+            ''')
+
+    async def test_edgeql_ddl_scalar_02(self):
+        await self.con.execute('''
+            CREATE ABSTRACT SCALAR TYPE test::a EXTENDING std::int64;
+            CREATE ABSTRACT SCALAR TYPE test::b EXTENDING std::str;
+        ''')
+
+        with self.assertRaisesRegex(
+                edgedb.SchemaError,
+                r'may not have more than one concrete base type'):
+            await self.con.execute('''
+                CREATE SCALAR TYPE test::myint EXTENDING test::a, test::b;
+            ''')
+
+    async def test_edgeql_ddl_scalar_03(self):
+        await self.con.execute('''
+            CREATE ABSTRACT SCALAR TYPE test::a EXTENDING std::int64;
+            CREATE ABSTRACT SCALAR TYPE test::b EXTENDING std::str;
+            CREATE SCALAR TYPE test::myint EXTENDING test::a;
+        ''')
+
+        with self.assertRaisesRegex(
+                edgedb.SchemaError,
+                r'may not have more than one concrete base type'):
+            await self.con.execute('''
+                ALTER SCALAR TYPE test::myint EXTENDING test::b;
+            ''')
+
+    async def test_edgeql_ddl_scalar_04(self):
+        await self.con.execute('''
+            CREATE ABSTRACT SCALAR TYPE test::a;
+            CREATE SCALAR TYPE test::myint EXTENDING int64, test::a;
+        ''')
+
+        with self.assertRaisesRegex(
+                edgedb.SchemaError,
+                r'may not have more than one concrete base type'):
+            await self.con.execute('''
+                ALTER SCALAR TYPE test::a EXTENDING str;
+            ''')
+
+    async def test_edgeql_ddl_scalar_05(self):
+        await self.con.execute('''
+            CREATE ABSTRACT SCALAR TYPE test::a EXTENDING std::int64;
+            CREATE ABSTRACT SCALAR TYPE test::b EXTENDING std::int64;
+            CREATE SCALAR TYPE test::myint EXTENDING test::a, test::b;
+        ''')
+
+    async def test_edgeql_ddl_scalar_06(self):
+        await self.con.execute('''
+            CREATE SCALAR TYPE test::myint EXTENDING int64;
+            CREATE SCALAR TYPE test::myint2 EXTENDING test::myint;
+        ''')
+
+    async def test_edgeql_ddl_scalar_07(self):
+        await self.con.execute('''
+            CREATE SCALAR TYPE test::a EXTENDING std::str;
+            CREATE SCALAR TYPE test::b EXTENDING std::str;
+        ''')
+
+        # I think we want to prohibit this kind of diamond pattern
+        with self.assertRaisesRegex(
+                edgedb.SchemaError,
+                r'may not have more than one concrete base type'):
+            await self.con.execute('''
+                CREATE SCALAR TYPE test::myint EXTENDING test::a, test::b;
+            ''')
+
     async def test_edgeql_ddl_cast_01(self):
         await self.con.execute('''
             CREATE SCALAR TYPE test::type_a EXTENDING std::str;


Code review comments:
diff_hunk: @@ -222,7 +222,45 @@ class ScalarTypeCommand(
     schema_metaclass=ScalarType,
     context_class=ScalarTypeCommandContext,
 ):
-    pass
+    def apply(
+        self,
+        schema: s_schema.Schema,
+        context: sd.CommandContext,
+    ) -> s_schema.Schema:
+        v_ancestors = self.get_attribute_value('ancestors')
+        if v_ancestors:
+            if isinstance(v_ancestors, so.ObjectCollection):
+                rancestors = list(v_ancestors.objects(schema))
+            else:
+                ancestors = (
+                    v_ancestors.items
+                    if isinstance(v_ancestors, so.ObjectCollectionShell)
+                    else v_ancestors)
+                rancestors = [x.resolve(schema) for x in ancestors]
+            concrete_ancestors = {
+                ancestor for ancestor in rancestors
+                if not ancestor.get_is_abstract(schema)
+            }
+            # Filter out anything that has a subclass relation with
+            # every other concrete ancestor. This lets us allow chains
+            # of concrete scalar types while prohibiting diamonds (for
+            # example if X <: A, B <: int64 where A, B are concrete).
+            # (If we wanted to allow diamonds, we could instead filter out
+            # anything that has concrete bases.)
+            concrete_ancestors = {
+                c1 for c1 in concrete_ancestors
+                if not all(c1 == c2 or c1.issubclass(schema, c2)
+                           or c2.issubclass(schema, c1)
+                           for c2 in concrete_ancestors)
+            }
+
+            if len(concrete_ancestors) > 1:
+                raise errors.SchemaError(
+                    f'scalar type may not have more than '
+                    f'one concrete base type',
path: edb/schema/scalars.py
text: @user1:
```suggestion
                    f'one concrete base type', context=self.source_context
```

diff_hunk: @@ -222,7 +222,45 @@ class ScalarTypeCommand(
     schema_metaclass=ScalarType,
     context_class=ScalarTypeCommandContext,
 ):
-    pass
+    def apply(
path: edb/schema/scalars.py
text: @user1:
It's better to overload `validate_create()` and `validate_alter()` instead of `apply()` (you almost never want to overload `apply()`).

@author:
This isn't why I did it that way originally, but that approach doesn't catch the case where an existing abstract base has a new concrete base added (since the AlterScalarType that updates ancestors is generated after `validate_alter` is called, I think).

I think that we could have validate_alter explicitly search all descendants when bases are added and explicitly do this check on them. Is that an improvement over overloading `apply()`?

@user1:
Yea, that would be a cleaner approach, I think.