[Django] #31416: FieldError when migrating field to new model subclass
Django
----------------------------------------+------------------------
Reporter: eduardanghel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------+------------------------
Analogous to #21890. If creating a model subclass and moving a field onto
it in the same step, `makemigrations` works but `migrate` dies with
`django.core.exceptions.FieldError: Local field 'title' in class 'Book'
clashes with field of the same name from base class 'Readable'.`
For example, take this model:
{{{
from django.db import models
class Readable(models.Model):
title = models.CharField(max_length=200)
}}}
And change to this:
{{{
from django.db import models
class Readable(models.Model):
pass
class Book(Readable):
title = models.CharField(max_length=200)
}}}
The migration generates with `CreateModel` for Book, then `RemoveField`
for Readable.title. But running it produces the error.
Reversing the order of the migration operations makes it pass. The auto-
detector should be able to use this order.
--
Ticket URL: <https://code.djangoproject.com/ticket/31416>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: Markus Holtermann, Simon Charette (added)
* stage: Unreviewed => Accepted
Comment:
Tentatively accepted for a future investigation. I'm not sure if this is
feasible because detecting changes related with models' bases are tricky.
Moreover you will lose data with such change, so IMO it's preferable to
handle this manually.
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:1>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chinmoy):
Maybe the makemigrations could present some kind of info about the data
being lost and let the user decide if the migrations be applied or not?
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:2>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
> Maybe the makemigrations could present some kind of info about the data
being lost and let the user decide if the migrations be applied or not?
Yes, or it could at least detect that the migration it generates can't be
applied without manual editing to control the data loss, and it could show
a warning/message about this.
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:3>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
would this be something doable for a new beginner like me? much
appreciated!
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:4>
Django
-------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nan Liu):
* cc: Nan Liu (added)
* owner: nobody => Nan Liu
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:5>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I think it should be doable to teach the auto-detector about generating
these operations in the right order.
I think the best approach would be to adjust `check_dependency` to account
for this case. What's happening now is that created models detection runs
before removed models one so while both operations depend on `Readable`
model. It's possible that `generate_created_models` might need to be
adjusted to add a dependency on field removal of all of its base to make
sure the order is swapped. I think the correct dependency representation
is `(base_app_label, base_model_name, field_name, False)` for all fields
in all bases in
https://github.com/django/django/blob/6fbce45b0376f0ec8f3bf244f4f1f8d62c201f58/django/db/migrations/autodetector.py#L561-L565.
Something the solution has to consider is when a field is removed from a
base while two subclasses are added in the same run with this field (e.g.
an extra `class Magazine(Readable): title =
models.CharField(max_length=200)` would be added to the reporter's
example). In this case a single `RemoveField` must be generated
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:6>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sanskar Jaiswal):
Replying to [comment:6 Simon Charette]:
> I think it should be doable to teach the auto-detector about generating
these operations in the right order.
>
> I think the best approach would be to adjust `check_dependency` to
account for this case.
I am not able to figure out how adjusting `check_dependency`, could help
solve this issue. Assuming that I loop through all fields present in the
base model, check if they're present in the inherited model, and if there
are any such fields present, append `base_app_label, base_name,
field_name, False` to `dependencies`, how would adjusting
`check_dependency` able to reorder the migrations?
Thanks
Sanskar
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:7>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
Replying to [comment:7 Sanskar Jaiswal]:
> Replying to [comment:6 Simon Charette]:
> > I think it should be doable to teach the auto-detector about
generating these operations in the right order.
> >
> > I think the best approach would be to adjust `check_dependency` to
account for this case.
>
> I am not able to figure out how adjusting `check_dependency`, could help
solve this issue. Assuming that I loop through all fields present in the
base model, check if they're present in the inherited model, and if there
are any such fields present, append `base_app_label, base_name,
field_name, False` to `dependencies`, how would adjusting
`check_dependency` able to reorder the migrations?
>
> Thanks
> Sanskar
hi Sanskar, I am still working on this issue since I am a newbie. since
this is for my school assignment, may you work on other issues? I really
appreciate that!
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:8>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sanskar Jaiswal):
Hey Nan,
Happy that you figured out the solution. By all means, continue working on
this, hope you get a good grade on your assignment. IMO, we could check
that this is the field we are trying to remove from the base class by
looping through all the fields and checking if that field is also present
in a sub class. Since that’s not allowed, you know that this is the field
you wanna remove
Cheers
Sanskar
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:9>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
Replying to [comment:9 Sanskar Jaiswal]:
> Hey Nan,
>
> Happy that you figured out the solution. By all means, continue working
on this, hope you get a good grade on your assignment. IMO, we could check
that this is the field we are trying to remove from the base class by
looping through all the fields and checking if that field is also present
in a sub class. Since that’s not allowed, you know that this is the field
you wanna remove
>
> Cheers
> Sanskar
Thank you so much for your advice! I still can't figure out how to find
`title` field in the base class. I was able to find `title` field from a
sub class using `model_state.fields`. And I tried `model_state =
self.to_state.models[base_app_label, base_model_name]` to find its fields,
but there is only one field left which is self-generated `id` field. I am
pretty sure I am doing it wrong. But I am just wondering if there is other
ways to retrieve all fields of the base class, which is `readable` in this
case.
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:10>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sanskar Jaiswal):
>And I tried `model_state = self.to_state.models[base_app_label,
base_model_name]` to find `readable`'s fields, but there is only one field
left which is the self-generated `id` field. Not sure why the field
`title` is gone. I am pretty sure I am doing it wrong. But I am just
wondering if there are other ways to retrieve all fields from the base
class, which is `readable` in this case. much appreciated!
Since we remove `title` from `Readable`, it doesn't show up in
`self.to_state.models[base_app_label, base_model_name]`. Doing this gives
you, the fields of our latest intended model, in which `Readable` is just
an empty model with one `AutoField` as the "self generated `id`." To get
the intended fields of `Readable`, you need to use
`self.from_state.models[base_app_label, base_model_name]` since
`from_state` refers to the previous state, i.e. in this case, just one
`Readable` model with a `CharField` named `title` (and the `id` of
course).
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:11>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
Replying to [comment:11 Sanskar Jaiswal]:
> >And I tried `model_state = self.to_state.models[base_app_label,
base_model_name]` to find `readable`'s fields, but there is only one field
left which is the self-generated `id` field. Not sure why the field
`title` is gone. I am pretty sure I am doing it wrong. But I am just
wondering if there are other ways to retrieve all fields from the base
class, which is `readable` in this case. much appreciated!
>
> Since we remove `title` from `Readable`, it doesn't show up in
`self.to_state.models[base_app_label, base_model_name]`. Doing this gives
you, the fields of our latest intended model, in which `Readable` is just
an empty model with one `AutoField` as the "self generated `id`." To get
the intended fields of `Readable`, you need to use
`self.from_state.models[base_app_label, base_model_name]` since
`from_state` refers to the previous state, i.e. in this case, just one
`Readable` model with a `CharField` named `title` (and the `id` of
course).
Hi Sanskar,
Thanks! I guess I should've done a better job at code reading :). I will
try it out today since I have an internship interview coming up in a few
hours! Much appreciated! I will keep u posted!
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:12>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
Hi Y'all,
I was looking into this test case
[https://github.com/django/django/blob/6fbce45b0376f0ec8f3bf244f4f1f8d62c201f58/tests/model_inheritance/test_abstract_inheritance.py#L160-L173].
This case seems to cover the case for this problem, but the issue is
`python manage.py migrate` throws an error that we want to handle. How
should I go about writing the unit test? Much appreciated!
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:13>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
Replying to [comment:13 Nan Liu]:
> Hi Y'all,
>
> I was looking into this test case
[https://github.com/django/django/blob/6fbce45b0376f0ec8f3bf244f4f1f8d62c201f58/tests/model_inheritance/test_abstract_inheritance.py#L160-L173].
This case seems to cover the case for this problem, but the issue is
`python manage.py migrate` throws an error that we want to handle. Or
there is no need to write a test case for that since we just want to
change the behavior? if we do need test cases, How should I go about
writing the unit test? Much appreciated!
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:14>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
Nan Liu, this test doesn't cover the issue, because it's not related with
migrations. We have here inline models so migrations are not involved, it
doesn't fail. New tests should be added to `tests/migrations`, you can
find there similar tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:15>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
Replying to [comment:15 felixxm]:
> Nan Liu, this test doesn't cover the issue, because it's not related
with migrations. We have here inline models so migrations are not
involved, it doesn't fail. New tests should be added to
`tests/migrations`, you can find there similar tests.
I'm not sure whether I should put tests in `test_autoencoder` or
`test_operations` since the issue is related to adjusting autoencoder as
well as operations swapping. My intuition tells me that I should add some
tests in `test_operations.py` since we want to swap operations so that
field removals of bases can take place before newly added sub-models can
be created, but I just thought it would be always good to ask here. much
appreciated!
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:16>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
Hi @nanliu
I believe the test for this should be in `test_autodetector.py`. The tests
there don't create whole migrations. They make assertions on the output
operations.
Check out the test I wrote for a similar ticket in
https://github.com/django/django/pull/12313/files#diff-
c11e6432df7086eda3dfb9ab8e5b2839 , and those around that. They set up the
before and after model states, ask the autodetector to find the operations
through `get_changes()`, then make assertions on the operations.
The "before" model states describe the state of the models before the
change, which would be pulled from the migration history - in this case
where just `Readable` exists. The "after" states describe the state the
current model state, which would be pulled from the apps' model definitios
- in this case, `Readable` with no fields and `Book` with the `title`
field.
When writing a test, please try use the predefined model states to reduce
memory usage.
Hope that helps you write a test. Once there's a failing test it should be
easier to try find the fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:17>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
Replying to [comment:17 Adam (Chainz) Johnson]:
> Hi @nanliu
>
> I believe the test for this should be in `test_autodetector.py`. The
tests there don't create whole migrations. They make assertions on the
output operations.
>
> Check out the test I wrote for a similar ticket in
https://github.com/django/django/pull/12313/files#diff-
c11e6432df7086eda3dfb9ab8e5b2839 , and those around that. They set up the
before and after model states, ask the autodetector to find the operations
through `get_changes()`, then make assertions on the operations.
>
> The "before" model states describe the state of the models before the
change, which would be pulled from the migration history - in this case
where just `Readable` exists. The "after" states describe the state the
current model state, which would be pulled from the apps' model definitios
- in this case, `Readable` with no fields and `Book` with the `title`
field.
>
> When writing a test, please try use the predefined model states to
reduce memory usage.
>
> Hope that helps you write a test. Once there's a failing test it should
be easier to try find the fix.
So, my code should be correct by using `self.get_changes([Readable],
[Readable with no fields, Book, Magazine])`. but my code meant to fix this
issue wasn't executed when I use `self.get_changes` while doing two
separate migrations will execute my added code, which is confusing. Also I
suspect that the first migration will construct a `loader.graph` that will
reorder the action operations.
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:18>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nan Liu):
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:19>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
I am wondering when people are gonna look at the pull request? since if it
is accepted, then I would possibly gain some extra credits for my class!
much appreciated!!
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:20>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: closed => new
* resolution: fixed =>
Comment:
Nan Liu, please don't close tickets that are not fixed. We first need to
have, review and merge patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:21>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nan Liu):
Replying to [comment:21 felixxm]:
> Nan Liu, please don't close tickets that are not fixed. We first need to
have, review and merge patch.
felixxm, my bad. sorry about that
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:22>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/12754 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:23>
Django
-------------------------------+------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"33c365781abbcc1b21a31b31d95d344a174df0d5" 33c36578]:
{{{
#!CommitTicketReference repository=""
revision="33c365781abbcc1b21a31b31d95d344a174df0d5"
Fixed #31416 -- Made autodetector find dependencies for MTI model creation
on base fields removal.
Removing a base field must take place before adding a new inherited
model that has a field with the same name.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Eduard Anghel | Owner: Nan Liu
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"114da2d0455594bae0bbda8f2b1d46c5ec854956" 114da2d0]:
{{{
#!CommitTicketReference repository=""
revision="114da2d0455594bae0bbda8f2b1d46c5ec854956"
[3.1.x] Fixed #31416 -- Made autodetector find dependencies for MTI model
creation on base fields removal.
Removing a base field must take place before adding a new inherited
model that has a field with the same name.
Backport of 33c365781abbcc1b21a31b31d95d344a174df0d5 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31416#comment:27>