Skip to content

Commit

Permalink
Fix some alter+rename combos on pointers in POPULATE MIGRATION
Browse files Browse the repository at this point in the history
Doing multiple operations on a pointer and renaming it at the same
time in a migration created with POPULATE MIGRATION sometimes fails,
because we weren't properly applying renames when generating the AST.

This *didn't* happen when using the normal migration flow, because the
renames get processed incrementally there.

I think I introduced this as a regression in #4007.

I also fixed an issue where in a CREATE MIGRATION script, you could
refer to the old name of something in a later command. The reason I
dealt with that here is because it was preventing schema tests from
finding the bug here, since in the schema test the migration script
was only checked by applying it with CREATE MIGRATION.
(In the actual server, POPULATE MIGRATION explicitly checks
the commands validity in a way not afected by this, which is
where these failures were happening.)

Because of those explicit validity checks, I'm confident that there
aren't auto-generated migrations out there that rely on this bug.

Fixes #6664.
  • Loading branch information
msullivan committed Jan 5, 2024
1 parent 98f67c8 commit 39130b0
Show file tree
Hide file tree
Showing 4 changed files with 157 additions and 2 deletions.
11 changes: 9 additions & 2 deletions edb/schema/delta.py
Original file line number Diff line number Diff line change
Expand Up @@ -2316,8 +2316,15 @@ def _get_ast(
astnode = self._get_ast_node(schema, context)

if astnode.get_field('name'):
name = sn.shortname_from_fullname(self.classname)
name = context.early_renames.get(name, name)
# We need to be able to catch both renames of the object
# itself, which might have a long name (for pointers, for
# example) as well as an object being referenced by
# shortname, if this is (for example) a concrete
# constraint and the abstract constraint was renamed.
name = context.early_renames.get(self.classname, self.classname)
name = sn.shortname_from_fullname(name)
if self.classname not in context.early_renames:
name = context.early_renames.get(name, name)
op = astnode( # type: ignore
name=self._deparse_name(schema, context, name),
)
Expand Down
17 changes: 17 additions & 0 deletions edb/schema/migrations.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,23 @@ def _cmd_tree_from_ast(

return cmd

def apply_subcommands(
self,
schema: s_schema.Schema,
context: sd.CommandContext,
) -> s_schema.Schema:
assert not self.get_prerequisites() and not self.get_caused()
# Renames shouldn't persist between commands in a migration script.
context.renames.clear()
for op in self.get_subcommands(
include_prerequisites=False,
include_caused=False,
):
if not isinstance(op, sd.AlterObjectProperty):
schema = op.apply(schema, context=context)
context.renames.clear()
return schema

def _get_ast(
self,
schema: s_schema.Schema,
Expand Down
19 changes: 19 additions & 0 deletions tests/test_edgeql_ddl.py
Original file line number Diff line number Diff line change
Expand Up @@ -13085,6 +13085,25 @@ async def test_edgeql_ddl_create_migration_04(self):
# ['test'],
# )

async def test_edgeql_ddl_create_migration_05(self):
await self.con.execute('''
create type X { create property x -> str; };
''')
async with self.assertRaisesRegexTx(
edgedb.InvalidReferenceError,
"property 'x' does not"):
await self.con.execute('''
CREATE MIGRATION
{
alter type default::X {
alter property x rename to y;
};
alter type default::X {
alter property x create constraint exclusive;
};
};
''')

async def test_edgeql_ddl_naked_backlink_in_computable(self):
await self.con.execute('''
CREATE TYPE User {
Expand Down
112 changes: 112 additions & 0 deletions tests/test_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -8190,6 +8190,118 @@ def test_schema_migrations_rename_with_stuff_04(self):
"""
])

def test_schema_migrations_rename_and_modify_01(self):
self._assert_migration_equivalence([
r"""
type Branch{
property branchURL: std::str {
constraint max_len_value(500);
constraint min_len_value(5);
};
};
""",
r"""
type Branch{
property email: std::str {
constraint max_len_value(50);
constraint min_len_value(5);
};
};
"""
])

def test_schema_migrations_rename_and_modify_02(self):
self._assert_migration_equivalence([
r"""
type X {
obj: Object {
foo: str;
};
};
""",
r"""
type X {
obj2: Object {
bar: int64;
};
};
"""
])

def test_schema_migrations_rename_and_modify_03(self):
self._assert_migration_equivalence([
r"""
type Branch{
property branchName: std::str {
constraint min_len_value(0);
constraint max_len_value(255);
};
property branchCode: std::int64;
property branchURL: std::str {
constraint max_len_value(500);
constraint regexp("url");
constraint min_len_value(5);
};
};
""",
r"""
type Branch{
property branchName: std::str {
constraint min_len_value(0);
constraint max_len_value(255);
};
property branchCode: std::int64;
property phoneNumber: std::str {
constraint min_len_value(5);
constraint max_len_value(50);
constraint regexp(r"phone");
};
property email: std::str {
constraint min_len_value(5);
constraint max_len_value(50);
constraint regexp(r"email");
};
};
"""
])

def test_schema_migrations_rename_and_modify_04(self):
self._assert_migration_equivalence([
r"""
type Branch{
property branchName: std::str {
constraint min_len_value(0);
constraint max_len_value(255);
};
property branchCode: std::int64;
property branchURL: std::str {
constraint max_len_value(500);
constraint regexp("url");
constraint min_len_value(5);
};
};
""",
r"""
type Branch2 {
property branchName: std::str {
constraint min_len_value(0);
constraint max_len_value(255);
};
property branchCode: std::int64;
property phoneNumber: std::str {
constraint min_len_value(5);
constraint max_len_value(50);
constraint regexp(r"phone");
};
property email: std::str {
constraint min_len_value(5);
constraint max_len_value(50);
constraint regexp(r"email");
};
};
"""
])

def test_schema_migrations_except_01(self):
self._assert_migration_equivalence([
r"""
Expand Down

0 comments on commit 39130b0

Please sign in to comment.