Repository: geldata/gel
Pull request: Update union types in reponse to changes in their constituent types
Fixes #2409.

Problem statement:
Server could not figure out migrations
- EdgeDB Version: 1.0-beta.2+dev.5560.g9e95b0fa7

Just doing `create-migration` for this schema from scratch makes an error:
```
module default {
    type Category {
        required property title -> str;
        required property deleted := EXISTS(.<element[IS DeletionRecord]);
    };
    type Article {
        required property title -> str;
        required property deleted := EXISTS(.<element[IS DeletionRecord]);
    };
    type DeletionRecord {
        required link element -> Article | Category {
            on target delete delete source;
            constraint exclusive;
        }
    }
};
```
Removing `title` or `deleted` from either `Article` or `Category` fixes the issue (perhaps making object not so similar), but adding another property doesn't. 

CLI output:
```
did you create object type 'default::Article'? [y,n,l,c,b,s,q,?]
y
did you create object type 'default::Category'? [y,n,l,c,b,s,q,?]
y
did you create object type 'default::DeletionRecord'? [y,n,l,c,b,s,q,?]
y
Following extra DDL statements will be applied:
    ALTER TYPE default::Article {
        CREATE REQUIRED PROPERTY deleted := (EXISTS (.<element[IS default::DeletionRecord]));
    };
(approved as part as part of an earlier prompt)
Following extra DDL statements will be applied:
    ALTER TYPE default::Category {
        CREATE REQUIRED PROPERTY deleted := (EXISTS (.<element[IS default::DeletionRecord]));
    };
(approved as part as part of an earlier prompt)
edgedb error: Server could not figure out migration with your answers. Please retry with different answers
```

Hints from issue discussion:
I don't see a link to `Category` in the schema and yet the output implies it's there.
> I don't see a link to `Category` in the schema and yet the output implies it's there.

`element` is a link to an union `Article | Category`
Yes, I see `element`, but not `category`.
> Yes, I see `element`, but not `category`.

Well, that one is commented out, without it bug also reproduces. I've updated CLI output in the issue description.
This one should be fun: the underlying issue seems to be that we don't recompute the contents of union types when one of the constituents is altered.

Commit message: Update union types in reponse to changes in their constituent types

Fixes #2409.

Patch to review:
diff --git a/edb/schema/links.py b/edb/schema/links.py
index 0f00ec29769..dc2f2417342 100644
--- a/edb/schema/links.py
+++ b/edb/schema/links.py
@@ -660,7 +660,7 @@ def _apply_field_ast(
 
 class DeleteLink(
     LinkCommand,
-    referencing.DeleteReferencedInheritingObject[Link],
+    pointers.DeletePointer[Link],
 ):
     astnode = [qlast.DropConcreteLink, qlast.DropLink]
     referenced_astnode = qlast.DropConcreteLink
diff --git a/edb/schema/lproperties.py b/edb/schema/lproperties.py
index 4a81ed5aed0..9fad1f63a2a 100644
--- a/edb/schema/lproperties.py
+++ b/edb/schema/lproperties.py
@@ -435,7 +435,7 @@ def _get_ast(
 
 class DeleteProperty(
     PropertyCommand,
-    referencing.DeleteReferencedInheritingObject[Property],
+    pointers.DeletePointer[Property],
 ):
     astnode = [qlast.DropConcreteProperty,
                qlast.DropProperty]
diff --git a/edb/schema/objtypes.py b/edb/schema/objtypes.py
index 877e62d2db4..ee2e0eb5876 100644
--- a/edb/schema/objtypes.py
+++ b/edb/schema/objtypes.py
@@ -465,6 +465,52 @@ class AlterObjectType(ObjectTypeCommand,
                       inheriting.AlterInheritingObject[ObjectType]):
     astnode = qlast.AlterObjectType
 
+    def _alter_finalize(
+        self,
+        schema: s_schema.Schema,
+        context: sd.CommandContext,
+    ) -> s_schema.Schema:
+
+        if not context.canonical:
+            # If this type is contained in any unions, we need to
+            # update them with any additions or alterations made to
+            # this type. (Deletions are already handled in DeletePointer.)
+            unions = schema.get_referrers(
+                self.scls, scls_type=ObjectType, field_name='union_of')
+
+            orig_disable = context.disable_dep_verification
+
+            for union in unions:
+                if union.get_is_opaque_union(schema):
+                    continue
+
+                delete = union.init_delta_command(schema, sd.DeleteObject)
+
+                context.disable_dep_verification = True
+                nschema = delete.apply(schema, context)
+                context.disable_dep_verification = orig_disable
+
+                nschema, nunion = utils.get_union_type(
+                    nschema,
+                    types=union.get_union_of(schema).objects(schema),
+                    opaque=union.get_is_opaque_union(schema),
+                    module=union.get_name(schema).module,
+                )
+                assert isinstance(nunion, ObjectType)
+
+                diff = union.as_alter_delta(
+                    other=nunion,
+                    self_schema=schema,
+                    other_schema=nschema,
+                    confidence=1.0,
+                    context=so.ComparisonContext(),
+                )
+
+                schema = diff.apply(schema, context)
+                self.add(diff)
+
+        return super()._alter_finalize(schema, context)
+
 
 class DeleteObjectType(
     ObjectTypeCommand,
diff --git a/edb/schema/pointers.py b/edb/schema/pointers.py
index 3cecd51c8cf..92621f6e04f 100644
--- a/edb/schema/pointers.py
+++ b/edb/schema/pointers.py
@@ -1628,6 +1628,30 @@ def as_inherited_ref_cmd(
         return cmd
 
 
+class DeletePointer(
+    referencing.DeleteReferencedInheritingObject[Pointer_T],
+    PointerCommand[Pointer_T],
+):
+    def _canonicalize(
+        self,
+        schema: s_schema.Schema,
+        context: sd.CommandContext,
+        scls: Pointer_T,
+    ) -> List[sd.Command]:
+        commands = super()._canonicalize(schema, context, scls)
+
+        # Any union type that references this field needs to have it
+        # deleted.
+        unions = schema.get_referrers(
+            self.scls, scls_type=Pointer, field_name='union_of')
+        for union in unions:
+            group, _, _ = union.init_delta_branch(
+                schema, context, sd.DeleteObject)
+            commands.append(group)
+
+        return commands
+
+
 class SetPointerType(
     referencing.ReferencedInheritingObjectCommand[Pointer_T],
     inheriting.AlterInheritingObjectFragment[Pointer_T],
diff --git a/tests/test_edgeql_data_migration.py b/tests/test_edgeql_data_migration.py
index 6e3e4ca01a1..45d3a48d0dc 100644
--- a/tests/test_edgeql_data_migration.py
+++ b/tests/test_edgeql_data_migration.py
@@ -8751,6 +8751,23 @@ async def test_edgeql_migration_user_input_04(self):
             ],
         )
 
+    async def test_edgeql_migration_union_01(self):
+        await self.migrate('''
+            type Category {
+                required property title -> str;
+                required property deleted :=
+                    EXISTS(.<element[IS DeletionRecord]);
+            };
+            type Article {
+                required property title -> str;
+                required property deleted :=
+                    EXISTS(.<element[IS DeletionRecord]);
+            };
+            type DeletionRecord {
+                link element -> Article | Category;
+            }
+        ''')
+
     async def test_edgeql_migration_misplaced_commands(self):
         async with self.assertRaisesRegexTx(
             edgedb.QueryError,
diff --git a/tests/test_edgeql_ddl.py b/tests/test_edgeql_ddl.py
index 7930ea61c96..05953a6573c 100644
--- a/tests/test_edgeql_ddl.py
+++ b/tests/test_edgeql_ddl.py
@@ -11115,6 +11115,93 @@ async def test_edgeql_ddl_new_required_pointer_05(self):
             ]
         )
 
+    async def test_edgeql_ddl_alter_union_01(self):
+        await self.con.execute(r"""
+            SET MODULE test;
+            CREATE TYPE Foo;
+            CREATE TYPE Bar;
+        """)
+
+        await self.con.execute(r"""
+            CREATE TYPE Ref {
+                CREATE LINK fubar -> Foo | Bar;
+            }
+        """)
+
+        await self.con.execute(r"""
+            ALTER TYPE Foo CREATE PROPERTY x -> str;
+            ALTER TYPE Bar CREATE PROPERTY x -> str;
+        """)
+
+        await self.assert_query_result(
+            r'''SELECT Ref.fubar.x''',
+            [],
+        )
+
+    async def test_edgeql_ddl_alter_union_02(self):
+        await self.con.execute(r"""
+            SET MODULE test;
+            CREATE TYPE Foo { CREATE PROPERTY x -> str; };
+            CREATE TYPE Bar { CREATE PROPERTY x -> str; };
+            CREATE TYPE Baz { CREATE PROPERTY x -> str; };
+        """)
+
+        await self.con.execute(r"""
+            CREATE TYPE Ref {
+                CREATE LINK everything -> Foo | Bar | Baz;
+                CREATE LINK fubar -> Foo | Bar;
+                CREATE LINK barbaz -> Bar | Baz;
+            }
+        """)
+
+        await self.con.execute(r"""
+            ALTER TYPE Baz DROP PROPERTY x;
+        """)
+
+        await self.assert_query_result(
+            r'''SELECT Ref.fubar.x''',
+            [],
+        )
+
+        await self.con.execute(r"""
+            ALTER TYPE Baz CREATE PROPERTY x -> str;
+        """)
+
+        await self.assert_query_result(
+            r'''SELECT Ref.everything.x''',
+            [],
+        )
+
+    async def test_edgeql_ddl_alter_union_03(self):
+        await self.con.execute(r"""
+            SET MODULE test;
+            CREATE TYPE Parent;
+            CREATE TYPE Child EXTENDING Parent {
+                CREATE PROPERTY prop -> str;
+            };
+            CREATE TYPE Foo {CREATE LINK y -> Child};
+            CREATE TYPE Bar {CREATE LINK y -> Child};
+        """)
+
+        await self.con.execute(r"""
+            CREATE TYPE Ref {
+                CREATE LINK fubar -> Foo | Bar;
+            }
+        """)
+
+        await self.con.execute(r"""
+            ALTER TYPE Foo ALTER LINK y SET TYPE Parent;
+        """)
+
+        async with self.assertRaisesRegexTx(
+            edgedb.QueryError,
+            "object type 'test::Parent' has no link or property 'prop'",
+        ):
+            await self.assert_query_result(
+                r'''SELECT Ref.fubar.y.prop''',
+                [],
+            )
+
 
 class TestConsecutiveMigrations(tb.DDLTestCase):
     TRANSACTION_ISOLATION = False
diff --git a/tests/test_schema.py b/tests/test_schema.py
index 2438fd1a703..edbc1d4fd8f 100644
--- a/tests/test_schema.py
+++ b/tests/test_schema.py
@@ -5595,6 +5595,23 @@ def test_schema_migrations_extend_enum_01(self):
             scalar type foo extending enum<Foo, Bar, Baz>;
         """])
 
+    def test_schema_migrations_union_01(self):
+        self._assert_migration_equivalence([r"""
+            type Category {
+                required property title -> str;
+                required property deleted :=
+                    EXISTS(.<element[IS DeletionRecord]);
+            };
+            type Article {
+                required property title -> str;
+                required property deleted :=
+                    EXISTS(.<element[IS DeletionRecord]);
+            };
+            type DeletionRecord {
+                link element -> Article | Category;
+            }
+        """])
+
     def test_schema_migrations_drop_depended_on_parent_01(self):
         self._assert_migration_equivalence([r"""
             type Person2 {


Code review comments:
diff_hunk: @@ -1628,6 +1628,30 @@ def as_inherited_ref_cmd(
         return cmd
 
 
+class DeletePointer(
+    referencing.DeleteReferencedInheritingObject[Pointer_T],
+    PointerCommand[Pointer_T],
+):
+    def _canonicalize(
+        self,
+        schema: s_schema.Schema,
+        context: sd.CommandContext,
+        scls: Pointer_T,
+    ) -> List[sd.Command]:
+        commands = super()._canonicalize(schema, context, scls)
+
+        # Any union type that references this field needs to have it
+        # deleted.
+        unions = schema.get_referrers(
+            self.scls, scls_type=Pointer, field_name='union_of')
+        for union in unions:
+            group, _, _ = union.init_delta_branch(
path: edb/schema/pointers.py
text: @user1:
I think you need to run `_canonicalize` on `self_cmd` returned by `init_delta_branch`.