#22659 closed Bug (fixed)
parent_link relationships ignored by migrations
Reported by: | tbartelmess | Owned by: | Simon Charette |
---|---|---|---|
Component: | Migrations | Version: | 1.7-beta-2 |
Severity: | Release blocker | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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
Attachments (1)
Change History (11)
by , 10 years ago
Attachment: | demoproject.zip added |
---|
comment:1 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
I could reproduce with on master and 1.7.X. Looking at it.
follow-up: 3 comment:2 by , 10 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
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 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.
comment:3 by , 10 years ago
@charettes. I encountered this problem also, and your patch resolved the issue for me.
comment:4 by , 10 years ago
@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 inside a VM with 768MB of memory. I had to kick up the RAM to finish applying initial migrations to my project. 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.
comment:5 by , 10 years ago
Patch needs improvement: | unset |
---|
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.
- Prevent cross-app models references. I guess we could add this in
RelatedField.contribute_to_class
. - 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?
comment:6 by , 10 years ago
Prevent cross-app models references. I guess we could add this in RelatedField.contribute_to_class.
I went ahead and gave this a try.
I had to tweak the SQLite3 DatabaseSchemaEditor._remake_table
method but I got the full test suite passing.
comment:7 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I've reviewed the PR and the first commit looks like a good fix.
comment:8 by , 10 years ago
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.
comment:9 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Demo Project