[Django] #28862: Error in auto-generated migration

29 views
Skip to first unread message

Django

unread,
Nov 29, 2017, 11:54:27 AM11/29/17
to django-...@googlegroups.com
#28862: Error in auto-generated migration
-------------------------------------+-------------------------------------
Reporter: elros | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 1.9
layer (models, ORM) |
Severity: Normal | Keywords: models migrations
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
In Django 1.9.11, after deleting a model field and removing it from
`index_together` attribute, the `makemigrations` command generates a
broken migration code with RemoveField operation preceding
AlterIndexTogether operation. That causes the following `migrate` command
to raise an exception while trying to apply the generated migration.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,
Nov 29, 2017, 1:01:38 PM11/29/17
to django-...@googlegroups.com
#28862: Removing a field from index_together and from the model generates a
migration that crashes

-------------------------------------+-------------------------------------
Reporter: elros | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:

Keywords: models migrations | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

Can you reproduce that issue with Django 2.0 or Django 1.11? It sounds
familiar and may have been fixed in a newer version of Django (in general,
we would always like bug reports to be confirmed against the latest
version of Django or even Django master, if possible).

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:1>

Django

unread,
Nov 30, 2017, 3:07:57 AM11/30/17
to django-...@googlegroups.com
#28862: Removing a field from index_together and from the model generates a
migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Artem Maslovskiy):

* cc: Artem Maslovskiy (added)


--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:2>

Django

unread,
Nov 30, 2017, 6:31:42 AM11/30/17
to django-...@googlegroups.com
#28862: Removing a field from index_together and from the model generates a
migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tomer Chachamu):

We have a test that says the generated migrations are in said order:

https://github.com/django/django/blob/6c0042430e3618ce5c276d195d92a6b884daa3a3/tests/migrations/test_autodetector.py#L1323

The docstring is incorrect, it says "Removed fields will be removed after
updating index/unique_together." but the test actually checks for "Removed
fields will be removed before updating index/unique_together."

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:3>

Django

unread,
Dec 4, 2017, 3:12:33 PM12/4/17
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model

generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Tim Graham):

* component: Database layer (models, ORM) => Migrations
* stage: Unreviewed => Accepted


Comment:

That test is from the ticket I was thinking of: #23614 (fixed in Django
1.7.2 and later). The comment isn't accurate because the order of
operations changed in 5c9c1e029d139bd3d5213804af2ed9f317cd0b86 (Django
1.9). That change in ordering looks incorrect.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:4>

Django

unread,
Dec 5, 2017, 3:20:35 PM12/5/17
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.8
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Ramiro Morales):

* owner: nobody => Ramiro Morales
* status: new => assigned
* version: 1.9 => 1.8


--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:5>

Django

unread,
Dec 5, 2017, 3:22:00 PM12/5/17
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.8
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Ramiro Morales):

Replying to [comment:4 Tim Graham]:


> the order of operations changed in
5c9c1e029d139bd3d5213804af2ed9f317cd0b86 (Django 1.9).

Actually, that change was committed during the 1.8 development cycle.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:6>

Django

unread,
Dec 5, 2017, 10:07:06 PM12/5/17
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.8
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Ramiro Morales):

This is what I've found so far:

It seems the optimization introduced in
e470f311d654267ec86f9a6325ec500345b9dff2 (later refactored in
49f4c9f4c61885189d136d7073c26ecc91b482b1) by which a sequence of schema
migrations operations like this:

{{{
- AlterIndexTogether(index_together=['title', 'author', 'newfield'] ->
index_together=['title', 'author'])
- RemoveField('newfield')
}}}

gets swapped to

{{{
- RemoveField('newfield')
- AlterIndexTogether(index_together=['title', 'author', 'newfield'] ->
index_together=['title', 'author'])
}}}

This is because the `references_field()` method of
`django.db.migrations.operations.AlterIndexTogether` considers only the
final set of `index_together` fields to conclude there is no overlap in
field affected by the two operations. This reasoning might be valid when
reordering operation sequences which involve e.g. `AddField` But when
it's interacting with `RemoveField` (and `RenameField`?) it needs to
consider the _initial_ set of `index_together` fields instead.

if it did, then it would discover it can't optimize the
`AlterIndexTogether` to be after the `RemoveField` whcih is that the OP
reports.

Example is for `Meta.index_together` but affects also al least
`Meta.unique_together` too. AFAICT fixing this might involve some non-
trivial refactoring.

I'm open to confirmation/rebuttal and to ideas on how this could be
solved.

Django

unread,
Dec 5, 2017, 10:45:43 PM12/5/17
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.8
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Ramiro Morales):

Forgot to say this happens in
`django.db.migrations.autodetector.MigrationAutodetector._optimize_migrations()`

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:7>

Django

unread,
Dec 6, 2017, 4:49:25 AM12/6/17
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Markus Holtermann):

* version: 1.8 => 1.9


Comment:

A good find, Artem. Thank you!

The issue seems to come from e470f311d654267ec86f9a6325ec500345b9dff2
which is part of 1.9 release cycle, but not 1.8.

While the docstring on
https://github.com/django/django/commit/e470f311d654267ec86f9a6325ec500345b9dff2
#diff-c11e6432df7086eda3dfb9ab8e5b2839R1141 is clearly wrong, the test
itself is still correct because the `RemoveField` operation doesn't touch
any of the fields referred to in `AlterUniqueTogether` or
`AlterIndexTogether`.

There is a
[https://github.com/MarkusH/django/blob/8e352876c337332b45a72da8bbccad2830c7b1e0/tests/migrations/test_optimizer.py#L603-L632
test for the migration optimizer] that shows a RemoveField operation after
the *Together operation is not moved to the front if both involve the same
field.

Adding this code though will make the test fail:
{{{#!python
self.assertOptimizesTo(
[
migrations.CreateModel("Foo", [
("a", models.IntegerField()),
("b", models.IntegerField()),
]),
migrations.RemoveField("Foo", "b"),
alter,
],
[
migrations.CreateModel("Foo", [
("a", models.IntegerField()),
("b", models.IntegerField()),
]),
alter,
migrations.RemoveField("Foo", "b"),
],
)
}}}
which I believe is what you're experiencing. Maybe even this would be an
expected behavior:
{{{#!python

self.assertOptimizesTo(
[
migrations.CreateModel("Foo", [
("a", models.IntegerField()),
("b", models.IntegerField()),
]),
migrations.RemoveField("Foo", "b"),
alter,
],
[
migrations.CreateModel("Foo", [
("a", models.IntegerField()),
]),
],
)
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:8>

Django

unread,
Dec 6, 2017, 5:31:45 AM12/6/17
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Ramiro Morales):

Replying to [comment:8 Markus Holtermann]:


> the test itself is still correct because the `RemoveField` operation
doesn't touch any of the fields referred to in `AlterUniqueTogether` or
`AlterIndexTogether`.

The point I tried to make in comment:6 is that this reasoning (and hence
the asserts of the test case) is wrong because if the user removes a field
from the model and from Meta.*_together (the test case scenario) then it's
wrong to optimize the sequence to have te RemoveField first even it it
doesn't mention any of the fields which will remain in *_together.

The asserts in the test case examine the order and type of parts of the
generated migration and don't fail because no DLL code is executed agaonst
the DB at that point. The breakage happens at migration application time
when it wants to execute the Alter*Together operation.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:9>

Django

unread,
Dec 6, 2017, 6:07:40 AM12/6/17
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Markus Holtermann):

Now I understand, Ramiro. I concur with your evaluation that the initial
assumption is wrong. I am as well surprised that this hasn't come up
earlier / more frequently.

Another idea that came to mind was adding temporary dependencies in
https://github.com/MarkusH/django/blob/8e352876c337332b45a72da8bbccad2830c7b1e0/django/db/migrations/autodetector.py#L1006-L1021

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:10>

Django

unread,
Dec 18, 2017, 10:47:24 AM12/18/17
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Ramiro Morales):

#28916 was a duplicate.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:11>

Django

unread,
Feb 11, 2018, 10:30:13 PM2/11/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

#29124 was another duplicate.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:12>

Django

unread,
Feb 12, 2018, 7:29:56 AM2/12/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-------------------------------------+-------------------------------------
Reporter: Artem Maslovskiy | Owner: Ramiro
| Morales
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Ed Morley):

* cc: Ed Morley (added)


--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:13>

Django

unread,
May 16, 2018, 7:29:26 PM5/16/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: (none)
Type: Bug | Status: new

Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Ramiro Morales):

* owner: Ramiro Morales => (none)
* status: assigned => new


--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:14>

Django

unread,
May 29, 2018, 11:05:22 PM5/29/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned

Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Jeff):

* owner: (none) => Jeff


* status: new => assigned


--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:15>

Django

unread,
May 29, 2018, 11:16:56 PM5/29/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Jeff):

Ramiro Morales pointed out I am working on another duplicate #29123. I'll
take ownership of this as well.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:16>

Django

unread,
May 30, 2018, 5:14:15 PM5/30/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Jeff):

Looking into overwriting the `FieldRelatedOptionOperation`'s reduce method
to additionally:

1) check if the current operation is a `RemoveField` (and maybe
`RenameField`, I need to confirm if it is also problematic)
2) if it is a `RemoveField` operation, to look into the model (as it
exists in the DB) and if the field being removed is in `unique_together`,
`index_together`, or `order_with_respect_to`, to skip the operation.

This would correct the optimizer's erroneous migrations and I do not think
would cause any other tests to fail.

I am still new to the codebase, could anyone point me in the right
direction on how I can get the current state of the table in the DB? Using
`apps.config`'s `get_model()` is returning the state of the model from
`models.py`, not the current state of the model as it exists in the DB. I
am trying to use `ProjectState` at the moment. I plan to continue down
this path, but please let me know if you can guide me to a more correct
method of inspecting the current DB table as a model.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:17>

Django

unread,
Jun 28, 2018, 12:27:24 PM6/28/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: (none)
Type: Bug | Status: new

Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Jeff):

* owner: Jeff => (none)


* status: assigned => new


--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:18>

Django

unread,
Jul 6, 2018, 8:46:17 PM7/6/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Jeff):

* owner: (none) => Jeff


* status: new => assigned


--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:19>

Django

unread,
Jul 9, 2018, 9:54:53 PM7/9/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Jeff):

* has_patch: 0 => 1


Comment:

has [https://github.com/django/django/pull/10162 patch] set for
consideration.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:20>

Django

unread,
Jul 12, 2018, 4:43:18 PM7/12/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Ramiro Morales):

#29123 was a duplicate.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:21>

Django

unread,
Jul 14, 2018, 12:41:58 AM7/14/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Simon Charette):

I had a look at the issue and I came to the conclusion that the only
solution for now is to disable `AlterFooTogether` optimization when a
`RemoveField` on the same model is involved.

As mentioned by Ramiro we'd need to have access to the ''previous''
`foo_together` value to determine whether or not the optimization can take
place. This is a class of problem that also came up when working on #27768
where I had to disable an optimization from taking place because I didn't
have the ''previous'' context of a `RemoveField`.

I guess I'd be possible to have newly generated operation embed a such a
''previous'' state to allow the optimization to take place but I figured
out what I believe is a clever way of working around this disabled
optimization. By implementing `CreateModel`/`AlterFooOperation` reduction
most of the negative side effect during migration squashing where the
optimizer perform a complete reduction are gone because the former is able
to reduce `RemoveField` operation.

All of these ideas are implemented in this
[https://github.com/django/django/pull/10178 PR].

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:22>

Django

unread,
Jul 14, 2018, 11:23:15 AM7/14/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Simon Charette):

#26180 was a duplicate.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:23>

Django

unread,
Jul 14, 2018, 12:16:17 PM7/14/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Simon Charette):

Hey Jeff, sorry from stealing this from you. I know you've invested a lot
of effort in getting this fixed and I should have reached out to discuss
why I think this a more appropriate solution.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:24>

Django

unread,
Jul 14, 2018, 12:58:46 PM7/14/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Jeff):

I wish you had. I was still working on this last night and as a new
contributor had invested a lot of time getting familiarized with all the
migrations/fields/optimizer code to get out that initial PR. As someone
new who wants to become a regular contributor I really would appreciate if
you'd help me get this across the line instead of coming in and just doing
it while I am still trying to figure out how to do it the way you
suggested, instead of the way I did that seemed to work correctly.

Replying to [comment:24 Simon Charette]:


> Hey Jeff, sorry from stealing this from you. I know you've invested a
lot of effort in getting this fixed and I should have reached out to
discuss why I think this a more appropriate solution.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:25>

Django

unread,
Jul 14, 2018, 3:53:47 PM7/14/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Tim Graham):

Take heart, Jeff, there are plenty of other tickets to use your new
knowledge. Also, you can review Simon's patch. We have more people writing
code than reviewing and we don't merge patches without review, so that's
an important task. Thanks for your interest and effort.

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:26>

Django

unread,
Jul 19, 2018, 6:04:35 PM7/19/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: closed
Component: Migrations | Version: 1.9
Severity: Normal | Resolution: fixed

Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Tim Graham <timograham@…>):

* status: assigned => closed
* resolution: => fixed


Comment:

In [changeset:"ed7898e1b58c29cda648a799ac4bd5bc7e193b8b" ed7898e1]:
{{{
#!CommitTicketReference repository=""
revision="ed7898e1b58c29cda648a799ac4bd5bc7e193b8b"
Fixed #28862 -- Disabled optimization of AlterFooTogether and RemoveField.

AlterFooTogether operations cannot be swapped with RemoveField operations
on
the same model as they could be removing the the same field as well.

Since AlterFooTogether operations don't track what their previous value
was,
it's impossible to determine whether or not the optimization is safe so
the
only way to proceed is to disable the optimization.

Thanks Ramiro Morales for the in-depth analysis of the issue.

Refs #24828
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:27>

Django

unread,
Jul 19, 2018, 6:04:35 PM7/19/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: closed
Component: Migrations | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Tim Graham <timograham@…>):

In [changeset:"8e3f22f2513a5b64153ea9903690a38ac159031b" 8e3f22f2]:
{{{
#!CommitTicketReference repository=""
revision="8e3f22f2513a5b64153ea9903690a38ac159031b"
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.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:28>

Django

unread,
Jul 19, 2018, 6:04:35 PM7/19/18
to django-...@googlegroups.com
#28862: Removing a field from index_together/unique_together and from the model
generates a migration that crashes
-----------------------------------+------------------------------------
Reporter: Artem Maslovskiy | Owner: Jeff
Type: Bug | Status: closed
Component: Migrations | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: models migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Tim Graham <timograham@…>):

In [changeset:"1e9b02a4c28142303fb4d33632e77ff7e26acb8b" 1e9b02a]:
{{{
#!CommitTicketReference repository=""
revision="1e9b02a4c28142303fb4d33632e77ff7e26acb8b"
Refs #28862 -- Removed the FieldRelatedOptionOperation.reduce()
optimization.

It isn't required anymore since AlterTogetherOperations can be reduced
into
CreateModels which can reduce DeleteField operations.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28862#comment:29>

Reply all
Reply to author
Forward
0 new messages