diff --git a/piccolo/apps/migrations/auto/diffable_table.py b/piccolo/apps/migrations/auto/diffable_table.py index 55d88cb64..f52b4c5b9 100644 --- a/piccolo/apps/migrations/auto/diffable_table.py +++ b/piccolo/apps/migrations/auto/diffable_table.py @@ -266,10 +266,7 @@ def __sub__(self, value: DiffableTable) -> TableDelta: ) def __hash__(self) -> int: - """ - We have to return an integer, which is why convert the string this way. - """ - return hash(self.class_name + self.tablename) + return hash((self.class_name, self.tablename, self.schema)) def __eq__(self, value) -> bool: """ @@ -277,8 +274,10 @@ def __eq__(self, value) -> bool: """ if not isinstance(value, DiffableTable): return False - return (self.class_name == value.class_name) and ( - self.tablename == value.tablename + return ( + (self.class_name == value.class_name) + and (self.tablename == value.tablename) + and (self.schema == value.schema) ) def __str__(self): diff --git a/piccolo/apps/migrations/auto/schema_differ.py b/piccolo/apps/migrations/auto/schema_differ.py index 58543cbca..b8c49d0e0 100644 --- a/piccolo/apps/migrations/auto/schema_differ.py +++ b/piccolo/apps/migrations/auto/schema_differ.py @@ -66,6 +66,10 @@ class ChangeTableSchemaCollection: def append(self, change_table_schema: ChangeTableSchema): self.collection.append(change_table_schema) + @property + def class_names(self) -> list[str]: + return [i.class_name for i in self.collection] + @dataclass class RenameColumnCollection: @@ -138,13 +142,22 @@ def check_rename_tables(self) -> RenameTableCollection: """ Work out whether any of the tables were renamed. """ - drop_tables: list[DiffableTable] = list( - set(self.schema_snapshot) - set(self.schema) - ) + # Tables which just had their schema changed are handled separately + # by ``check_table_schema_changes`` - excluded here so a schema + # change alone doesn't get misdetected as a same-named rename. + drop_tables: list[DiffableTable] = [ + i + for i in set(self.schema_snapshot) - set(self.schema) + if i.class_name + not in self.table_schema_changes_collection.class_names + ] - new_tables: list[DiffableTable] = list( - set(self.schema) - set(self.schema_snapshot) - ) + new_tables: list[DiffableTable] = [ + i + for i in set(self.schema) - set(self.schema_snapshot) + if i.class_name + not in self.table_schema_changes_collection.class_names + ] # A mapping of the old table name (i.e. dropped table) to the new # table name. @@ -353,12 +366,16 @@ def create_tables(self) -> AlterStatements: set(self.schema) - set(self.schema_snapshot) ) - # Remove any which are renames + # Remove any which are renames, or just had their schema changed + # (both are handled separately, and only match on class_name, so + # they're unaffected by the schema-aware equality check above). new_tables = [ i for i in new_tables if i.class_name not in self.rename_tables_collection.new_class_names + and i.class_name + not in self.table_schema_changes_collection.class_names ] alter_statements = AlterStatements() @@ -384,12 +401,16 @@ def drop_tables(self) -> AlterStatements: set(self.schema_snapshot) - set(self.schema) ) - # Remove any which are renames + # Remove any which are renames, or just had their schema changed + # (both are handled separately, and only match on class_name, so + # they're unaffected by the schema-aware equality check above). drop_tables = [ i for i in drop_tables if i.class_name not in self.rename_tables_collection.old_class_names + and i.class_name + not in self.table_schema_changes_collection.class_names ] alter_statements = AlterStatements() diff --git a/tests/apps/migrations/auto/test_schema_differ.py b/tests/apps/migrations/auto/test_schema_differ.py index 64a1f2d67..9b5321da5 100644 --- a/tests/apps/migrations/auto/test_schema_differ.py +++ b/tests/apps/migrations/auto/test_schema_differ.py @@ -101,11 +101,14 @@ def test_change_schema(self) -> None: """ Testing changing the schema. """ + name_column = Varchar() + name_column._meta.name = "name" + schema: list[DiffableTable] = [ DiffableTable( class_name="Band", tablename="band", - columns=[], + columns=[name_column], schema="schema_1", ) ] @@ -113,7 +116,7 @@ def test_change_schema(self) -> None: DiffableTable( class_name="Band", tablename="band", - columns=[], + columns=[name_column], schema=None, ) ] @@ -132,6 +135,42 @@ def test_change_schema(self) -> None: self.assertListEqual(schema_differ.create_tables.statements, []) self.assertListEqual(schema_differ.drop_tables.statements, []) + # A schema change alone shouldn't be misdetected as a table rename + # (this table has overlapping columns, which is what the rename + # heuristic keys off). + self.assertListEqual(schema_differ.rename_tables.statements, []) + + def test_same_table_name_different_schema(self) -> None: + """ + Two tables with the same class_name / tablename, but in different + schemas, are different tables - both should be created. + + https://github.com/piccolo-orm/piccolo/issues/1426 + """ + schema: list[DiffableTable] = [ + DiffableTable( + class_name="Band", tablename="band", schema="schema_1" + ), + DiffableTable( + class_name="Band", tablename="band", schema="schema_2" + ), + ] + schema_snapshot: list[DiffableTable] = [] + + schema_differ = SchemaDiffer( + schema=schema, schema_snapshot=schema_snapshot, auto_input="y" + ) + + create_tables = schema_differ.create_tables + self.assertEqual(len(create_tables.statements), 2) + self.assertEqual( + set(create_tables.statements), + { + "manager.add_table(class_name='Band', tablename='band', schema='schema_1', columns=None)", # noqa: E501 + "manager.add_table(class_name='Band', tablename='band', schema='schema_2', columns=None)", # noqa: E501 + }, + ) + def test_add_column(self) -> None: """ Test adding a column to an existing table.