Opened 2 years ago

Closed 5 months ago

#27731 closed Cleanup/optimization (fixed)

Squashmigrations doesn't optimize AlterField related_name across AlterUniqueTogether/AlterIndexTogether

Reported by: Ed Morley Owned by: Simon Charette
Component: Migrations Version: master
Severity: Normal Keywords: squashmigrations
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The squashmigrations command doesn't combine an AlterField() with CreateModel() in the case below, when all that was changed was the related_name.

ie: This fails on Django master...

diff --git a/tests/migrations/test_optimizer.py b/tests/migrations/test_optimizer.py
index fc4f0ac..93a43a7 100644
--- a/tests/migrations/test_optimizer.py
+++ b/tests/migrations/test_optimizer.py
@@ -572,6 +572,26 @@ class OptimizerTests(SimpleTestCase):
             ],
         )

+    self.assertOptimizesTo(
+        [
+            migrations.CreateModel("Bar", [("name", models.CharField(max_length=255))]),
+            migrations.CreateModel("Foo", [
+                ("a", models.ForeignKey("testapp.Bar", models.CASCADE)),
+                ("b", models.IntegerField()),
+            ]),
+            alter,
+            migrations.AlterField("Foo", "a", models.ForeignKey("testapp.Bar", models.CASCADE, related_name="baz")),
+        ],
+        [
+            migrations.CreateModel("Bar", [("name", models.CharField(max_length=255))]),
+            migrations.CreateModel("Foo", [
+                ("a", models.ForeignKey("testapp.Bar", models.CASCADE, related_name="baz")),
+                ("b", models.IntegerField()),
+            ]),
+            alter,
+        ],
+    )
+
         # RenameField
         self.assertDoesNotOptimize(
             [

This causes failures in:

  • test_create_alter_index_field
  • test_create_alter_unique_field

ie optimization should occur but doesn't, when alter is:
migrations.AlterUniqueTogether("Foo", [["a", "b"]])

...or when alter is:
migrations.AlterIndexTogether("Foo", [["a", "b"]])

(But passes in the AlterOrderWithRespectTo case).

Change History (6)

comment:1 Changed 23 months ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:2 Changed 23 months ago by Andrew Nester

I guess this behaviour is expected.
We have following assert in OptimizerTest

self.assertDoesNotOptimize(
            [
                migrations.CreateModel("Foo", [
                    ("a", models.IntegerField()),
                    ("b", models.IntegerField()),
                ]),
                alter,
                migrations.AlterField("Foo", "b", models.CharField(max_length=255)),
            ],
        )

In general, if we have AlterUniqueTogether set for some field, let's say a, and after that performs AlterField on same field then we do not perform optimisation.

Actually it could be fixed, but it requires some thoughts about how to determine what changes introduce this AlterFIeld - some of them could be safe to optimise, some of them not.

comment:3 Changed 23 months ago by Andrew Nester

Has patch: set
Owner: changed from nobody to Andrew Nester
Status: newassigned

Anyway I've added PR that fixes this and could be discussed.
It's a bit straightforward implementation PR

Last edited 23 months ago by Andrew Nester (previous) (diff)

comment:4 Changed 22 months ago by Tim Graham

Patch needs improvement: set

comment:5 Changed 5 months ago by Simon Charette

Owner: changed from Andrew Nester to Simon Charette
Patch needs improvement: unset

This is handled by PR which implements the CreateModel/AlterTogetherOptionOperation reduction into CreateModel.options and then allows the CreateModel/AlterField reduction into CreateModel.fields to take place.

comment:6 Changed 5 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 8e3f22f2:

Fixed #27731 -- Implemented CreateModel/AlterFooOperation reduction.

This should alleviate the side effects of disabling the AlterFooOperation
reduction with RemoveField to fix refs #28862 during migration squashing
because CreateModel can perform a reduction with RemoveField.

Thanks Nick Pope for the review.

Note: See TracTickets for help on using tickets.
Back to Top