[Django] #24282: Cannot Modify Foreign Keys Within Data Migrations
Django
---------------------------------+-----------------------
Reporter: jeffasinger | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-----------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24282>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jeffasinger | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:1>
Django
Reporter: jeffasinger | Owner: coldmind
Type: Bug | Status: assigned
Component: Migrations | Version: 1.8alpha1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* owner: nobody => coldmind
* status: new => assigned
Comment:
I'll create patch for this
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:2>
Django
Reporter: jeffasinger | Owner: coldmind
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:3>
Django
Reporter: jeffasinger | Owner: coldmind
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:4>
Django
Reporter: jeffasinger | Owner: coldmind
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jeffasinger):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:5>
Django
Reporter: jeffasinger | Owner:
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* owner: coldmind =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:6>
Django
Reporter: jeffasinger | Owner:
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Jeff, could you test with the patch from
[https://github.com/django/django/pull/4097 PR 4097] and see if that
solves this issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:7>
Django
Reporter: jeffasinger | Owner:
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jeffasinger):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:8>
Django
Reporter: jeffasinger | Owner:
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jeffasinger):
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:9>
Django
Reporter: jeffasinger | Owner: MarkusH
Type: Bug | Status: assigned
Component: Migrations | Version: 1.8alpha1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* status: new => assigned
* owner: => MarkusH
* has_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:10>
Django
Reporter: jeffasinger | Owner: MarkusH
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"273bc4b667b964b69b15bc438bcdae3dc6529a2a"]:
{{{
#!CommitTicketReference repository=""
revision="273bc4b667b964b69b15bc438bcdae3dc6529a2a"
Refs #24282 -- Added failing test case for assigning models of wrong type
to FK
Thanks Jeff Singer for the test case.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:11>
Django
Reporter: jeffasinger | Owner: MarkusH
Component: Migrations | Version: 1.8alpha1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Markus Holtermann <info@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"b29f3b51204d53c1c8745966476543d068c173a2"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:12>
Django
Reporter: jeffasinger | Owner: MarkusH
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"4e9ecfee7759f365dd75dbf89df0ee9a64ef50aa"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:13>
Django
Reporter: jeffasinger | Owner: MarkusH
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"a1ba4627931591b80afa46e38e261f354151d91a"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:14>
Django
Reporter: Jeff Singer | Owner: Markus
| Holtermann
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ubica):
* status: closed => new
* resolution: fixed =>
Comment:
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:15>
Django
Reporter: Jeff Singer | Owner: Markus
| Holtermann
Component: Migrations | Version: 1.8alpha1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => fixed
Comment:
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24282#comment:16>