Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#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)

demoproject.zip (10.3 KB ) - added by tbartelmess 10 years ago.
Demo Project

Download all attachments as: .zip

Change History (11)

by tbartelmess, 10 years ago

Attachment: demoproject.zip added

Demo Project

comment:1 by Simon Charette, 10 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned
Triage Stage: UnreviewedAccepted

I could reproduce with on master and 1.7.X. Looking at it.

comment:2 by Simon Charette, 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 ModelStates 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.

in reply to:  2 comment:3 by faldridge, 10 years ago

@charettes. I encountered this problem also, and your patch resolved the issue for me.

comment:4 by faldridge, 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.

Last edited 10 years ago by faldridge (previous) (diff)

comment:5 by Simon Charette, 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.

  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?

comment:6 by Simon Charette, 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 Andrew Godwin, 10 years ago

Triage Stage: AcceptedReady for checkin

I've reviewed the PR and the first commit looks like a good fix.

comment:8 by Simon Charette, 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 Simon Charette <charette.s@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 7a38f889222dfbdf0e0d8d22001c30264d420054:

Fixed #22659 -- Prevent model states from sharing field instances.

Thanks to Trac alias tbartelmess for the report and the test project.

comment:10 by Simon Charette <charette.s@…>, 10 years ago

In 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

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