Opened 14 years ago

Closed 14 years ago

Last modified 13 years ago

#13987 closed (fixed)

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

Reported by: Aram Dulyan 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: no UI/UX: no

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 Aram Dulyan 14 years ago.
1st possible fix: adding a primary_key=True to the OneToOneField initialisation.
13987_primary_key_fix_2.diff (661 bytes ) - added by Aram Dulyan 14 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 Aram Dulyan 14 years ago.
2nd (superior) fix variant, with regression tests.
13987_fix_2_with_tests_updated.diff (3.0 KB ) - added by Aram Dulyan 14 years ago.
Updated patch to bring test into line with the changes in the test suite.

Download all attachments as: .zip

Change History (13)

by Aram Dulyan, 14 years ago

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

by Aram Dulyan, 14 years ago

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

by Aram Dulyan, 14 years ago

Attachment: 13987_fix_2_with_tests.diff added

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

comment:1 by Aram Dulyan, 14 years ago

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 by mike@…, 14 years ago

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 by Russell Keith-Magee, 14 years ago

"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 by Daishiman, 14 years ago

Triage Stage: UnreviewedAccepted

Tested problem description with second fix. Works as advertised.

by Aram Dulyan, 14 years ago

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

comment:5 by Julien Phalip, 14 years ago

Keywords: sprintdec2010 added
Triage Stage: AcceptedReady for checkin

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

comment:6 by Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: newclosed

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 by Russell Keith-Magee, 14 years ago

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 by Ramiro Morales, 14 years ago

In [15525]:

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

comment:9 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

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