Repository: geldata/gel
Pull request: Fix migration prompting while adding required pointers via parent types
The key idea is that we force a `CREATE PROPERTY ... { SET REQUIRED  ... }`
to be generated when appropriate even if we would normally elide it.

Forcing this to happen exposed a funny corner case in which the
diffing mechanism sometimes generates DELETE/CREATE pairs for the
__type__ link when a type is renamed. This might be worth dealing with
more robustly, but I worked around this for now by having the compiler
regenerate the proposed plan after applying DDL that does a rename.
This turns out to have been a good idea in general, since it fixed
issues where we would generate prompts that referred to old names
instead of new ones.

Fixes #2486.

Problem statement:
Ask for default when adding new required property
<!-- Please search existing issues to avoid creating duplicates. -->
- EdgeDB Version: 1-beta.1
- EdgeDB CLI Version: beta.1
- OS Version: macOS Big Sur

Steps to Reproduce:

I made a minimal repro of how I got my schema into an inconsistent state.

1. Create an initial schema

```
module default {
    type BlogPost {
        property title -> str;
    }
};
```

2. Add some data;

```
INSERT BlogPost;
```

3. Update
```
module default {
    type BlogPost {
        property title -> str;
        required property name -> str;
    }
};
```

4. Run `create-migration` (works fine). If you make an existing non-required property required, the migration tool prompts you for a default, but not if you define a new required property.

5. Inconsistent state:

Running migrate:

```
$ edgedb -I inconsistent migrate
edgedb error: ERROR: MissingRequiredError: missing value for required property 'name' of object type 'default::BlogPost'
```

Runing create-migration (e.g. after setting a default for .content):

```
$ edgedb -I inconsistent create-migration
edgedb error: Database must be updated to the last migration on the filesystem for `create-migration`. Run:
  edgedb migrate
```

Hints from issue discussion:
I have fixes for the updated test case where we add a new required property, but frustratingly, the *original* issue of adding a property via a new parent class might be a bit trickier. I'm going to spend a couple hours looking at it, but I might put up a partial fix without handling that case yes.

Commit message: Fix migration prompting while adding required pointers via parent types

The key idea is that we force a `CREATE PROPERTY ... { SET REQUIRED  ... }`
to be generated when appropriate even if we would normally elide it.

Forcing this to happen exposed a funny corner case in which the
diffing mechanism sometimes generates DELETE/CREATE pairs for the
__type__ link when a type is renamed. This might be worth dealing with
more robustly, but I worked around this for now by having the compiler
regenerate the proposed plan after applying DDL that does a rename.
This turns out to have been a good idea in general, since it fixed
issues where we would generate prompts that referred to old names
instead of new ones.

Fixes #2486.

Patch to review:
diff --git a/edb/schema/delta.py b/edb/schema/delta.py
index f42c5e5263e..f1b0baebca6 100644
--- a/edb/schema/delta.py
+++ b/edb/schema/delta.py
@@ -2163,6 +2163,7 @@ def _apply_fields_ast(
                         # and that have their value actually changed.
                         not fop.new_inherited
                         or context.descriptive_mode
+                        or self.ast_ignore_ownership()
                     )
                     and (
                         fop.old_value != new_value
@@ -2605,6 +2606,10 @@ def set_annotation(self, name: str, value: Any) -> None:
             self.annotations = {}
         self.annotations[name] = value
 
+    def ast_ignore_ownership(self) -> bool:
+        """Whether to force something into the AST even though it is owned"""
+        return False
+
 
 class ObjectCommandContext(CommandContextToken[ObjectCommand[so.Object_T]]):
 
diff --git a/edb/schema/ordering.py b/edb/schema/ordering.py
index 191dea4850f..0738807cdcc 100644
--- a/edb/schema/ordering.py
+++ b/edb/schema/ordering.py
@@ -770,7 +770,11 @@ def write_dep_matrix(
                     referrer_name = renames_r[referrer_name]
                 ref_name_str = str(referrer_name)
                 deps.add(('create', ref_name_str))
-                deps.add(('rebase', ref_name_str))
+                if op.ast_ignore_ownership():
+                    ref_item = get_deps(('rebase', ref_name_str))
+                    ref_item.deps.add((tag, this_name_str))
+                else:
+                    deps.add(('rebase', ref_name_str))
 
                 if isinstance(obj, referencing.ReferencedInheritingObject):
                     implicit_ancestors = [
diff --git a/edb/schema/pointers.py b/edb/schema/pointers.py
index ffaec9f54d1..3cecd51c8cf 100644
--- a/edb/schema/pointers.py
+++ b/edb/schema/pointers.py
@@ -1589,6 +1589,13 @@ class CreatePointer(
     PointerCommand[Pointer_T],
 ):
 
+    def ast_ignore_ownership(self) -> bool:
+        # If we have a SET REQUIRED with a fill_expr, we need to force
+        # this operation to appear in the AST in a useful position,
+        # even if it normally would be skipped.
+        subs = list(self.get_subcommands(type=AlterPointerLowerCardinality))
+        return len(subs) == 1 and bool(subs[0].fill_expr)
+
     @classmethod
     def as_inherited_ref_cmd(
         cls,
@@ -2272,7 +2279,6 @@ def _needs_fill_expr(
         return (
             not old_required and new_required
             and not self.is_attribute_computed('required')
-            and not self.is_attribute_inherited('required')
             and not ptr_op.maybe_get_object_aux_data('from_alias')
             and self.fill_expr is None
             and not (
diff --git a/edb/schema/referencing.py b/edb/schema/referencing.py
index 28a77452b08..5975ccf104b 100644
--- a/edb/schema/referencing.py
+++ b/edb/schema/referencing.py
@@ -587,7 +587,11 @@ def _get_ast_node(
         scls = self.get_object(schema, context)
         assert isinstance(scls, ReferencedInheritingObject)
         implicit_bases = scls.get_implicit_bases(schema)
-        if implicit_bases and not context.declarative:
+        if (
+            implicit_bases
+            and not context.declarative
+            and not self.ast_ignore_ownership()
+        ):
             alter = scls.init_delta_command(schema, sd.AlterObject)
             return alter._get_ast_node(schema, context)
         else:
@@ -993,7 +997,10 @@ def _get_ast(
             if self.get_attribute_value('from_alias'):
                 return None
 
-            elif not self.get_attribute_value('owned'):
+            elif (
+                not self.get_attribute_value('owned')
+                and not self.ast_ignore_ownership()
+            ):
                 if context.descriptive_mode:
                     astnode = super()._get_ast(
                         schema,
diff --git a/edb/server/compiler/compiler.py b/edb/server/compiler/compiler.py
index 0f4708a7659..47de0602799 100644
--- a/edb/server/compiler/compiler.py
+++ b/edb/server/compiler/compiler.py
@@ -764,9 +764,14 @@ def _compile_and_apply_ddl_stmt(
             schema = delta.apply(schema, context=context)
 
             if mstate.last_proposed:
-                if mstate.last_proposed[0].required_user_input:
+                if (
+                    mstate.last_proposed[0].required_user_input
+                    or mstate.last_proposed[0].prompt_id.startswith("Rename")
+                ):
                     # Cannot auto-apply the proposed DDL
                     # if user input is required.
+                    # Also skip auto-applying for renames, since
+                    # renames often force a bunch of rethinking.
                     mstate = mstate._replace(last_proposed=tuple())
                 else:
                     proposed_stmts = mstate.last_proposed[0].statements
diff --git a/tests/test_edgeql_data_migration.py b/tests/test_edgeql_data_migration.py
index f276c9be9eb..6e3e4ca01a1 100644
--- a/tests/test_edgeql_data_migration.py
+++ b/tests/test_edgeql_data_migration.py
@@ -205,8 +205,7 @@ async def interact(self, parts, check_complete=True):
                 ans = "y"
                 user_input = None
             else:
-                prompt, ans = part[0:2]
-                user_input = part[2] if len(part) > 2 else None
+                prompt, ans, *user_input = part
 
             await self.assert_describe_migration({
                 'proposed': {'prompt': prompt}
@@ -4928,10 +4927,8 @@ async def test_edgeql_migration_rebase_03(self):
             "did you rename object type 'test::OrgUniquelyNamed' to "
             "'test::OrgUniquelyNamedResource'?",
 
-            "did you alter constraint 'std::exclusive' of object type "
-            "'test::OrgUniquelyNamed'?",
-
-            "did you alter object type 'test::OrgUniquelyNamed'?",
+            "did you alter object type 'test::OrgUniquelyNamedResource'?",
+            "did you alter object type 'test::OrgUniquelyNamedResource'?",
         ])
 
     async def test_edgeql_migration_rename_01(self):
@@ -4947,7 +4944,7 @@ async def test_edgeql_migration_rename_01(self):
 
         await self.interact([
             "did you rename object type 'test::Foo' to 'test::Bar'?",
-            "did you create property 'asdf' of object type 'test::Foo'?",
+            "did you create property 'asdf' of object type 'test::Bar'?",
         ])
 
     async def test_edgeql_migration_rename_02(self):
@@ -4975,7 +4972,7 @@ async def test_edgeql_migration_rename_02(self):
             "did you rename property 'asdf' of object type 'test::Foo' to "
             "'womp'?" ,
 
-            "did you create annotation 'std::title' of property 'asdf'?",
+            "did you create annotation 'std::title' of property 'womp'?",
         ])
 
     async def test_edgeql_migration_rename_03(self):
@@ -8709,12 +8706,6 @@ async def test_edgeql_migration_user_input_03(self):
             ],
         )
 
-    @test.xfail('''
-        We still don't get it right for rebase situations though. Argh!
-
-        This test shouldn't be unxfailed directly when made to pass, though:
-        it should be extended to finish the migration.
-    ''')
     async def test_edgeql_migration_user_input_04(self):
         await self.migrate('''
             type BlogPost {
@@ -8726,18 +8717,39 @@ async def test_edgeql_migration_user_input_04(self):
             INSERT BlogPost { title := "Programming Considered Harmful" }
         ''')
 
-        # await self.start_migration('''
-        with self.assertRaisesRegex(
-                AssertionError,
-                'Please specify an expression'):
-            await self.migrate('''
-                abstract type HasContent {
-                    required property content -> str;
-                }
-                type BlogPost extending HasContent {
-                    property title -> str;
-                }
-            ''')
+        await self.start_migration('''
+            abstract type HasContent {
+                required property content -> str;
+            }
+            type BlogPost extending HasContent {
+                property title -> str;
+            }
+        ''')
+
+        await self.interact([
+            "did you create object type 'test::HasContent'?",
+            ("did you alter object type 'test::BlogPost'?", "y",
+             '"This page intentionally left blank"'),
+            # XXX: There is a final follow-up prompt, since the DDL
+            # generated above somewhat wrongly leaves 'content' owned
+            # by the child. This is kind of wrong, but also *works*, so
+            # maybe it's fine for now.
+            "did you alter property 'content' of object type "
+            "'test::BlogPost'?",
+        ])
+        await self.fast_forward_describe_migration()
+
+        await self.assert_query_result(
+            '''
+                SELECT BlogPost {title, content}
+            ''',
+            [
+                {
+                    'title': "Programming Considered Harmful",
+                    'content': "This page intentionally left blank",
+                },
+            ],
+        )
 
     async def test_edgeql_migration_misplaced_commands(self):
         async with self.assertRaisesRegexTx(


Code review comments:
diff_hunk: @@ -2605,6 +2606,10 @@ def set_annotation(self, name: str, value: Any) -> None:
             self.annotations = {}
         self.annotations[name] = value
 
+    def ast_ignore_ownership(self) -> bool:
+        """Whether to force something into the AST even though it is owned"""
path: edb/schema/delta.py
text: @user1:
Did you mean "not owned" here?