#31416 closed Bug (fixed)
FieldError when migrating field to new model subclass.
Reported by: | Eduard Anghel | Owned by: | Nan Liu |
---|---|---|---|
Component: | Migrations | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | me@…, Markus Holtermann, Simon Charette, Nan Liu | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (27)
comment:1 by , 4 years ago
Cc: | added |
---|---|
Summary: | FieldError when migrating field to new model subclass → FieldError when migrating field to new model subclass. |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 4 years ago
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?
comment:3 by , 4 years ago
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.
comment:4 by , 4 years ago
would this be something doable for a new beginner like me? much appreciated!
should I just display some message or warning like "the migration can't be applied unless some manual editing takes place" whenever this happens? or do I give the user some sort of yes-no option so that they can choose to makemigration or not? or it is appropriate to allow auto-detector to use the reversed order when encountering such a situation?
or I think this may be more appropriate: we may need to allow auto-detector to use the reversed or correct order (that doesn't throw an error) when encountering such a situation as well as display some sort of message or warning along the way since the message may educate people who want to take action like this.
comment:5 by , 4 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
follow-up: 7 comment:6 by , 4 years ago
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 and not multiple ones.
follow-up: 8 comment:7 by , 4 years ago
Replying to 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
comment:8 by , 4 years ago
Replying to Sanskar Jaiswal:
Replying to 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, appendbase_app_label, base_name, field_name, False
todependencies
, how would adjustingcheck_dependency
able to reorder the migrations?
Thanks
Sanskar
hi Sanskar, I am still working on this issue. it may take a longer time for me to fix it since I am a newbie. since this is for my school assignment, may you work on other issues or if we can solve it together? I really appreciate that!
And I also had the same issue. Since in this case, we want to Remove_field
before Create_model
, but the migration list is ordered in _sort_migrations
. How does check_dependency
reorder the migration list so that it can solve this issue?
follow-up: 10 comment:9 by , 4 years ago
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
follow-up: 11 comment:10 by , 4 years ago
Replying to 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 readable
. I was able to find title
field from a sub class Book
using model_state.fields
. 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!
Cheers,
Nan
follow-up: 12 comment:11 by , 4 years ago
And I tried
model_state = self.to_state.models[base_app_label, base_model_name]
to findreadable
's fields, but there is only one field left which is the self-generatedid
field. Not sure why the fieldtitle
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 isreadable
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).
comment:12 by , 4 years ago
Replying to Sanskar Jaiswal:
And I tried
model_state = self.to_state.models[base_app_label, base_model_name]
to findreadable
's fields, but there is only one field left which is the self-generatedid
field. Not sure why the fieldtitle
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 isreadable
in this case. much appreciated!
Since we remove
title
fromReadable
, it doesn't show up inself.to_state.models[base_app_label, base_model_name]
. Doing this gives you, the fields of our latest intended model, in whichReadable
is just an empty model with oneAutoField
as the "self generatedid
." To get the intended fields ofReadable
, you need to useself.from_state.models[base_app_label, base_model_name]
sincefrom_state
refers to the previous state, i.e. in this case, just oneReadable
model with aCharField
namedtitle
(and theid
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!
follow-up: 14 comment:13 by , 4 years ago
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!
And I am still trying to adjust the dependencies...
follow-up: 16 comment:15 by , 4 years ago
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.
comment:16 by , 4 years ago
Replying to 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_autoencoder.py
.
When I do local testings, two separate migrations(one is to create Readable
with fields title
and name
, second one is to remove fields from Readable
and add these two fields into inheriting models like Book
, Magazine
), will solve the issue. But if i do self.get_change([Readable], [Readable_with_no_fields, Book, Magazine])
, the test case tells me that models are created before base fields are removed, which is different from what I have on local testings according to the console message.
Migrations for 'test': test/migrations/0002_auto_20200418_1007.py - Remove field name from readable - Remove field title from readable - Create model Book - Create model Magazine
In this case, it should've been ["RemoveField", "RemoveField", "CreateModel", "CreateModel"]
, but currently it is ["CreateModel", "CreateModel", "RemoveField", "RemoveField"]
. How can I create one migration for Readable
first and then create the second one for the updated models? Since the problem is that we need the first migration to be associated with the second one.
follow-up: 18 comment:17 by , 4 years ago
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.
comment:18 by , 4 years ago
Replying to 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 andBook
with thetitle
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.
Hi Adam,
After I spent hours and hours on this, I found out keyword tuples (app_label_name, name)
are supposed to be lower case when we access it using self.from_state.models
and self.from_state.models
...
comment:19 by , 4 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:20 by , 4 years ago
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!!
follow-up: 22 comment:21 by , 4 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
Nan Liu, please don't close tickets that are not fixed. We first need to have, review and merge patch.
comment:22 by , 4 years ago
Replying to 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
comment:24 by , 4 years ago
Status: | new → assigned |
---|
comment:25 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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.