Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#26320 closed Cleanup/optimization (fixed)

Deprecate implicit OneToOneField parent_link

Reported by: Bertrand Bordage Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
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

Suppose we have these models:

class Parent(Model):
    pass

class Child(Parent):
    parent_ptr2 = OneToOneField(Parent, null=True, related_name='+')

Child should have two OneToOneFields, parent_ptr and parent_ptr2.
Unfortunately, Django has a critical issue where it merges these two fields into this:

parent_ptr2 = OneToOneField(Parent, primary_key=True, null=True, related_name='+')

And of course, a nullable primary key is impossible, so we can’t even do a migration: fields.E007 is raised.
But defining without multi-table inheritance exactly the same model – in terms of database definition – works just fine:

class WorkingModel(Model):
    parent_ptr = OneToOneField(Parent, primary_key=True, related_name='+')
    parent_ptr2 = OneToOneField(Parent, null=True, related_name='+')

So multi-table inheritance is probably what causes this confusion.
This bug is present on all supported Django versions, from 1.8 to 1.9.3.

I guess this can be seen as a release blocker, there is no regression but this is quite critical (and blocking, in my case).

Change History (13)

comment:1 by Tim Graham, 8 years ago

The promotion of the OneToOneField to a primary_key happens in options.py. I didn't see a mention of the behavior in the multi-table inheritance docs but it seems to date to merge of the queryset refactor branch in 9c52d56f6f8a9cdafb231adf9f4110473099c9b5.

I don't know if we can change the behavior but we could at least document it.

Is there some problem defining your model like this:

class Child(Parent):
    parent_ptr = models.OneToOneField(Parent, primary_key=True, related_name='+')
    parent_ptr2 = models.OneToOneField(Parent, null=True, related_name='+')

I didn't get any check errors in that case but I didn't test any queries.

comment:2 by Simon Charette, 8 years ago

We explicitly document that OneToOneField will only be used as the link back to the parent class iff parent_link=True.

I think the bug lies in ModelBase.__new__ during parent_link collection. The isinstance(field, OneToOneField) check should also make sure that field.remote_field.parent_link is truthy.

comment:3 by Bertrand Bordage, 8 years ago

Yes, this workaround works :)

@charettes: I was about to write this, my investigations lead to the same! That’s the cause.

Thanks for the help, should I do a pull request?

comment:4 by Tim Graham, 8 years ago

The current behavior is tested. Checking field.remote_field.parent_link as suggested breaks that test and a few others.

comment:5 by Bertrand Bordage, 8 years ago

It will raise FieldError: Local field … clashes with field of similar name from base class …, like with any other field.
Isn’t that more consistent, since parent_link == False? I know, it breaks backwards compatibility for this edge case, though.

comment:6 by Tim Graham, 8 years ago

I'm okay with requiring parent_link=True but could we let it go through the deprecation cycle?

in reply to:  6 comment:7 by Simon Charette, 8 years ago

Replying to timgraham:

I'm okay with requiring parent_link=True but could we let it go through the deprecation cycle?

In the light of the existing tests I suppose we should just to be extra safe even if the test model's check() method returns multiple errors and the model is simply unusable with the ORM as field.primary_key is True and field.remote_field.parent_link is False.

comment:8 by Bertrand Bordage, 8 years ago

@timgraham: Sure! Since this is a rare use case, do you think it could be a RemovedInDjango110Warning introduced in 1.9.4? Or maybe it’s too soon?

comment:9 by Tim Graham, 8 years ago

Summary: Multi-table inheritance pointer merged with another OneToOneFieldDeprecate implicit OneToOneField parent_link
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I don't see this as a critical issue that needs an accelerated deprecation.

comment:10 by Tim Graham, 8 years ago

Has patch: set

comment:11 by Simon Charette, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 8733819:

Fixed #26320 -- Deprecated implicit OneToOnField parent_link.

comment:13 by Tim Graham <timograham@…>, 8 years ago

In 9d0e8c1e:

Refs #26320 -- Removed implicit OneToOnField parent_link per deprecation timeline.

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