Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#22659 closed Bug (fixed)

parent_link relationships ignored by migrations

Reported by: tbartelmess Owned by: charettes
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 months ago.
Demo Project

Download all attachments as: .zip

Change History (11)

Changed 10 months ago by tbartelmess

Demo Project

comment:1 Changed 10 months ago by charettes

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to charettes
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 follow-up: Changed 10 months ago by charettes

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

comment:3 in reply to: ↑ 2 Changed 10 months ago by faldridge

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

comment:4 Changed 10 months ago 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 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 months ago by faldridge (previous) (diff)

comment:5 Changed 9 months ago by charettes

  • 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 Changed 9 months ago by charettes

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 Changed 9 months ago by andrewgodwin

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:8 Changed 9 months ago 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.

comment:9 Changed 9 months ago by Simon Charette <charette.s@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 Changed 9 months ago by 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

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