Opened 9 years ago

Closed 3 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 9 years ago.
test to reproduce the issue (but it is passing correct)
patch_failing_test (2.6 KB) - added by Jeff Singer 9 years ago.
Failing Test

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by Claude Paroz

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 Changed 9 years ago by Andriy Sokolovskiy

Owner: changed from nobody to Andriy Sokolovskiy
Status: newassigned

I'll create patch for this

Changed 9 years ago by Andriy Sokolovskiy

Attachment: ticket_24282.patch added

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

comment:3 Changed 9 years ago by 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?

comment:4 Changed 9 years ago by 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)

comment:5 Changed 9 years ago by 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.

comment:6 Changed 9 years ago by Andriy Sokolovskiy

Owner: Andriy Sokolovskiy deleted
Status: assignednew

comment:7 Changed 9 years ago by Tim Graham

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

comment:8 Changed 9 years ago by 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.

Changed 9 years ago by Jeff Singer

Attachment: patch_failing_test added

Failing Test

comment:9 Changed 9 years ago by 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)
Last edited 9 years ago by Jeff Singer (previous) (diff)

comment:10 Changed 9 years ago by Markus Holtermann

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 Changed 9 years ago by 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.

comment:12 Changed 9 years ago by Markus Holtermann <info@…>

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 Changed 9 years ago by 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

comment:14 Changed 9 years ago by 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

comment:15 Changed 3 years ago by Ubica

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 Changed 3 years ago by Carlton Gibson

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