Opened 7 years ago

Closed 6 years 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: dev
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 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Andrew Nester, 7 years ago

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 by Andrew Nester, 7 years ago

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 7 years ago by Andrew Nester (previous) (diff)

comment:4 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:5 by Simon Charette, 6 years ago

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 by Tim Graham <timograham@…>, 6 years ago

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