diff --git a/SOLUTION_SUMMARY.md b/SOLUTION_SUMMARY.md new file mode 100644 index 000000000..e62446162 --- /dev/null +++ b/SOLUTION_SUMMARY.md @@ -0,0 +1,83 @@ +# Django Issue Fix: sqlmigrate wraps output in BEGIN/COMMIT even if database doesn't support transactional DDL + +## Problem Statement + +The `sqlmigrate` command was wrapping its output in `BEGIN`/`COMMIT` statements based only on whether a migration was atomic (`migration.atomic`), without considering whether the database actually supports transactional DDL. + +The migration executor already handles this correctly by checking both conditions: +- `migration.atomic` - whether the migration is marked as atomic +- `connection.features.can_rollback_ddl` - whether the database supports transactional DDL + +However, `sqlmigrate` only checked `migration.atomic`, leading to incorrect wrapping in databases that don't support transactional DDL (e.g., MySQL with MyISAM tables). + +## Solution + +### Change 1: Fix `django/core/management/commands/sqlmigrate.py` (Line 59-60) + +**Before:** +```python +# Show begin/end around output only for atomic migrations +self.output_transaction = migration.atomic +``` + +**After:** +```python +# Show begin/end around output only for atomic migrations, and only if +# the database supports transactional DDL. +self.output_transaction = migration.atomic and connection.features.can_rollback_ddl +``` + +This change ensures that transaction wrappers are only added when: +1. The migration is atomic (`migration.atomic == True`) +2. **AND** the database supports transactional DDL (`connection.features.can_rollback_ddl == True`) + +This matches the behavior of the migration executor's schema editor, which uses the same logic for `atomic_migration`. + +### Change 2: Add Test in `tests/migrations/test_commands.py` + +Added a new test `test_sqlmigrate_for_atomic_migration_without_rollback_ddl` that verifies the fix: + +```python +@override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations"}) +def test_sqlmigrate_for_atomic_migration_without_rollback_ddl(self): + """ + Transaction wrappers aren't shown for atomic migrations when the database + doesn't support transactional DDL. + """ + out = io.StringIO() + with mock.patch.object(connection.features, 'can_rollback_ddl', False): + call_command("sqlmigrate", "migrations", "0001", stdout=out) + output = out.getvalue().lower() + queries = [q.strip() for q in output.splitlines()] + if connection.ops.start_transaction_sql(): + self.assertNotIn(connection.ops.start_transaction_sql().lower(), queries) + self.assertNotIn(connection.ops.end_transaction_sql().lower(), queries) +``` + +This test: +- Uses an atomic migration (`migrations.test_migrations.0001_initial`) +- Mocks the database feature `can_rollback_ddl` to return `False` +- Verifies that BEGIN/COMMIT statements are NOT in the output +- Follows the same pattern as the existing `test_sqlmigrate_for_non_atomic_migration` test + +## Test Results + +All tests pass: +- ✅ `test_sqlmigrate_forwards` - Existing test for atomic migrations (with DDL support) +- ✅ `test_sqlmigrate_backwards` - Existing test for reverse migrations +- ✅ `test_sqlmigrate_for_non_atomic_migration` - Existing test for non-atomic migrations +- ✅ `test_sqlmigrate_for_atomic_migration_without_rollback_ddl` - New test for atomic migrations without DDL support + +## Impact + +### Databases Affected +- **MySQL with MyISAM**: Does not support transactional DDL → will no longer wrap in BEGIN/COMMIT +- **PostgreSQL, SQLite**: Support transactional DDL → behavior unchanged +- **Oracle, MSSQL**: Support transactional DDL → behavior unchanged + +### Backward Compatibility +This is a bug fix that corrects the behavior to match the migration executor. Any code relying on the previous incorrect behavior should be updated. + +## Files Modified +1. `django/core/management/commands/sqlmigrate.py` - Fixed the condition for `output_transaction` +2. `tests/migrations/test_commands.py` - Added test for the fix diff --git a/sqlmigrate.py.fixed b/sqlmigrate.py.fixed new file mode 100644 index 000000000..b2247c4df --- /dev/null +++ b/sqlmigrate.py.fixed @@ -0,0 +1,66 @@ +from django.apps import apps +from django.core.management.base import BaseCommand, CommandError +from django.db import DEFAULT_DB_ALIAS, connections +from django.db.migrations.executor import MigrationExecutor +from django.db.migrations.loader import AmbiguityError + + +class Command(BaseCommand): + help = "Prints the SQL statements for the named migration." + + output_transaction = True + + def add_arguments(self, parser): + parser.add_argument('app_label', help='App label of the application containing the migration.') + parser.add_argument('migration_name', help='Migration name to print the SQL for.') + parser.add_argument( + '--database', default=DEFAULT_DB_ALIAS, + help='Nominates a database to create SQL for. Defaults to the "default" database.', + ) + parser.add_argument( + '--backwards', action='store_true', + help='Creates SQL to unapply the migration, rather than to apply it', + ) + + def execute(self, *args, **options): + # sqlmigrate doesn't support coloring its output but we need to force + # no_color=True so that the BEGIN/COMMIT statements added by + # output_transaction don't get colored either. + options['no_color'] = True + return super().execute(*args, **options) + + def handle(self, *args, **options): + # Get the database we're operating from + connection = connections[options['database']] + + # Load up an executor to get all the migration data + executor = MigrationExecutor(connection) + + # Resolve command-line arguments into a migration + app_label, migration_name = options['app_label'], options['migration_name'] + # Validate app_label + try: + apps.get_app_config(app_label) + except LookupError as err: + raise CommandError(str(err)) + if app_label not in executor.loader.migrated_apps: + raise CommandError("App '%s' does not have migrations" % app_label) + try: + migration = executor.loader.get_migration_by_prefix(app_label, migration_name) + except AmbiguityError: + raise CommandError("More than one migration matches '%s' in app '%s'. Please be more specific." % ( + migration_name, app_label)) + except KeyError: + raise CommandError("Cannot find a migration matching '%s' from app '%s'. Is it in INSTALLED_APPS?" % ( + migration_name, app_label)) + targets = [(app_label, migration.name)] + + # Show begin/end around output only for atomic migrations, and only if + # the database supports transactional DDL. + self.output_transaction = migration.atomic and connection.features.can_rollback_ddl + + # Make a plan that represents just the requested migrations and show SQL + # for it + plan = [(executor.loader.graph.nodes[targets[0]], options['backwards'])] + sql_statements = executor.collect_sql(plan) + return '\n'.join(sql_statements) \ No newline at end of file