Opened 10 years ago

Last modified 4 weeks ago

#23521 assigned Bug

removal of concrete Model from bases doesn't remove it from ModelState bases

Reported by: Sergey Fedoseev Owned by: Tom L.
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: info+coding@…, bugs@…, ericmills2@…, Charlie Denton, Ian Foote, Daniel Rios, Sardorbek Imomaliev Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Markus Holtermann)

Steps to reproduce:

  • add test app with models.py:
    class Thing(models.Model):
        pass
    
    
    class SuperThing(Thing):
        pass
    
  • do
manage.py makemigrations test
  • change models.py (remove Thing):
    class SuperThing(models.Model):
        thing_ptr = models.AutoField(primary_key=True)
    
  • do
manage.py makemigrations test
  • do it one more time
manage.py makemigrations test

last command results in

django.db.migrations.state.InvalidBasesError: Cannot resolve bases for [<ModelState: 'test.SuperThing'>]
This can happen if you are inheriting models from an app with migrations (e.g. contrib.auth)
 in an app with no migrations; see https://docs.djangoproject.com/en/1.7/topics/migrations/#dependencies for more

Change History (32)

comment:1 by Sergey Fedoseev, 10 years ago

Component: UncategorizedMigrations
Type: UncategorizedBug

comment:2 by Baptiste Mispelon, 10 years ago

Hi,

I can't reproduce the issue you're describing.

On master, the second makemigrations doesn't detect any changes.
On stable/1.7.x, it does create a migration but running it a third time works (and detects no change as expected).

As a side note, it's not possible to create an app called test because you get this error when doing manage.py startapp test:

CommandError: 'test' conflicts with the name of an existing Python module and cannot be used as an app name. Please try another name.

Can you provide us with some details on how you're trigerring the issue?

Thanks.

in reply to:  2 comment:3 by Sergey Fedoseev, 10 years ago

Replying to bmispelon:

Hi,

Probably this is not obvious, but removal of Thing model from models.py, not only from SuperThing bases was implied.


As a side note, it's not possible to create an app called test because you get this error when doing manage.py startapp test

I have never used this for creating apps =)

comment:4 by Baptiste Mispelon, 10 years ago

Triage Stage: UnreviewedAccepted

Thanks, that's the bit I was missing.

comment:5 by Markus Holtermann, 10 years ago

Cc: info+coding@… added
Description: modified (diff)

comment:6 by Markus Holtermann, 10 years ago

Are you working on a patch sir-sigurd? If not, I'd write one.

in reply to:  6 comment:7 by Sergey Fedoseev, 10 years ago

Replying to Markush2010:

Are you working on a patch sir-sigurd? If not, I'd write one.

Yes, I'm working on it.

comment:8 by Yuval Adam, 10 years ago

There's an easier way to reproduce what seems to be the exact same error:

  • Create a BaseModel and a SubModel that inherits from it
  • Run makemigrations and migrate to add both models to database
  • Delete both models together
  • Run makemigrations and migrate to delete both models from the database

The same error should now appear.

comment:9 by ris, 9 years ago

I invite you all to look at my marked-as-duplicate bug #23818. To me it seems there is simply no way for django's migration operations to express a change of bases. parent_links are created & removed, but the built-up in-memory representation of the models never understands the new parentage, meaning certain operations (e.g. in a RunPython operation) will simply not work right when referring to inherited fields. This goes beyond makemigrations simply not detecting a change.

comment:10 by ris, 9 years ago

Cc: bugs@… added

comment:13 by Eric Mills, 8 years ago

Cc: ericmills2@… added

This bug has burned me pretty bad. I was working through steps to switch from concrete inheritance to abstract inheritance. Is someone going to revisit this?

comment:14 by Tim Graham, 8 years ago

#26488 may be related or a duplicate.

Last edited 8 years ago by Markus Holtermann (previous) (diff)

comment:15 by Federico Jaramillo Martínez, 8 years ago

Having the same issue.
I had to manually alter my initial migration and remove the offending base class for my app to work.

comment:16 by rm_, 7 years ago

I had the need as described in #23818 but instead of squashing migrations i've created a migrations operator to update the bases on migrations models state. This works fine for me on django 1.8. I still haven't tested if django master behaves differently. Do you think this is a valuable addition to django?

class AlterBaseOperation(Operation):
    reduce_to_sql = False
    reversible = True

    def __init__(self, model_name, bases, prev_bases):
        self.model_name = model_name
        self.bases = bases
        self.prev_bases = prev_bases

    def state_forwards(self, app_label, state):
        state.models[app_label, self.model_name].bases = self.bases
        state.reload_model(app_label, self.model_name)

    def state_backwards(self, app_label, state):
        state.models[app_label, self.model_name].bases = self.prev_bases
        state.reload_model(app_label, self.model_name)

    def database_forwards(self, app_label, schema_editor, from_state, to_state):
        pass

    def database_backwards(self, app_label, schema_editor, from_state, to_state):
        pass

    def describe(self):
        return "Update %s bases to %s" % (self.model_name, self.bases)

comment:19 by Charlie Denton, 5 years ago

Cc: Charlie Denton added

comment:20 by Ian Foote, 5 years ago

Cc: Ian Foote added

comment:21 by Ian Foote, 5 years ago

Owner: set to Ian Foote
Status: newassigned

comment:22 by Ian Foote, 5 years ago

Has patch: set
Last edited 5 years ago by Mariusz Felisiak (previous) (diff)

comment:23 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set
Version: 1.7master

comment:24 by Mariusz Felisiak, 5 years ago

Another use case of migrations.AlterModelBases() is described in #30513.

comment:25 by Daniel Rios, 5 years ago

Cc: Daniel Rios added

comment:26 by Karolis Ryselis, 4 years ago

There is one more case that needs to be addressed related to bases in migrations.

Step 1:
Consider this model:

class Good(GoodServiceBase):
    title = models.CharField(max_length=16)

this creates migration with the following operation:

migrations.CreateModel(
    name='Good',
    fields=[
        ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
        ('title', models.CharField(max_length=16)),
    ],
),

Step 2:
Then we add a new model and change existing model to inherit from the new model.

class GoodServiceBase(models.Model):
    active = models.BooleanField(default=True)

class Good(GoodServiceBase):
    title = models.CharField(max_length=16)

This generates migration like this:

migrations.CreateModel(
    name='GoodServiceBase',
    fields=[
        ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
        ('active', models.BooleanField(default=True)),
    ],
),
migrations.RemoveField(
    model_name='good',
    name='id',
),
migrations.AddField(
    model_name='good',
    name='goodservicebase_ptr',
    field=models.OneToOneField(auto_created=True, default=1, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='goods.GoodServiceBase'),
    preserve_default=False,
),

If we have no data in database, this will pass.

Step 3 with crash:
Try to access field active in datamigration and migration crashes. Operation in migration to reproduce this:

def migrate_my_data(apps, schema_editor):
    for good in apps.get_model("goods", "Good").objects.all():
        print(good.active)

migrations.RunPython(migrate_my_data, reverse_code=lambda x, y: None)

This crashes with the error:

<...>
File "<project home>/goods/migrations/0003_auto_20191025_1157.py", line 8, in migrate_my_data
  print(good.active)
AttributeError: 'Good' object has no attribute 'active'

As far as I understand this happens because bases is not altered in migration and fake models do not inherit fields from their true parent models. Thus, currently if we change inheritance chain of existing models, we lose the ability to use those models in data migrations.

Tested in version 2.2.6.

comment:27 by Mariusz Felisiak, 4 years ago

Another two duplicates: #31329 and #26488.

comment:28 by Mariusz Felisiak, 4 years ago

Another use case is described in #31343.

comment:29 by Ian Foote, 4 years ago

Owner: Ian Foote removed
Status: assignednew

Deassigning because I don't think I'm likely to get to this again in the near future and I'd be happy for someone else to take my work and finish the job.

comment:30 by Sardorbek Imomaliev, 4 years ago

Cc: Sardorbek Imomaliev added

comment:31 by Vadim Fabrichnov, 4 years ago

Owner: set to Vadim Fabrichnov
Status: newassigned
Last edited 4 years ago by Mariusz Felisiak (previous) (diff)

comment:32 by Vadim Fabrichnov, 3 years ago

Patch needs improvement: unset

comment:33 by Mariusz Felisiak, 3 years ago

Needs tests: set

comment:34 by Tom L., 4 weeks ago

Owner: changed from Vadim Fabrichnov to Tom L.

comment:35 by Tom L., 4 weeks ago

Needs tests: unset

comment:36 by Mariusz Felisiak, 4 weeks ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top