Cannot Modify Foreign Keys Within Data Migrations

[Jeff Singer]

The following data migration works in 1.7, but fails in 1.8a1

def forwards(apps, schema_editor):
    A = apps.get_model('app_name', 'A')
    B = apps.get_model('app_name', 'B')
    
    a = A.objects.create()
    b = B(a=a)

Fails with:

    ValueError: Cannot assign "<A: A object>": "B.a" must be a "A" instance.

I believe that this is because the model classes returned apps.get_model aren't necessarily instances of the right type.

I'd be happy to create a failing test case, however, I'm new to the project, and would like a little direction on where that should go.

[Claude Paroz]

This is surely a consequence of the migration optimizations introduced in 1.8. One possible resolution would be to ditch the isinstance check and replace it by a same_model comparison utility which would compare models using their _meta app_label/model_name combination.

[Andriy Sokolovskiy]

I'll create patch for this

[Andriy Sokolovskiy]

test to reproduce the issue (but it is passing correct)

[Andriy Sokolovskiy]

Can not reproduce.
Latest commit on 1.8.x branch locally was
​https://github.com/django/django/commit/fc1e9107d7d750f6eed3c8e679dfe96af8f05150

I created test project and similar datamigration runs correct.
I also created a test for it (see prev. comment), and it is passing.
Maybe, I'm missing something?

[Tim Graham]

I also couldn't reproduce using the models from the tutorial. Jeff, could you provide more details?

def forwards(apps, schema_editor):
    Poll = apps.get_model('polls', 'Poll')
    Choice = apps.get_model('polls', 'Choice')

    a = Poll.objects.create(question='foo bar', pub_date=timezone.now())
    b = Choice(poll=a)

[Jeff Singer]

Unfortunately, I can't share the exact setup, because it's a project from work.

The main difference I can see is between your example and my actual situation, is that the models are from different apps.

I should have some time tomorrow night to try to create a test case for this.

[Tim Graham]

Jeff, could you test with the patch from ​PR 4097 and see if that solves this issue?

[Jeff Singer]

Still fails unfortunately when tested against that patch.

I am having trouble creating a failing test case though. The PR definitely seems related.

Here's a few things I've noticed since today:

1. It's not actually a ForeignKey, but the forward end of a OneToOneField. I'm doubting that matters, but I figured I'd throw that out there.
2. The repr of type(a) and field.rel.to are the same, however, the id of them is not the same.
3. The id of type(a) is the same as the one in the apps.all_models dictionary.
4. Stepping through several migrations of the same app, the id(type(a)) is different each time, which makes sense, as the model is changing. However, id(field.rel.to) does not match the current version of a, but one from a previous migration.

Hope that helps a little bit, in the mean time, I'll keep trying to get a testcase that reproduces this.

[Jeff Singer]

Failing Test

[Jeff Singer]

I've created a test that fails.

It creates two models, in two seperate apps, and then alters the one with a ForeignKey pointing at it before running the DataMigration.

    def inner_method(models, schema_editor):
        Author = models.get_model("test_authors", "Author")
        Book = models.get_model("test_books", "Book")
        author = Author.objects.create(name="Hemingway")
        book = Book("Old Man and The Sea")
        book.author = author

    create_author = migrations.CreateModel(
        "Author",
        [
            ("id", models.AutoField(primary_key=True)),
            ("name", models.CharField(max_length=100)),
        ],
        options={},
    )
    create_book = migrations.CreateModel(
        "Book",
        [
            ("id", models.AutoField(primary_key=True)),
            ("title", models.CharField(max_length=100)),
            ("author", models.ForeignKey("test_authors.Author"))
        ],
        options={},
    )
    add_hometown = migrations.AddField(
        "Author",
        "hometown",
        models.CharField(max_length=100),
    )
    create_old_man = migrations.RunPython(
        inner_method, inner_method
    )

    project_state = ProjectState()
    with connection.schema_editor() as editor:
        new_state = project_state.clone()
        create_author.state_forwards("test_authors", new_state)
        create_author.database_forwards("test_authors", editor, project_state, new_state)
        project_state, new_state = new_state, new_state.clone()
        create_book.state_forwards("test_books", new_state)
        create_book.database_forwards("test_books", editor, project_state, new_state)
        project_state, new_state = new_state, new_state.clone()
        add_hometown.state_forwards("test_authors", new_state)
        add_hometown.database_forwards("test_authors", editor, project_state, new_state)
        project_state, new_state = new_state, new_state.clone()
        create_old_man.state_forwards("test_books", new_state)
        create_old_man.database_forwards("test_books", editor, project_state, new_state)

[Markus Holtermann]

Thanks Jeff. I added your test case to the pull request. Before applying the patch (4th commit in the PR right now), your tests fails, after my patch it succeeds. Waiting for CI confirmation right now.

[Markus Holtermann <info@…>]

In 273bc4b667b964b69b15bc438bcdae3dc6529a2a:

Refs #24282 -- Added failing test case for assigning models of wrong type to FK

Thanks Jeff Singer for the test case.

[Markus Holtermann <info@…>]

In b29f3b51204d53c1c8745966476543d068c173a2:

Fixed #24225, #24264, #24282 -- Rewrote model reloading in migration project state

Instead of naively reloading only directly related models (FK, O2O, M2M
relationship) the project state needs to reload their relations as well
as the model changes as well. Furthermore inheriting models (and super
models) need to be reloaded in order to keep inherited fields in sync.

To prevent endless recursive calls an iterative approach is taken.

[Markus Holtermann <info@…>]

In 4e9ecfee7759f365dd75dbf89df0ee9a64ef50aa:

[1.8.x] Refs #24282 -- Added failing test case for assigning models of wrong type to FK

Thanks Jeff Singer for the test case.

Backport of 273bc4b667b964b69b15bc438bcdae3dc6529a2a from master

[Markus Holtermann <info@…>]

In a1ba4627931591b80afa46e38e261f354151d91a:

[1.8.x] Fixed #24225, #24264, #24282 -- Rewrote model reloading in migration project state

Instead of naively reloading only directly related models (FK, O2O, M2M
relationship) the project state needs to reload their relations as well
as the model changes as well. Furthermore inheriting models (and super
models) need to be reloaded in order to keep inherited fields in sync.

To prevent endless recursive calls an iterative approach is taken.

Backport of b29f3b51204d53c1c8745966476543d068c173a2 from master

[Ubica]

I am getting this same issue in Django==2.2.10 as well as 2.2.17

When using models obtained from apps.get_model they are returned as <class 'fake.ModelName'> and if I try to assign it to a ForeignKey field that causes a ValueError Exception:

ValueError: Cannot assign "<ModelName: ModelName object (10232)>": "ModelName.field_name" must be a "ModelName" instance.

This can be avoided by using PKs instead, but I am not sure if it's supposed to work with objects as well.

instance.foreign_key_field_id = instance.pk

[Carlton Gibson]

@Ubica, please don’t re-open old issues.

If you believe there is a bug, please open a new issue, with a complete description, that will enable us to reproduce and assess the report.

Please note that Django v2.2 no longer receives bug fixes, as it is in extended support, and so ideally your report will reproduce against Django’s master branch. Thanks.