Opened 10 years ago

Closed 4 years ago

#24282 closed Bug (fixed)

Cannot Modify Foreign Keys Within Data Migrations

Reported by: Jeff Singer Owned by: Markus Holtermann
Component: Migrations Version: 1.8alpha1
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (2)

ticket_24282.patch (2.3 KB ) - added by Andriy Sokolovskiy 10 years ago.
test to reproduce the issue (but it is passing correct)
patch_failing_test (2.6 KB ) - added by Jeff Singer 10 years ago.
Failing Test

Download all attachments as: .zip

Change History (18)

comment:1 by Claude Paroz, 10 years ago

Triage Stage: UnreviewedAccepted

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.

comment:2 by Andriy Sokolovskiy, 10 years ago

Owner: changed from nobody to Andriy Sokolovskiy
Status: newassigned

I'll create patch for this

by Andriy Sokolovskiy, 10 years ago

Attachment: ticket_24282.patch added

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

comment:3 by Andriy Sokolovskiy, 10 years ago

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?

comment:4 by Tim Graham, 10 years ago

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)

comment:5 by Jeff Singer, 10 years ago

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.

comment:6 by Andriy Sokolovskiy, 10 years ago

Owner: Andriy Sokolovskiy removed
Status: assignednew

comment:7 by Tim Graham, 10 years ago

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

comment:8 by Jeff Singer, 10 years ago

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.

by Jeff Singer, 10 years ago

Attachment: patch_failing_test added

Failing Test

comment:9 by Jeff Singer, 10 years ago

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)
Last edited 10 years ago by Jeff Singer (previous) (diff)

comment:10 by Markus Holtermann, 10 years ago

Has patch: set
Owner: set to Markus Holtermann
Status: newassigned

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.

comment:11 by Markus Holtermann <info@…>, 10 years ago

In 273bc4b667b964b69b15bc438bcdae3dc6529a2a:

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

Thanks Jeff Singer for the test case.

comment:12 by Markus Holtermann <info@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

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.

comment:13 by Markus Holtermann <info@…>, 10 years ago

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

comment:14 by Markus Holtermann <info@…>, 10 years ago

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

comment:15 by Ubica, 4 years ago

Resolution: fixed
Status: closednew

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

comment:16 by Carlton Gibson, 4 years ago

Resolution: fixed
Status: newclosed

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

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