Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#13987 closed (fixed)

Primary key not set correctly for concrete->abstract->concrete model inheritance.

Reported by: Aramgutang Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Keywords: sprintdec2010, inheritance, abstract, primary key
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When a concrete model inherits from an abstract model which in turn inherits from a concrete model, the "primary_key" attribute on the OneToOneField in the child model is not set, and the SQL definition for that model does not define a primary key. However, any subsequent children inheriting from the abstract class will have a correctly set primary key. For example:

class ConcreteParent(models.Model):
	concrete_field = models.TextField()

class AbstractParent(ConcreteParent):
	abstract_field = models.TextField()
	
	class Meta:
		abstract = True

class FirstChild(AbstractParent):
	child_field = models.TextField()

class SecondChild(AbstractParent):
	child_field = models.TextField()

The output of "sqlall" for these definitions is:

CREATE TABLE "testapp_concreteparent" (
    "id" serial NOT NULL PRIMARY KEY,
    "concrete_field" text NOT NULL
);
CREATE TABLE "testapp_firstchild" (
    "concreteparent_ptr_id" integer NOT NULL UNIQUE REFERENCES "testapp_concreteparent" ("id") DEFERRABLE INITIALLY DEFERRED,
    "abstract_field" text NOT NULL,
    "child_field" text NOT NULL
);
CREATE TABLE "testapp_secondchild" (
    "concreteparent_ptr_id" integer NOT NULL PRIMARY KEY REFERENCES "testapp_concreteparent" ("id") DEFERRABLE INITIALLY DEFERRED,
    "abstract_field" text NOT NULL,
    "child_field" text NOT NULL
);

Note the absence of "PRIMARY KEY" in the first child and its presence in the second child.

We can also go in the shell to see what's happening:

>>> FirstChild._meta.get_field_by_name('concreteparent_ptr')[0].primary_key
False
>>> SecondChild._meta.get_field_by_name('concreteparent_ptr')[0].primary_key
True

>>> FirstChild._meta.get_field_by_name('concreteparent_ptr')[0] == FirstChild._meta.pk
True
>>> FirstChild._meta.get_field_by_name('concreteparent_ptr')[0] is FirstChild._meta.pk
False

>>> SecondChild._meta.get_field_by_name('concreteparent_ptr')[0] == SecondChild._meta.pk
True
>>> SecondChild._meta.get_field_by_name('concreteparent_ptr')[0] is SecondChild._meta.pk
True

I believe what is happening is that in db.models.options.py, when initialising a Child object in Options._prepare(), the first parent link is selected to be the primary key, its primary key attribute is set to True, and it is then passed to setup_pk(). However, the field fetched through the parent link is the field on the AbstractParent model, and is thus not properly registered with the Child model. But in this process, its primary_key attribute is set to True, which allows the second Child to set it as its PK on the initial add_field() call.

The simplest fix for this is to initialise the OneToOneField in ModelBase.__new__() with primary_key=True. Alternately, and perhaps preferably, Options._prepare() can be altered to fetch the OneToOneField for the Child model, rather than the AbstractParent model. I've attached two patches, one for each solution, and perhaps someone more qualified than me can decide which one is the better one.

Attachments (4)

13987_primary_key_fix_1.diff (647 bytes) - added by Aramgutang 5 years ago.
1st possible fix: adding a primary_key=True to the OneToOneField initialisation.
13987_primary_key_fix_2.diff (661 bytes) - added by Aramgutang 5 years ago.
2nd possible fix: fetching the field for current model when "_preparing" it.
13987_fix_2_with_tests.diff (2.2 KB) - added by Aramgutang 5 years ago.
2nd (superior) fix variant, with regression tests.
13987_fix_2_with_tests_updated.diff (3.0 KB) - added by Aramgutang 5 years ago.
Updated patch to bring test into line with the changes in the test suite.

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by Aramgutang

1st possible fix: adding a primary_key=True to the OneToOneField initialisation.

Changed 5 years ago by Aramgutang

2nd possible fix: fetching the field for current model when "_preparing" it.

Changed 5 years ago by Aramgutang

2nd (superior) fix variant, with regression tests.

comment:1 Changed 5 years ago by Aramgutang

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

After doing some testing, it seems that the 1st variant of the fix breaks some tests, so I've attached a new diff that includes the 2nd variant of the fix (which doesn't break anything), along with regression tests for this bug.

comment:2 Changed 5 years ago by mike@…

And this kind of bug is causing much pain with something like south migrations. Is it really scheduled to be fixed in version 1.3 only?

comment:3 Changed 5 years ago by russellm

"Milestone 1.3" means that it's on the schedule to be looked at in the 1.3 development timeframe. All bugs fixed in that timeframe are backported to the previous release branch.

comment:4 Changed 5 years ago by Daishiman

  • Triage Stage changed from Unreviewed to Accepted

Tested problem description with second fix. Works as advertised.

Changed 5 years ago by Aramgutang

Updated patch to bring test into line with the changes in the test suite.

comment:5 Changed 5 years ago by julien

  • Keywords sprintdec2010 added
  • Triage Stage changed from Accepted to Ready for checkin

The new patch applies and the whole test suite passes on sqlite.

comment:6 Changed 5 years ago by russellm

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

In [15498]:

Fixed #13987 -- Ensure that the primary key is set correctly for all models that have concrete-abstract-concrete inheritance, not just the first model. Thanks to Aramgutang for the report and patch.

comment:7 Changed 5 years ago by russellm

In [15499]:

[1.2.X] Fixed #13987 -- Ensure that the primary key is set correctly for all models that have concrete-abstract-concrete inheritance, not just the first model. Thanks to Aramgutang for the report and patch.

Backport of r15498 from trunk.

comment:8 Changed 5 years ago by ramiro

In [15525]:

[1.2.X] Tweaked some asserts not present in older unittest or deprecated from tests added in r15499. Refs #13987

comment:9 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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