From b7068ff9efd88d321b6c61c8ce19c64991c22473 Mon Sep 17 00:00:00 2001 From: pctablet505 Date: Wed, 15 Jul 2026 18:03:43 +0000 Subject: [PATCH 1/2] fix: don't treat an already-run migration as unrecognised When a specific migration ID is requested via the forwards command and it has already run, Piccolo now exits successfully with no migrations to run. Fixes #1359 --- piccolo/apps/migrations/commands/forwards.py | 5 ++ .../commands/test_forwards_backwards.py | 61 ++++++++++++++++++- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/piccolo/apps/migrations/commands/forwards.py b/piccolo/apps/migrations/commands/forwards.py index adc7e657d..5e23509cd 100644 --- a/piccolo/apps/migrations/commands/forwards.py +++ b/piccolo/apps/migrations/commands/forwards.py @@ -56,6 +56,11 @@ async def run_migrations(self, app_config: AppConfig) -> MigrationResult: subset = havent_run elif self.migration_id == "1": subset = havent_run[:1] + elif self.migration_id in already_ran: + # The migration has already been run, so there's nothing to do. + message = "🏁 No migrations need to be run" + print(message) + return MigrationResult(success=True, message=message) else: try: index = havent_run.index(self.migration_id) diff --git a/tests/apps/migrations/commands/test_forwards_backwards.py b/tests/apps/migrations/commands/test_forwards_backwards.py index e763bdc23..34595b15b 100644 --- a/tests/apps/migrations/commands/test_forwards_backwards.py +++ b/tests/apps/migrations/commands/test_forwards_backwards.py @@ -6,10 +6,13 @@ from unittest.mock import MagicMock, call, patch from piccolo.apps.migrations.commands.backwards import backwards -from piccolo.apps.migrations.commands.forwards import forwards +from piccolo.apps.migrations.commands.forwards import ( + ForwardsMigrationManager, + forwards, +) from piccolo.apps.migrations.tables import Migration from piccolo.utils.sync import run_sync -from tests.base import engines_only +from tests.base import AsyncMock, engines_only from tests.example_apps.music.tables import ( Band, Concert, @@ -256,3 +259,57 @@ def tearDown(self): table_class.alter().drop_table( cascade=True, if_exists=True ).run_sync() + + +class TestForwardsAlreadyRan(TestCase): + """ + Regression test for when a specific migration ID has already run, but + there are later migrations which haven't. The command should succeed + rather than reporting the migration as unrecognised. + """ + + @patch("piccolo.apps.migrations.commands.forwards.print") + @patch( + "piccolo.apps.migrations.commands.forwards.Migration.get_migrations_which_ran", + new_callable=AsyncMock, + ) + @patch( + "piccolo.apps.migrations.commands.forwards.ForwardsMigrationManager.get_migration_modules" + ) + def test_already_ran_migration_with_later_unrun( + self, + get_migration_modules: MagicMock, + get_migrations_which_ran: MagicMock, + print_: MagicMock, + ): + migration_id = "2020-12-17T18:44:30" + later_migration_id = "2020-12-17T18:44:39" + + get_migrations_which_ran.return_value = [migration_id] + + first_module = MagicMock() + first_module.ID = migration_id + later_module = MagicMock() + later_module.ID = later_migration_id + + get_migration_modules.return_value = { + migration_id: first_module, + later_migration_id: later_module, + } + + manager = ForwardsMigrationManager( + app_name="music", migration_id=migration_id + ) + + app_config = MagicMock() + app_config.app_name = "music" + app_config.resolved_migrations_folder_path = "/tmp" + + result = run_sync(manager.run_migrations(app_config)) + + self.assertTrue(result.success) + self.assertIn( + call("🏁 No migrations need to be run"), print_.mock_calls + ) + first_module.forwards.assert_not_called() + later_module.forwards.assert_not_called() From f1332c0b95b43d3665e8092265041e9ff2857700 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Mon, 20 Jul 2026 15:22:14 +0100 Subject: [PATCH 2/2] shift logic a bit, and tweak test --- piccolo/apps/migrations/commands/forwards.py | 10 +-- .../commands/test_forwards_backwards.py | 88 +++++++------------ 2 files changed, 35 insertions(+), 63 deletions(-) diff --git a/piccolo/apps/migrations/commands/forwards.py b/piccolo/apps/migrations/commands/forwards.py index 5e23509cd..0f0a5791c 100644 --- a/piccolo/apps/migrations/commands/forwards.py +++ b/piccolo/apps/migrations/commands/forwards.py @@ -31,6 +31,11 @@ async def run_migrations(self, app_config: AppConfig) -> MigrationResult: app_name=app_config.app_name ) + if self.migration_id in already_ran: + message = f"🏁 Migration {self.migration_id} has already been run" + print(message) + return MigrationResult(success=True, message=message) + migration_modules: dict[str, MigrationModule] = ( self.get_migration_modules( app_config.resolved_migrations_folder_path @@ -56,11 +61,6 @@ async def run_migrations(self, app_config: AppConfig) -> MigrationResult: subset = havent_run elif self.migration_id == "1": subset = havent_run[:1] - elif self.migration_id in already_ran: - # The migration has already been run, so there's nothing to do. - message = "🏁 No migrations need to be run" - print(message) - return MigrationResult(success=True, message=message) else: try: index = havent_run.index(self.migration_id) diff --git a/tests/apps/migrations/commands/test_forwards_backwards.py b/tests/apps/migrations/commands/test_forwards_backwards.py index 34595b15b..b9d89860c 100644 --- a/tests/apps/migrations/commands/test_forwards_backwards.py +++ b/tests/apps/migrations/commands/test_forwards_backwards.py @@ -6,10 +6,7 @@ from unittest.mock import MagicMock, call, patch from piccolo.apps.migrations.commands.backwards import backwards -from piccolo.apps.migrations.commands.forwards import ( - ForwardsMigrationManager, - forwards, -) +from piccolo.apps.migrations.commands.forwards import forwards from piccolo.apps.migrations.tables import Migration from piccolo.utils.sync import run_sync from tests.base import AsyncMock, engines_only @@ -144,6 +141,35 @@ def test_forwards_unknown_migration(self, print_: MagicMock): in print_.mock_calls ) + @patch("piccolo.apps.migrations.commands.forwards.print") + @patch( + "piccolo.apps.migrations.commands.forwards.Migration.get_migrations_which_ran", # noqa: E501 + new_callable=AsyncMock, + ) + def test_already_ran( + self, + get_migrations_which_ran: MagicMock, + print_: MagicMock, + ): + """ + When a specific migration ID has already run, but there are later + migrations which haven't, then the command should succeed rather than + reporting the migration as unrecognised. + + https://github.com/piccolo-orm/piccolo/issues/1359 + + """ + migration_id = "2020-12-17T18:44:30" + + get_migrations_which_ran.return_value = [migration_id] + + run_sync(forwards(app_name="music", migration_id=migration_id)) + + self.assertIn( + call(f"🏁 Migration {migration_id} has already been run"), + print_.mock_calls, + ) + @patch("piccolo.apps.migrations.commands.backwards.print") def test_backwards_unknown_migration(self, print_: MagicMock): """ @@ -259,57 +285,3 @@ def tearDown(self): table_class.alter().drop_table( cascade=True, if_exists=True ).run_sync() - - -class TestForwardsAlreadyRan(TestCase): - """ - Regression test for when a specific migration ID has already run, but - there are later migrations which haven't. The command should succeed - rather than reporting the migration as unrecognised. - """ - - @patch("piccolo.apps.migrations.commands.forwards.print") - @patch( - "piccolo.apps.migrations.commands.forwards.Migration.get_migrations_which_ran", - new_callable=AsyncMock, - ) - @patch( - "piccolo.apps.migrations.commands.forwards.ForwardsMigrationManager.get_migration_modules" - ) - def test_already_ran_migration_with_later_unrun( - self, - get_migration_modules: MagicMock, - get_migrations_which_ran: MagicMock, - print_: MagicMock, - ): - migration_id = "2020-12-17T18:44:30" - later_migration_id = "2020-12-17T18:44:39" - - get_migrations_which_ran.return_value = [migration_id] - - first_module = MagicMock() - first_module.ID = migration_id - later_module = MagicMock() - later_module.ID = later_migration_id - - get_migration_modules.return_value = { - migration_id: first_module, - later_migration_id: later_module, - } - - manager = ForwardsMigrationManager( - app_name="music", migration_id=migration_id - ) - - app_config = MagicMock() - app_config.app_name = "music" - app_config.resolved_migrations_folder_path = "/tmp" - - result = run_sync(manager.run_migrations(app_config)) - - self.assertTrue(result.success) - self.assertIn( - call("🏁 No migrations need to be run"), print_.mock_calls - ) - first_module.forwards.assert_not_called() - later_module.forwards.assert_not_called()