Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#26384 closed Bug (fixed)

Migrations crash renaming the pk of a model with a self-referential foreign key on SQLite

Reported by: Brian May Owned by: nobody
Component: Migrations Version: 1.9
Severity: Release blocker Keywords:
Cc: Alex Hill Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Akshesh Doshi)

With open source project called spud.

Generates Exception:

FieldDoesNotExist: album has no field named u'album_id'

This field was removed. However in the migration that is being run when it fails, it did exist. So as a result, I suspect this isn't #26180.

The migration in question attempts to rename the album_id field to id.

Works fine with Django < =1.8

Stack Trace:

/usr/lib/python2.7/dist-packages/pytest_django/fixtures.py:54: in _django_db_setup
    interactive=False)
/tmp/deleteme/local/lib/python2.7/site-packages/django/test/runner.py:726: in setup_databases
    serialize=connection.settings_dict.get("TEST", {}).get("SERIALIZE", True),
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/backends/base/creation.py:70: in create_test_db
    run_syncdb=True,
/tmp/deleteme/local/lib/python2.7/site-packages/django/core/management/__init__.py:119: in call_command
    return command.execute(*args, **defaults)
/tmp/deleteme/local/lib/python2.7/site-packages/django/core/management/base.py:399: in execute
    output = self.handle(*args, **options)
/tmp/deleteme/local/lib/python2.7/site-packages/django/core/management/commands/migrate.py:200: in handle
    executor.migrate(targets, plan, fake=fake, fake_initial=fake_initial)
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/migrations/executor.py:92: in migrate
    self._migrate_all_forwards(plan, full_plan, fake=fake, fake_initial=fake_initial)
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/migrations/executor.py:121: in _migrate_all_forwards
    state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial)
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/migrations/executor.py:198: in apply_migration
    state = migration.apply(state, schema_editor)
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/migrations/migration.py:123: in apply
    operation.database_forwards(self.app_label, schema_editor, old_state, project_state)
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/migrations/operations/fields.py:275: in database_forwards
    to_model._meta.get_field(self.new_name),
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/backends/base/schema.py:482: in alter_field
    old_db_params, new_db_params, strict)
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/backends/sqlite3/schema.py:245: in _alter_field
    self._remake_table(model, alter_fields=[(old_field, new_field)])
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/backends/sqlite3/schema.py:181: in _remake_table
    self.create_model(temp_model)
/tmp/deleteme/local/lib/python2.7/site-packages/django/db/backends/base/schema.py:250: in create_model
    to_column = field.remote_field.model._meta.get_field(field.remote_field.field_name).column

Actual error is in get_field() at

/tmp/deleteme/local/lib/python2.7/site-packages/django/db/models/options.py:582: FieldDoesNotExist

The migration that appears to be causing the problems (wish I could tell which step was failing):

# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import models, migrations


class Migration(migrations.Migration):

    dependencies = [
        ('spud', '0003_auto_20141217_0908'),
    ]

    operations = [
        migrations.AlterModelOptions(
            name='photo',
            options={'ordering': ['datetime', 'id']},
        ),
        migrations.RenameField(
            model_name='album',
            old_name='album_id',
            new_name='id',
        ),
        migrations.RenameField(
            model_name='category',
            old_name='category_id',
            new_name='id',
        ),
        migrations.RenameField(
            model_name='person',
            old_name='person_id',
            new_name='id',
        ),
        migrations.RenameField(
            model_name='photo',
            old_name='photo_id',
            new_name='id',
        ),
        migrations.RenameField(
            model_name='place',
            old_name='place_id',
            new_name='id',
        ),
    ]

Attachments (1)

rename.tar.gz (880 bytes) - added by Tim Graham 5 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 5 years ago by Brian May

Code that causes this failure on github.

comment:2 Changed 5 years ago by Brian May

Looks like the above migration fails for sqlite (above error), fails for mysql (different error), but works for postgresql. So to try and summarize:

I said earlier this works for Python3.4, doesn't appear to be the case any more; it fails regardless of Python version. So maybe I missed up the earlier test.

So to get this fail:

  • Use mysql (different error) OR
  • Use sqlite and Django >= 1.9 (error quoted above)

Postgresql appears to work with any Django version. So maybe I somehow accidentally made the migration postgresql specific???

Test results available here: https://travis-ci.org/brianmay/spud/builds/117384657

comment:3 Changed 5 years ago by Brian May

Suspect the failure on mysql is that mysql doesn't like having the primary key renamed.

The failure with sqlite looks like some sort of regression in Django 1.9. No idea if is it feasible to fix or not.

Apologies if any of this was documented somewhere and I missed it...

I plan to work around this for now by squashing the migrations and removing the rename.

comment:4 Changed 5 years ago by Tim Graham

Severity: NormalRelease blocker
Summary: migrations fail with Python 2.7 and Django > 1.9Django 1.9 regression in migrations due to lazy model operations refactor
Triage Stage: UnreviewedAccepted

Bisected the behavior change to 720ff740e70e649a97fcf0232fec132569a52c7e. Haven't confirmed it's a bug in Django and not in the application but accepting for further investigation.

If you could create a more minimal set of steps to reproduce the issue, that would be helpful.

comment:5 Changed 5 years ago by Tim Graham

I'm attaching a minimal project to reproduce. Add the rename app to a project and run python manage.py test rename and you should see FieldDoesNotExist: Foo has no field named u'album_id' (reproducible with SQLite only). If someone can propose a fix, I don't mind spending more time trying to write a regression test but as I'm not sure where the fix lies, I'm not sure at what level to write a test.

Changed 5 years ago by Tim Graham

Attachment: rename.tar.gz added

comment:6 Changed 5 years ago by Tim Graham

Cc: Alex Hill added

Alex, could you investigate as you're the author on the patch where the behavior changed?

comment:7 Changed 5 years ago by Alex Hill

Sure. I'm away for Easter weekend with just phone, I'll have a look next week.

I reckon it will have something to do with the issues described in https://github.com/django/django/pull/4122

comment:8 Changed 5 years ago by Akshesh Doshi

Description: modified (diff)

comment:9 Changed 5 years ago by Alex Hill

Has patch: set

The model in Tim's test case looks like this:

class Foo(models.Model):
    # album_id = models.AutoField(primary_key=True) (renamed to `id` in migration 2)
    parent = models.ForeignKey('self', null=True, blank=True)

The problematic migration wants to rename Foo.album_id to Foo.id. To perform this kind of schema alteration under SQLite, we have to create a new model with updated fields, and from that model make a new table, copy the data, then remove the old table. The action happens in db.backends.sqlite3.schema.DatabaseSchemaEditor._remake_table().

The bug occurs because only the AutoField is changed, while the ForeignKey is considered not to have changed and is copied directly from the pre-migration model. In fact the ForeignKey, in its remote_field.field_name, holds a copy of the AutoField's field name, so it needs to be updated too to ensure the temporary model is internally consistent.

To fix the bug we can re-create a model's self-referential related fields using Field.clone() rather than just using the old model's field instances.

I've opened a PR at https://github.com/django/django/pull/6355.

comment:10 Changed 5 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:11 Changed 5 years ago by Tim Graham

Summary: Django 1.9 regression in migrations due to lazy model operations refactorMigrations crash renaming the pk of a model with a self-referential foreign key

comment:12 Changed 5 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 4b2cf1c:

Fixed #26384 -- Fixed renaming the PK on a model with a self-referential FK on SQLite.

comment:13 Changed 5 years ago by Tim Graham <timograham@…>

In ed87af3:

[1.9.x] Fixed #26384 -- Fixed renaming the PK on a model with a self-referential FK on SQLite.

Backport of 4b2cf1cd27587a30b3b081091627d7ee13141afe from master

comment:14 Changed 5 years ago by Claude Paroz

Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted

This appears to cause failures with MySQL (5.5?): http://djangoci.com/job/django-master/

comment:15 Changed 5 years ago by Alex Hill

Hi Claude,

Interesting...my best guess is that this is a separate problem exposed by the test that was added in this patch, since the only code changes in the patch were to the SQLite schema editor. The original report bears that out:

So to get this fail:

  • Use mysql (different error) OR
  • Use sqlite and Django >= 1.9 (error quoted above)

I don't have MySQL set up, but this will be useful: http://stackoverflow.com/questions/1457305/mysql-creating-tables-with-foreign-keys-giving-errno-150

That said, the pull request passed all the tests under all databases. How could that have happened?

Cheers,
Alex

comment:16 Changed 5 years ago by Alex Hill

I think this is the problem:

Prior to MySQL 5.6.6, adding and dropping a foreign key in the same ALTER TABLE statement may be problematic in some cases and is therefore unsupported. Separate statements should be used for each operation. As of MySQL 5.6.6, adding and dropping a foreign key in the same ALTER TABLE statement is supported for ALTER TABLE ... ALGORITHM=INPLACE but remains unsupported for ALTER TABLE ... ALGORITHM=COPY.

From http://dev.mysql.com/doc/refman/5.6/en/alter-table.html

comment:17 in reply to:  15 Changed 5 years ago by Claude Paroz

That said, the pull request passed all the tests under all databases. How could that have happened?

Because the main pull request builder is using a recent MySQL (unless you ask the buildbot to test on precise).

It's true that you probably revealed an existing bug with older MySQL versions. Do you have an idea for fixing this one?

comment:18 Changed 5 years ago by Tim Graham

I think the MySQL issue is #24995 ("MySQL error when renaming a primary key"). There is a patch there but I closed the PR because the test didn't act as a regression test for me. Now I see that maybe I needed to test with an older MySQL. The patch there still doesn't solve the issue but it changes the exception on the test added in this ticket to something within Django AttributeError: 'AutoField' object has no attribute 'model' instead of something from MySQL: OperationalError: (1025, "Error on rename of './test_django/#sql-58c_94' to './test_django/schema_node' (errno: 150)"). I think we can continue the discussion on that ticket.

comment:19 Changed 5 years ago by Tim Graham <timograham@…>

In f3595b2:

Refs #26384, #24995 -- Skipped a schema test on older MySQL versions.

comment:20 Changed 5 years ago by Tim Graham

Resolution: fixed
Status: newclosed
Summary: Migrations crash renaming the pk of a model with a self-referential foreign keyMigrations crash renaming the pk of a model with a self-referential foreign key on SQLite

comment:21 Changed 5 years ago by Tim Graham <timograham@…>

In a0e3cbaa:

[1.9.x] Refs #26384, #24995 -- Skipped a schema test on older MySQL versions.

Backport of f3595b25496691966d4ff858a3b395735ad85a6e from master

comment:22 Changed 5 years ago by Tim Graham <timograham@…>

In d81d02d:

Refs #26384, #24995 -- Avoided a module-level MySQL query in the schema tests.

comment:23 Changed 5 years ago by Tim Graham <timograham@…>

In 5c1944f9:

[1.9.x] Refs #26384, #24995 -- Avoided a module-level MySQL query in the schema tests.

Backport of d81d02d449edd046a94de5f171f4ae87fa331c7d from master

comment:24 Changed 4 years ago by Simon Charette <charette.s@…>

In 96181080:

Refs #26384 -- Isolated a test model in schema tests.

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