FieldError when migrating field to new model subclass.

[Eduard Anghel]

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.

[Mariusz Felisiak]

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.

[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?

[Adam 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.

[Nan Liu]

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.

[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 and not multiple ones.

[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, append base_app_label, base_name, field_name, False to dependencies, how would adjusting check_dependency able to reorder the migrations?

Thanks
Sanskar

[Nan Liu]

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, 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. Sorry about not posting my progress here! it may take a longer time for me to fix it since I am a newbie, so i gotta read the code. since this is for my school assignment, may you work on other issues or if it would be much better if we can solve it together so that we both get credits for it? This is really important for me, since the school assignment requires me to do some actual contribution to the project, and I have spent a fair amount of time reading the code and testing it. I really appreciate that!

And I think the operations for you weren't swapped maybe because you didn't add a dependency of the removed field for all its bases? And also adjusting check_dependency seems unnecessary since it already handles everything well from my testing.

I was able to swap the operations when I appended a manual dependency on field removal of all of its bases in generate_created_models. If you don't mind me asking, how am I able to know that is the field we are tryna remove? I am reading the code currently, but I will still leave this "dumb" question here just in case I don't find it by the time.

In addition, in terms of possible duplicate Remove_fields, I wasn't able to generate multiple same Remove_fields for title, so I guess this is already handled somewhere in the code?

[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

[Nan Liu]

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

[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).

[Nan Liu]

Replying to 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!

[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!

And I am still trying to adjust the dependencies...

[Mariusz Felisiak]

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.

[Nan Liu]

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.

[Adam 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.

[Nan Liu]

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 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.

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...

[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!!

[Mariusz Felisiak]

Nan Liu, please don't close tickets that are not fixed. We first need to have, review and merge patch.

[Nan Liu]

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

[Mariusz Felisiak]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 33c36578:

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.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 114da2d0:

[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