Repository: piccolo-orm/piccolo
Issue: Rename column causes persistent rename column prompt in migration manager

If you rename a column in a Table and auto generate a migration you will be asked if you renamed a column. Answering "Y" and running the migration gives you the correct behavior. However, on subsequent migrations you are asked about the exact same column again. Also note that when renaming multiple columns answering "N" to the first prompt stops further ones. Additionally, The name pairings in the prompt aren't matched up properly.

This can be demonstrated using the basic asgi scaffold.

To get started...
```console
> piccolo asgi new
> piccolo migrations new home --auto
> piccolo migrations forwards home
```

- Then go to `./home/tables.py` and change the `completed` columns to `success` or anything other name.
- Another migration which will run fine
```console
> piccolo migrations new home --auto
Creating new migration ...
Did you rename the `completed` column to `success` on the `Task` table? (y/N)
> y
> piccolo migrations forwards home
```
Now go update the table again. I just added a `completed_at = Timestamptz` for ease.
Migrations again which produces and identical prompting about the previous rename.
```console
> piccolo migrations new home --auto
Creating new migration ...
Did you rename the `completed` column to `success` on the `Task` table? (y/N)
> n
> piccolo migrations forwards home
```
This results in an add_column/drop_column in the latest migration which is problematic.


Changed files:
tests/apps/migrations/auto/test_schema_differ.py, piccolo/apps/migrations/auto/diffable_table.py, piccolo/apps/migrations/auto/schema_differ.py

diff --git a/piccolo/apps/migrations/auto/diffable_table.py b/piccolo/apps/migrations/auto/diffable_table.py
index 318edfd2..e73065ed 100644
--- a/piccolo/apps/migrations/auto/diffable_table.py
+++ b/piccolo/apps/migrations/auto/diffable_table.py
@@ -109,6 +109,12 @@ class DiffableTable:
                 "The two tables don't appear to have the same name."
             )
 
+        #######################################################################
+
+        # Because we're using sets here, the order is indeterminate. We sort
+        # them, otherwise it's difficult to write good unit tests if the order
+        # constantly changes.
+
         add_columns = [
             AddColumn(
                 table_class_name=self.class_name,
@@ -118,9 +124,12 @@ class DiffableTable:
                 column_class=i.column.__class__,
                 params=i.column._meta.params,
             )
-            for i in (
+            for i in sorted(
                 {ColumnComparison(column=column) for column in self.columns}
-                - {ColumnComparison(column=column) for column in value.columns}
+                - {
+                    ColumnComparison(column=column) for column in value.columns
+                },
+                key=lambda x: x.column._meta.name,
             )
         ]
 
@@ -131,12 +140,15 @@ class DiffableTable:
                 db_column_name=i.column._meta.db_column_name,
                 tablename=value.tablename,
             )
-            for i in (
+            for i in sorted(
                 {ColumnComparison(column=column) for column in value.columns}
-                - {ColumnComparison(column=column) for column in self.columns}
+                - {ColumnComparison(column=column) for column in self.columns},
+                key=lambda x: x.column._meta.name,
             )
         ]
 
+        #######################################################################
+
         alter_columns: t.List[AlterColumn] = []
 
         for existing_column in value.columns:
diff --git a/piccolo/apps/migrations/auto/schema_differ.py b/piccolo/apps/migrations/auto/schema_differ.py
index 54fd1015..71c4cc50 100644
--- a/piccolo/apps/migrations/auto/schema_differ.py
+++ b/piccolo/apps/migrations/auto/schema_differ.py
@@ -200,13 +200,17 @@ class SchemaDiffer:
             # type. For now, each time a column is added and removed from a
             # table, ask if it's a rename.
 
-            renamed_column_names: t.List[str] = []
+            # We track which dropped columns have already been identified by
+            # the user as renames, so we don't ask them if another column
+            # was also renamed from it.
+            used_drop_column_names: t.List[str] = []
 
             for add_column in delta.add_columns:
-                if add_column.table_class_name in renamed_column_names:
-                    continue
 
                 for drop_column in delta.drop_columns:
+                    if drop_column.column_name in used_drop_column_names:
+                        continue
+
                     user_response = (
                         self.auto_input
                         if self.auto_input
@@ -217,9 +221,7 @@ class SchemaDiffer:
                         )
                     )
                     if user_response.lower() == "y":
-                        renamed_column_names.append(
-                            add_column.table_class_name
-                        )
+                        used_drop_column_names.append(drop_column.column_name)
                         collection.append(
                             RenameColumn(
                                 table_class_name=add_column.table_class_name,
@@ -230,6 +232,7 @@ class SchemaDiffer:
                                 new_db_column_name=add_column.db_column_name,
                             )
                         )
+                        break
 
         return collection
 
diff --git a/tests/apps/migrations/auto/test_schema_differ.py b/tests/apps/migrations/auto/test_schema_differ.py
index 35e4a6a1..6fccff2e 100644
--- a/tests/apps/migrations/auto/test_schema_differ.py
+++ b/tests/apps/migrations/auto/test_schema_differ.py
@@ -2,6 +2,7 @@ from __future__ import annotations
 
 import typing as t
 from unittest import TestCase
+from unittest.mock import MagicMock, call, patch
 
 from piccolo.apps.migrations.auto import DiffableTable, SchemaDiffer
 from piccolo.columns.column_types import Numeric, Varchar
@@ -164,12 +165,20 @@ class TestSchemaDiffer(TestCase):
         """
         Test renaming a column in an existing table.
         """
+        # We're going to rename the 'name' column to 'title'
         name_column = Varchar()
         name_column._meta.name = "name"
 
         title_column = Varchar()
         title_column._meta.name = "title"
 
+        schema_snapshot: t.List[DiffableTable] = [
+            DiffableTable(
+                class_name="Band",
+                tablename="band",
+                columns=[title_column],
+            )
+        ]
         schema: t.List[DiffableTable] = [
             DiffableTable(
                 class_name="Band",
@@ -177,22 +186,195 @@ class TestSchemaDiffer(TestCase):
                 columns=[name_column],
             )
         ]
+
+        # Test 1 - Tell Piccolo the column was renamed
+        schema_differ = SchemaDiffer(
+            schema=schema, schema_snapshot=schema_snapshot, auto_input="y"
+        )
+        self.assertEqual(schema_differ.add_columns.statements, [])
+        self.assertEqual(schema_differ.drop_columns.statements, [])
+        self.assertEqual(
+            schema_differ.rename_columns.statements,
+            [
+                "manager.rename_column(table_class_name='Band', tablename='band', old_column_name='title', new_column_name='name', old_db_column_name='title', new_db_column_name='name')"  # noqa
+            ],
+        )
+
+        # Test 2 - Tell Piccolo the column wasn't renamed
+        schema_differ = SchemaDiffer(
+            schema=schema, schema_snapshot=schema_snapshot, auto_input="n"
+        )
+        self.assertEqual(
+            schema_differ.add_columns.statements,
+            [
+                "manager.add_column(table_class_name='Band', tablename='band', column_name='name', db_column_name='name', column_class_name='Varchar', column_class=Varchar, params={'length': 255, 'default': '', 'null': False, 'primary_key': False, 'unique': False, 'index': False, 'index_method': IndexMethod.btree, 'choices': None, 'db_column_name': None, 'secret': False})"  # noqa: E501
+            ],
+        )
+        self.assertEqual(
+            schema_differ.drop_columns.statements,
+            [
+                "manager.drop_column(table_class_name='Band', tablename='band', column_name='title', db_column_name='title')"  # noqa: E501
+            ],
+        )
+        self.assertTrue(schema_differ.rename_columns.statements == [])
+
+    @patch("piccolo.apps.migrations.auto.schema_differ.input")
+    def test_rename_multiple_columns(self, input: MagicMock):
+        """
+        Make sure renaming columns works when several columns have been
+        renamed.
+        """
+        # We're going to rename a1 to a2, and b1 to b2.
+        a1 = Varchar()
+        a1._meta.name = "a1"
+
+        a2 = Varchar()
+        a2._meta.name = "a2"
+
+        b1 = Varchar()
+        b1._meta.name = "b1"
+
+        b2 = Varchar()
+        b2._meta.name = "b2"
+
         schema_snapshot: t.List[DiffableTable] = [
             DiffableTable(
                 class_name="Band",
                 tablename="band",
-                columns=[title_column],
+                columns=[a1, b1],
+            )
+        ]
+        schema: t.List[DiffableTable] = [
+            DiffableTable(
+                class_name="Band",
+                tablename="band",
+                columns=[a2, b2],
+            )
+        ]
+
+        def mock_input(value: str):
+            """
+            We need to dynamically set the return value based on what's passed
+            in.
+            """
+            return (
+                "y"
+                if value
+                in (
+                    "Did you rename the `a1` column to `a2` on the `Band` table? (y/N)",  # noqa: E501
+                    "Did you rename the `b1` column to `b2` on the `Band` table? (y/N)",  # noqa: E501
+                )
+                else "n"
+            )
+
+        input.side_effect = mock_input
+
+        schema_differ = SchemaDiffer(
+            schema=schema, schema_snapshot=schema_snapshot
+        )
+        self.assertEqual(schema_differ.add_columns.statements, [])
+        self.assertEqual(schema_differ.drop_columns.statements, [])
+        self.assertEqual(
+            schema_differ.rename_columns.statements,
+            [
+                "manager.rename_column(table_class_name='Band', tablename='band', old_column_name='a1', new_column_name='a2', old_db_column_name='a1', new_db_column_name='a2')",  # noqa: E501
+                "manager.rename_column(table_class_name='Band', tablename='band', old_column_name='b1', new_column_name='b2', old_db_column_name='b1', new_db_column_name='b2')",  # noqa: E501
+            ],
+        )
+
+        self.assertEqual(
+            input.call_args_list,
+            [
+                call(
+                    "Did you rename the `a1` column to `a2` on the `Band` table? (y/N)"  # noqa: E501
+                ),
+                call(
+                    "Did you rename the `b1` column to `b2` on the `Band` table? (y/N)"  # noqa: E501
+                ),
+            ],
+        )
+
+    @patch("piccolo.apps.migrations.auto.schema_differ.input")
+    def test_rename_some_columns(self, input: MagicMock):
+        """
+        Make sure that some columns can be marked as renamed, and others are
+        dropped / created.
+        """
+        # We're going to rename a1 to a2, but want b1 to be dropped, and b2 to
+        # be created.
+        a1 = Varchar()
+        a1._meta.name = "a1"
+
+        a2 = Varchar()
+        a2._meta.name = "a2"
+
+        b1 = Varchar()
+        b1._meta.name = "b1"
+
+        b2 = Varchar()
+        b2._meta.name = "b2"
+
+        schema_snapshot: t.List[DiffableTable] = [
+            DiffableTable(
+                class_name="Band",
+                tablename="band",
+                columns=[a1, b1],
             )
         ]
+        schema: t.List[DiffableTable] = [
+            DiffableTable(
+                class_name="Band",
+                tablename="band",
+                columns=[a2, b2],
+            )
+        ]
+
+        def mock_input(value: str):
+            """
+            We need to dynamically set the return value based on what's passed
+            in.
+            """
+            return (
+                "y"
+                if value
+                == "Did you rename the `a1` column to `a2` on the `Band` table? (y/N)"  # noqa: E501
+                else "n"
+            )
+
+        input.side_effect = mock_input
 
         schema_differ = SchemaDiffer(
-            schema=schema, schema_snapshot=schema_snapshot, auto_input="y"
+            schema=schema, schema_snapshot=schema_snapshot
+        )
+        self.assertEqual(
+            schema_differ.add_columns.statements,
+            [
+                "manager.add_column(table_class_name='Band', tablename='band', column_name='b2', db_column_name='b2', column_class_name='Varchar', column_class=Varchar, params={'length': 255, 'default': '', 'null': False, 'primary_key': False, 'unique': False, 'index': False, 'index_method': IndexMethod.btree, 'choices': None, 'db_column_name': None, 'secret': False})"  # noqa: E501
+            ],
+        )
+        self.assertEqual(
+            schema_differ.drop_columns.statements,
+            [
+                "manager.drop_column(table_class_name='Band', tablename='band', column_name='b1', db_column_name='b1')"  # noqa: E501
+            ],
+        )
+        self.assertEqual(
+            schema_differ.rename_columns.statements,
+            [
+                "manager.rename_column(table_class_name='Band', tablename='band', old_column_name='a1', new_column_name='a2', old_db_column_name='a1', new_db_column_name='a2')",  # noqa: E501
+            ],
         )
 
-        self.assertTrue(len(schema_differ.rename_columns.statements) == 1)
         self.assertEqual(
-            schema_differ.rename_columns.statements[0],
-            "manager.rename_column(table_class_name='Band', tablename='band', old_column_name='title', new_column_name='name', old_db_column_name='title', new_db_column_name='name')",  # noqa
+            input.call_args_list,
+            [
+                call(
+                    "Did you rename the `a1` column to `a2` on the `Band` table? (y/N)"  # noqa: E501
+                ),
+                call(
+                    "Did you rename the `b1` column to `b2` on the `Band` table? (y/N)"  # noqa: E501
+                ),
+            ],
         )
 
     def test_alter_column_precision(self):