Opened 2 years ago

Last modified 6 months ago

#25247 new Cleanup/optimization

makemigrations unable to generate necessary migration for making a superclass abstract

Reported by: David Sanders Owned by: nobody
Component: Migrations Version: 1.8
Severity: Normal Keywords: abstract makemigrations
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If a user has an existing model state that uses model inheritance and changes a superclass to become abstract then makemigrations is unable to produce a suitable migration. In fact the resulting migration will actually raise an exception when run.

This is non-trivial as a generated migration may require some data migration operations.

This was reported by a user on #django and I was able to reproduce with a small test:

# Initial model state:

class Foo(models.Model):
    name = models.CharField(max_length=255)


class Bar(Foo):
    pass


# Subsequent model state:

class Foo(models.Model):
    name = models.CharField(max_length=255)

    class Meta:
        abstract=True


class Bar(Foo):
    pass


# running makemigrations results in:

class Migration(migrations.Migration):

    dependencies = [
        ('foobar', '0001_initial'),
    ]

    operations = [
        migrations.RemoveField(
            model_name='bar',
            name='foo_ptr',
        ),
        migrations.AddField(
            model_name='bar',
            name='id',
            field=models.AutoField(auto_created=True, primary_key=True, default=1, serialize=False, verbose_name='ID'),
            preserve_default=False,
        ),
        migrations.AddField(
            model_name='bar',
            name='name',
            field=models.CharField(default='asdf', max_length=255),
            preserve_default=False,
        ),
        migrations.DeleteModel(
            name='Foo',
        ),
    ]


# which results in the following when attempting to run migrate:

(env)dsanders ~/test/abstract_update/test_abstract $ ./manage.py migrate
Operations to perform:
  Synchronize unmigrated apps: staticfiles, messages
  Apply all migrations: admin, foobar, contenttypes, auth, sessions
Synchronizing apps without migrations:
  Creating tables...
    Running deferred SQL...
  Installing custom SQL...
Running migrations:
  Rendering model states...Traceback (most recent call last):
  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/core/management/__init__.py", line 338, in execute_from_command_line
    utility.execute()
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/core/management/__init__.py", line 330, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/core/management/base.py", line 393, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/core/management/base.py", line 444, in execute
    output = self.handle(*args, **options)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/core/management/commands/migrate.py", line 221, in handle
    executor.migrate(targets, plan, fake=fake, fake_initial=fake_initial)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/db/migrations/executor.py", line 104, in migrate
    state = migration.mutate_state(state, preserve=do_run)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/db/migrations/migration.py", line 83, in mutate_state
    operation.state_forwards(self.app_label, new_state)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/db/migrations/operations/fields.py", line 51, in state_forwards
    state.reload_model(app_label, self.model_name_lower)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/db/migrations/state.py", line 152, in reload_model
    self.apps.render_multiple(states_to_be_rendered)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/db/migrations/state.py", line 262, in render_multiple
    model.render(self)
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/db/migrations/state.py", line 546, in render
    body,
  File "/Users/dsanders/test/abstract_update/env/lib/python2.7/site-packages/django/db/models/base.py", line 254, in __new__
    'base class %r' % (field.name, name, base.__name__)
django.core.exceptions.FieldError: Local field u'id' in class 'Bar' clashes with field of similar name from base class 'Foo'

Change History (6)

comment:1 Changed 2 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

I guess it's probably not feasible to allow makemigrations to generate a correct migration in cases like this (e.g. auto-generating data migration seems risky), but we could at least throw a more helpful error message or add some documentation about how to handle this case to docs/howto/writing-migrations.txt.

comment:2 Changed 2 years ago by Simon Charette

I agree that generating a data migration might be risky but we could at least try do express a OneToOneField(auto_created=True, primary_key=True) -> AutoField(auto_created=True, primary_key=True) change as an AlterField operation.

comment:3 Changed 2 years ago by Markus Holtermann

Even that is not really feasible, Simon, as PostgreSQL will automatically populate the new id column with sequence values, potentially changing the order of the result set when ordering by primary key.

comment:4 Changed 2 years ago by Simon Charette

@MarkusH, I think it shouldn't be an issue if the sequence is started at the referenced field's sequence current value:

CREATE TABLE foo (
    id serial primary key
);

CREATE TABLE bar (
    foo_ptr_id integer primary key REFERENCES foo (id)
);

INSERT INTO foo DEFAULT VALUES;
INSERT INTO foo DEFAULT VALUES;
INSERT INTO foo DEFAULT VALUES;

SELECT * FROM foo;
 id 
----
  1
  2
  3
(3 rows)

INSERT INTO bar SELECT id FROM foo;

SELECT * FROM bar;
 foo_ptr_id 
------------
          1
          2
          3
(3 rows)

CREATE SEQUENCE bar_id_seq;
SELECT setval('bar_id_seq', currval('foo_id_seq'));
ALTER TABLE bar RENAME COLUMN foo_ptr_id TO id;
ALTER TABLE bar ALTER COLUMN id SET DEFAULT nextval('bar_id_seq');
ALTER TABLE bar DROP CONSTRAINT bar_foo_ptr_id_fkey;

INSERT INTO bar DEFAULT VALUES;
INSERT INTO bar DEFAULT VALUES;

SELECT * FROM bar;
 id 
----
  1
  2
  3
  4
  5
(5 rows)
Last edited 2 years ago by Simon Charette (previous) (diff)

comment:5 Changed 2 years ago by Simon Charette

It looks like it's only a matter of teaching the auto detector to consider auto_created primary keys changes as automatic rename and alters since most the integer to serial alteration logic is already implemented.

comment:6 Changed 6 months ago by Tim Graham

#27940 and #27941 are duplicates.

Note: See TracTickets for help on using tickets.
Back to Top