Fixed #24529 - Allow double squashing of migrations #14380

rtpg · 2021-05-11T03:20:20Z

In order to support multi-level squashing, we need to be a bit smarter about how we traverse replacements. The solution here introduces some extra checks on squashed migrations (mainly a lookup for whether its replacements need to be squashed first), but the performance hit shouldn't be very large.

This is missing documentation updates and changelog entries, but I would like to get a first look at the implementation strategy here, as well as field any extra testing requests before going further down this path.

django/core/management/commands/squashmigrations.py

django/db/migrations/loader.py

jacobtylerwalls

Hi @rtpg, thanks for this. I left some comments to move this forward. The main thing at this point is that we will need some tests that show the necessity of your changes in the loader. I would expect some changes to the loader to be necessary, but since the test passes without them, the test coverage looks lacking. You might look at the original failure case from ticket-23090 where the restriction against double-squashing was introduced.

Make sure to keep the ticket flags updated (uncheck "Needs ..." flags to become visible for re-review.) Happy to help out as you iterate--thanks again for the patch.

jacobtylerwalls · 2021-07-25T02:36:35Z

django/core/management/commands/squashmigrations.py

-            if migration.replaces:
-                replaces.extend(migration.replaces)
-            else:
-                replaces.append((migration.app_label, migration.name))


I checked your regression test; it fails on main, as it should. However, when applying only your changes to the squashmigrations.py command (here) and not your changes to the migration loader, your test case passes.

Alright, I looked into this a bit today.

So my main dilemma is that, at least running locally, the order of migration loading (for disk migrations) is non-deterministic in the old version of the loader. In attempting to make a failing test for this, I found that the existing test does fail sometimes with the old code without the migration loader. But only sometimes!

It's dependent on the ordering of the iteration of the migrations when the disk migrations are loaded in load_disk. In particular the names are loaded into a set (that is where the non-determinism comes I think, since dictionary iteration should in theory be stable over runs).

So I think it's not really possible to make a test case that would fail consistently with the old code, while being a correct migration graph in the new model.

Would it be good to say that the added test_squashmigrations_squashes_already_squashed test (which fails non-deterministically with the old logic) covers the main logic and the "happy path", but that I still should add one test for the cycle tracking in the replacement logic?

I'm just getting up to speed again, but I do see that reverting the loader changes now leads to a consistent failure (good):

django.db.migrations.exceptions.NodeNotFoundError: Unable to find replacement node ('migrations', '3_squashed_5'). It was either never added to the migration graph, or has been removed.

tests/migrations/test_commands.py

django/core/management/commands/squashmigrations.py

django/db/migrations/loader.py

felixxm · 2022-03-03T11:17:08Z

@rtpg Do you have time to keep working on this?

rtpg · 2022-03-03T13:20:59Z

Hey @felixxm, thanks for the ping, this one fell by the wayside. Will find some time to work on this (unless someone else wants to pick it up and run with it, of course. I just want the feature in place)

rtpg · 2022-03-14T13:10:39Z

django/db/migrations/loader.py

+        memo[arg] = [True, result]
+        return result
+
+    return wrapped_func


I wrote this but now feel like it's a bit "engineered" (especially given that it's harder to write nice error messages this way), thinking now that I should just put in memoization and cycle tracking into the has_been_applied helper function directly

Also this sort of function probably belongs in utils (if it belongs at all)

rtpg · 2022-03-14T13:11:58Z

django/db/migrations/loader.py

+
+            def do_replacement(key):
+                # Toggle here to test between old code path and new one
+                NEW_WAY = True


a helper boolean for me to toggle between the old and new ways when exploring how to write a regression test (turns out the existing test would fail sporadically in practice)

rtpg · 2022-03-14T13:14:44Z

tests/migrations/test_commands.py

+
+            with open(squashed_migration_file, "r", encoding="utf-8") as fp:
+                content = fp.read()
+                # HACK Really I would just like to import the migration and do a Real Python List Comparison


I was looking around and it looks like most of the migration tests are doing these sorts of string comparisons (along with the WITH_BLACK toggles to make the formatting-dependent expected cases)

rtpg · 2022-03-14T13:23:28Z

Alright I spent some time looking at this (mainly identifying that the existing test does indeed fail with the old code sometimes), would just like some confirmation about whether my game plan (to add one test for the cycle detection code, clean up the lint issues) sounds like a good game plan here.

I also am a bit unsatisfied with various little lines of code here, though it's not so much correctness as it is legibility. Searching for strings in the generated migrations feels weird to be honest. Also a bit lost as to how to make that memoization code very clean...

In short, a bit of guidance would be appreciated

rtpg · 2022-03-16T15:06:58Z

Added the missing test and tried to clean up the code to the best of my abilities. Will look at changelog/doc editing tomorrow, a cursory search didn't mead me to any obvious documentation changes, but I know there has to be something.

jacobtylerwalls

Thanks for the updates. I haven't looked at the memoization code in detail, but did leave some comments to keep this moving.

Will look at changelog/doc editing tomorrow, a cursory search didn't mead me to any obvious documentation changes, but I know there has to be something.

The admonition in the docs needs re-writing, if it no longer applies (in full):

.. note::
    Once you've squashed a migration, you should not then re-squash that squashed
    migration until you have fully transitioned it to a normal migration.

Also, a new feature earns a small note in docs/releases/4.1.txt.

jacobtylerwalls · 2022-03-20T14:41:59Z

django/db/migrations/loader.py

+                if visited:
+                    # we visited this node but have not finished the replacement
+                    # this means we have a circular dependency
+                    raise ValueError(


Can this be CommandError?

jacobtylerwalls · 2022-03-20T14:42:13Z

tests/migrations/test_commands.py

+
+            # we expect to hit a squash replacement cycle check error, but the actual
+            # failure is dependent on the order in which the files are read on disk.
+            self.assertTrue(


style: maybe assertIn?

Also, maybe assertRaisesRegex() would help you specify the small ambiguity (squashed|auto) and collapse all of this into one assertion.

went with assertRaisesRegex

jacobtylerwalls · 2022-03-20T14:48:54Z

tests/migrations/test_commands.py

+                # HACK Really I would just like to import
+                # the migration and do a Real Python List Comparison
+                #
+                # Check the replaces list, while trying to normalize the text
+                # independently of whether Black is in place.


Have you considered using the MigrationLoader? See e.g. test_loading_squashed().

went with that, thanks for the pointer!

jacobtylerwalls · 2022-03-20T15:05:04Z

django/core/management/commands/squashmigrations.py

-            if migration.replaces:
-                replaces.extend(migration.replaces)
-            else:
-                replaces.append((migration.app_label, migration.name))


I'm just getting up to speed again, but I do see that reverting the loader changes now leads to a consistent failure (good):

django.db.migrations.exceptions.NodeNotFoundError: Unable to find replacement node ('migrations', '3_squashed_5'). It was either never added to the migration graph, or has been removed.

In order to support multi-level squashing, we need to be a bit smarter about how we traverse replacements. The solution here introduces some extra checks on squashed migrations (mainly a lookup for whether its replacements need to be squashed first), but the performance hit shouldn't be very large. Allow squashed migrations to also be squashed In order to support multi-level squashing, we need to be a bit smarter about how we traverse replacements. The solution here introduces some extra checks on squashed migrations (mainly a lookup for whether its replacements need to be squashed first), but the performance hit shouldn't be very large. Try out some changes for discussion Allow squashed migrations to also be squashed In order to support multi-level squashing, we need to be a bit smarter about how we traverse replacements. The solution here introduces some extra checks on squashed migrations (mainly a lookup for whether its replacements need to be squashed first), but the performance hit shouldn't be very large. Try out some changes for discussion Add a test confirming loop handling Fix line length Add documentation for double squashing of migrations Fix up isort

rtpg · 2022-04-20T10:16:33Z

I believe to have answered all the outstanding questions on this branch, so it should be ready for a re-review!

tests/migrations/test_commands.py

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>

felixxm · 2022-05-16T07:09:16Z

@rtpg Thanks for this patch 👍 I have an issue in the following scenario:

app with 3 migrations:
- 0001_initial.py
- 0002_mymodel1_field_1_mymodel2_field_2_and_more.py,
- 0003_alter_mymodel2_unique_together.py,

steps:

apply migrations 0001 and 0002: python manage.py migrate test_one 0002
squash migrations 0001 → 0003: python manage.py squashmigrations test_one 0001 0003
make a change in the models definition and create a new migration file 0004_remove_mymodel1_field_1_mymodel1_field_3_and_more.py: python manage.py makemigrations,

squash migrations 0001 → 0004: python manage.py squashmigrations test_one 0001_initial_squashed 0004

Traceback (most recent call last):
 File "manage.py", line 22, in <module>
   main()
 File "manage.py", line 18, in main
   execute_from_command_line(sys.argv)
 File "/django/django/core/management/__init__.py", line 446, in execute_from_command_line
   utility.execute()
 File "/django/django/core/management/__init__.py", line 440, in execute
   self.fetch_command(subcommand).run_from_argv(self.argv)
 File "/django/django/core/management/base.py", line 402, in run_from_argv
   self.execute(*args, **cmd_options)
 File "/django/django/core/management/base.py", line 448, in execute
   output = self.handle(*args, **options)
 File "/django/django/core/management/commands/squashmigrations.py", line 100, in handle
   start = loader.get_migration(
 File "/django/django/db/migrations/loader.py", line 144, in get_migration
   return self.graph.nodes[app_label, name_prefix]
KeyError: ('test_one', '0001_initial_squashed_0003_alter_mymodel2_unique_together')

Sample project: ticket_24529.zip.

felixxm · 2024-03-15T09:07:54Z

Closing due to inactivity.

rtpg commented May 11, 2021

View reviewed changes

django/core/management/commands/squashmigrations.py Show resolved Hide resolved

rtpg commented May 11, 2021

View reviewed changes

django/db/migrations/loader.py Show resolved Hide resolved

rtpg changed the title ~~#24529 - Allow double squashing of migrations~~ Fixed #24529 - Allow double squashing of migrations May 17, 2021

jacobtylerwalls reviewed Jul 25, 2021

View reviewed changes

rtpg commented Mar 14, 2022

View reviewed changes

rtpg force-pushed the main branch from ad6a024 to 6f55398 Compare March 14, 2022 13:17

jacobtylerwalls reviewed Mar 20, 2022

View reviewed changes

rtpg force-pushed the main branch 3 times, most recently from 5267297 to 9ca0edf Compare April 6, 2022 02:00

rtpg force-pushed the main branch from 9ca0edf to 6917b17 Compare April 20, 2022 10:11

jacobtylerwalls reviewed Apr 20, 2022

View reviewed changes

tests/migrations/test_commands.py Outdated Show resolved Hide resolved

Update tests/migrations/test_commands.py

40edc10

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>

felixxm closed this Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #24529 - Allow double squashing of migrations #14380

Fixed #24529 - Allow double squashing of migrations #14380

rtpg commented May 11, 2021

jacobtylerwalls left a comment

jacobtylerwalls Jul 25, 2021

rtpg Mar 14, 2022

jacobtylerwalls Mar 20, 2022

felixxm commented Mar 3, 2022

rtpg commented Mar 3, 2022

rtpg Mar 14, 2022

rtpg Mar 14, 2022

rtpg Mar 14, 2022

rtpg Mar 14, 2022

rtpg commented Mar 14, 2022 •

edited

rtpg commented Mar 16, 2022

jacobtylerwalls left a comment

jacobtylerwalls Mar 20, 2022

jacobtylerwalls Mar 20, 2022

jacobtylerwalls Mar 20, 2022

rtpg Apr 6, 2022

jacobtylerwalls Mar 20, 2022

rtpg Apr 6, 2022

jacobtylerwalls Mar 20, 2022

rtpg commented Apr 20, 2022

felixxm commented May 16, 2022

felixxm commented Mar 15, 2024

Fixed #24529 - Allow double squashing of migrations #14380

Fixed #24529 - Allow double squashing of migrations #14380

Conversation

rtpg commented May 11, 2021

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixxm commented Mar 3, 2022

rtpg commented Mar 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtpg commented Mar 14, 2022 • edited

rtpg commented Mar 16, 2022

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtpg commented Apr 20, 2022

felixxm commented May 16, 2022

felixxm commented Mar 15, 2024

rtpg commented Mar 14, 2022 •

edited