Deprecate implicit OneToOneField parent_link

[Bertrand Bordage]

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

[Tim Graham]

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.

[Simon Charette]

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.

[Bertrand Bordage]

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?

[Tim Graham]

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

[Bertrand Bordage]

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.

[Tim Graham]

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

[Simon Charette]

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.

[Bertrand Bordage]

@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?

[Tim Graham]

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

[Tim Graham]

​PR

[Tim Graham <timograham@…>]

In 8733819:

Fixed #26320 -- Deprecated implicit OneToOnField parent_link.

[Tim Graham <timograham@…>]

In 9d0e8c1e:

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