[Django] #22659: parent_link relationships ignored by migrations
Django
---------------------------------+------------------------
Reporter: tbartelmess | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------
The parent_link property on OneToOne relationships is ignored by the
migration system. This causes the database to end up with two parent
relationships.
For example if you have a model like this:
{{{
class Company(models.Model):
name = models.CharField(max_length=100)
class FooCompany(Company):
parent_company = models.OneToOneField(Company, primary_key = True,
parent_link=True)
}}}
without migrations the generated tables are correct
{{{
BEGIN;
CREATE TABLE "foo_company" (
"id" integer NOT NULL PRIMARY KEY AUTOINCREMENT,
"name" varchar(100) NOT NULL
)
;
CREATE TABLE "foo_foocompany" (
"parent_company_id" integer NOT NULL PRIMARY KEY REFERENCES
"foo_company" ("id")
)
;
COMMIT;
}}}
the migration system however ignores that there is a parent link set and
generate the tables like this
{{{
CREATE TABLE "foo_company" (
"id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "name" varchar(100)
NOT NULL
);
CREATE TABLE "foo_foocompany" (
"company_ptr_id" integer NOT NULL UNIQUE REFERENCES "foo_company"
("id"),
"parent_company_id" integer NOT NULL PRIMARY KEY REFERENCES
"foo_company" ("id")
);
}}}
I've categorized this as a blocker, because it makes parent_links
unusable, since the ORM won't know about the additional 'company_ptr_id'
column causing inserts to fail because of the NOT NULL violation on that
column
--
Ticket URL: <https://code.djangoproject.com/ticket/22659>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: tbartelmess | Owner: charettes
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution:
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 charettes):
* status: new => assigned
* needs_better_patch: => 0
* needs_tests: => 0
* owner: nobody => charettes
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
I could reproduce with on master and 1.7.X. Looking at it.
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:1>
Django
Reporter: tbartelmess | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
The whole issue was related to `ModelState` using their internal `fields`
representation when building model classes and not copying of them.
Since `ModelState`s can be re-used to render models in different `Apps` it
didn't detect the `parent_company` field as a valid `parent_link` the
second time the state was rendered since it was already bound to
`apps_used_on_first_render.get_model('foo.Company')` thus adding a new
`OneToOneField` pointing to
`apps_used_on_second_render.get_model('foo.Company')`.
Created a PR with an initial test
[https://github.com/django/django/pull/2703 here]. It seems to fix the
issue encountered in the provided test project on my side, can you confirm
it's also working for you @tbartelmess? I'm working on adding an
additional test case to prevent a regression for the exact issue described
here.
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:2>
Django
Reporter: tbartelmess | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by faldridge):
@charettes. I encountered this problem also, and your patch resolved the
issue for me.
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:3>
Django
Reporter: tbartelmess | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by faldridge):
@charettes: Even more interestingly, this patch also fixes a memory leak
that occurred previously when running migrations and actually lead to (I'm
not kidding) a segmentation fault. Attempting to recreate the problem in
an SSCCE might be a lot of work - especially given that this patch fixes
the problem anyway - but if you'd like to me to take the time to do so,
please let me know.
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:4>
Django
Reporter: tbartelmess | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 1 => 0
Comment:
Updated my patch with additional sanity checks. I wouldn't be surprised if
it fixed a couple of other migration tickets.
In the light of my investigation I guess we could add two additional
things to detect those kind of failures earlier.
1. Prevent cross-app models references. I guess we could add this in
`RelatedField.contribute_to_class`.
2. Add a check to make sure that `OneToOneField(parent_link=True)` of non
abstract models are pointing to one of their model parent (`self.rel.to in
self.model._meta.parents`).
Thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:5>
Django
Reporter: tbartelmess | Owner: charettes
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 charettes):
> Prevent cross-app models references. I guess we could add this in
RelatedField.contribute_to_class.
I went ahead and
[https://github.com/charettes/django/commit/fbe308b2a7b3f8a9d6cce4d63741bc764f7d9ecb
gave this a try].
I had to tweak the SQLite3 `DatabaseSchemaEditor._remake_table` method but
I got the full test suite passing.
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:6>
Django
Reporter: tbartelmess | Owner: charettes
Type: Bug | Status: assigned
Component: Migrations | Version:
Keywords: | Resolution:
Has patch: 1 | Triage Stage: Ready for
Needs tests: 1 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by andrewgodwin):
* stage: Accepted => Ready for checkin
Comment:
I've reviewed the PR and the first commit looks like a good fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:7>
Django
Reporter: tbartelmess | Owner: charettes
Type: Bug | Status: assigned
Component: Migrations | Version:
Severity: Release blocker | 1.7-beta-2
Keywords: | Resolution:
Has patch: 1 | Triage Stage: Ready for
Needs tests: 1 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charettes):
Updated the PR to remove the experimental prevention of cross-apps
references, I'll it for another ticket. I'll commit as soon as master
stabilize.
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:8>
Django
Reporter: tbartelmess | Owner: charettes
Component: Migrations | Version:
Severity: Release blocker | 1.7-beta-2
Has patch: 1 | Triage Stage: Ready for
Needs tests: 1 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"7a38f889222dfbdf0e0d8d22001c30264d420054"]:
{{{
#!CommitTicketReference repository=""
revision="7a38f889222dfbdf0e0d8d22001c30264d420054"
Fixed #22659 -- Prevent model states from sharing field instances.
Thanks to Trac alias tbartelmess for the report and the test project.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:9>
Django
Reporter: tbartelmess | Owner: charettes
Type: Bug | Status: closed
Component: Migrations | Version:
Severity: Release blocker | 1.7-beta-2
Keywords: | Resolution: fixed
Has patch: 1 | Triage Stage: Ready for
Needs tests: 1 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette <charette.s@…>):
In [changeset:"33511662ddbff0ca22cf5a0b7fdeca2f6764759f"]:
{{{
#!CommitTicketReference repository=""
revision="33511662ddbff0ca22cf5a0b7fdeca2f6764759f"
[1.7.x] Fixed #22659 -- Prevent model states from sharing field instances.
Thanks to Trac alias tbartelmess for the report and the test project.
Backport of 7a38f88922 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22659#comment:10>