parent_link relationships ignored by migrations

[tbartelmess]

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

[tbartelmess]

Demo Project

[Simon Charette]

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

[Simon Charette]

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.

[faldridge]

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

[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 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.

[Simon Charette]

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?

[Simon Charette]

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.

[Andrew Godwin]

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

[Simon Charette]

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.

[Simon Charette <charette.s@…>]

In 7a38f889222dfbdf0e0d8d22001c30264d420054:

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

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

[Simon Charette <charette.s@…>]

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