Fix migration failure if a DML function refers to required property.

edb/schema/delta.py

@@ -58,6 +58,7 @@
 from edb.common import parsing
 from edb.common import struct
 from edb.common import topological
+from edb.common import typeutils
 from edb.common import typing_inspect
 from edb.common import verutils
 
@@ -217,6 +218,9 @@ def can_delete(obj: so.Object_T, name: sn.Name) -> bool:
     alters = []
     alter_pairs = []
 
+    # Alters which need to be processed as a paired delete/create
+    paired_creates: dict[so.Object, sn.Name] = {}
+
     if comparison_map:
         if issubclass(sclass, so.InheritingObject):
             # Generate the diff from the top of the inheritance
@@ -246,6 +250,44 @@ def can_delete(obj: so.Object_T, name: sn.Name) -> bool:
                 )
                 or x_name in renames_x
             ):
+                from . import functions as s_func
+
+                if (
+                    isinstance(x, s_func.Function)
+                    and isinstance(y, s_func.Function)
+                ):
+                    # Treat altering a DML function as a paired delete/create
+                    # See CreateObject.paired_delete for more details.
+
+                    from edb.edgeql.compiler import astutils as ql_astutils
+                    from edb.edgeql.compiler import stmtctx as ql_stmtctx
+
+                    new_nativecode = typeutils.not_none(
+                        x.get_nativecode(new_schema)
+                    )
+                    old_nativecode = typeutils.not_none(
+                        y.get_nativecode(old_schema)
+                    )
+
+                    new_contains_dml = ql_astutils.contains_dml(
+                        new_nativecode.parse(),
+                        ctx=ql_stmtctx.init_context(
+                            schema=new_schema,
+                            options=qlcompiler.CompilerOptions(),
+                        )
+                    )
+                    old_contains_dml = ql_astutils.contains_dml(
+                        old_nativecode.parse(),
+                        ctx=ql_stmtctx.init_context(
+                            schema=old_schema,
+                            options=qlcompiler.CompilerOptions(),
+                        )
+                    )
+
+                    if new_contains_dml or old_contains_dml:
+                        paired_creates[x] = y_name
+                        continue
+
                 alter_pairs.append((x, y))
 
                 alter = y.as_alter_delta(
@@ -287,6 +329,8 @@ def can_delete(obj: so.Object_T, name: sn.Name) -> bool:
             else:
                 confidence = 1.0
             create.set_annotation('confidence', confidence)
+            if x in paired_creates:
+                create.paired_delete = paired_creates[x]
             delta.add(create)
 
     delta.update(alters)
@@ -3017,6 +3061,34 @@ class CreateObject(ObjectCommand[so.Object_T], Generic[so.Object_T]):
     # If the command is conditioned with IF NOT EXISTS
     if_not_exists = struct.Field(bool, default=False)
 
+    # A paired CreateObject depends on all commands which depend on its paired
+    # DeleteObject.
+    #
+    # Given the following schema:
+    #   type Foo { required a: int64 }
+    #   function insert_foo() using ( insert Foo { a := 1 } )
+    #
+    # being migrated to:
+    #   type Foo {}
+    #   function insert_foo() using ( insert Foo {} )
+    #
+    # Suppose this is done using 2 commands:
+    #   Command A: delete property `Foo.a`
+    #   Command B: alter function `insert_foo`
+    #
+    # If Command A is applied first, then it is invalid because it attempts to
+    # insert `Foo` with an unknown property `a`. If Command B is applied first,
+    # then that is also invalid since required property `a` is missing.
+    #
+    # To resolve this, the alter function is split into two commands, thus:
+    #   Command A: delete property `Foo.a`
+    #   Command B: delete function `insert_foo`
+    #   Command C: create function `insert_foo`
+    #
+    # In linearize_delta, to ensure that the commands are ordered [B, A, C] and
+    # not [B, C, A], Command C is made to depend on Command A.
+    paired_delete = struct.Field(sn.Name, default=None)
+
     def is_data_safe(self) -> bool:
         # Creations are always data-safe.
         return True

edb/schema/ordering.py

@@ -98,6 +98,15 @@ def linearize_delta(
     for op in _get_sorted_subcommands(delta):
         _break_down(opmap, strongrefs, [delta, op])
 
+    paired_create_deletes: dict[sn.Name, sn.Name] = {
+        create_op.classname: create_op.paired_delete
+        for create_op in opmap
+        if (
+            isinstance(create_op, sd.CreateObject)
+            and create_op.paired_delete is not None
+        )
+    }
+
     depgraph: DepGraph = {}
     renames: Dict[sn.Name, sn.Name] = {}
     renames_r: Dict[sn.Name, sn.Name] = {}
@@ -114,8 +123,33 @@ def linearize_delta(
         if isinstance(op, sd.AlterObject) and not op.get_subcommands():
             continue
 
-        _trace_op(op, opbranch, depgraph, renames,
-                  renames_r, strongrefs, old_schema, new_schema)
+        _trace_op(
+            op,
+            opbranch,
+            depgraph,
+            renames,
+            renames_r,
+            strongrefs,
+            old_schema,
+            new_schema,
+            paired_create_deletes,
+        )
+
+    # Ensure that intermediate commands happen between paired delete/create.
+    # If a command depends on the delete, make create depend on it in turn.
+    for create_name, delete_name in paired_create_deletes.items():
+
+        create_key = ('create', str(create_name))
+        delete_key = ('delete', str(delete_name))
+
+        create_dep_entry = depgraph[create_key]
+
+        for other_key, other_entry in depgraph.items():
+            if other_key == create_key:
+                continue
+
+            if delete_key in other_entry.deps:
+                create_dep_entry.deps.add(other_key)
 
     depgraph = dict(filter(lambda i: i[1].item != (), depgraph.items()))
     everything = set(depgraph)
@@ -509,6 +543,7 @@ def _trace_op(
     strongrefs: Dict[sn.Name, sn.Name],
     old_schema: Optional[s_schema.Schema],
     new_schema: s_schema.Schema,
+    paired_create_deletes: dict[sn.Name, sn.Name],
 ) -> None:
     def get_deps(key: DepGraphKey) -> DepGraphEntry:
         try:
@@ -832,6 +867,17 @@ def write_ref_deps(
             for ref in _get_referrers(old_schema, old_obj, strongrefs):
                 deps.add(('delete', str(ref.get_name(old_schema))))
 
+        if tag == 'alter':
+            # If there is a paired create/delete which refers something being
+            # altered, make sure that that something happens between the delete
+            # and create.
+            assert old_schema
+            old_obj = get_object(old_schema, op, op.classname)
+            for ref in _get_referrers(old_schema, old_obj, strongrefs):
+                ref_name = ref.get_name(old_schema)
+                if ref_name in paired_create_deletes.values():
+                    deps.add(('delete', str(ref_name)))
+
         refs = _get_referrers(new_schema, obj, strongrefs)
         for ref in refs:
             write_ref_deps(ref, obj, this_name_str)