Erorneous OneToOneField for instances that have pk unset

[jbzdak@…]

Basically if one uses reverse relation for OneToOneField from instance that has pk unset it results MultipleObjectsReturned error, because django serches database for rows that have NULL inside OneToOne column. For example:

class Move(models.Model):
    (...)
    return_move = models.OneToOneField("self", related_name="forward_move", blank=True, null=True)

    objects = models.GeoManager()

Then when I use forward_move attibute from move instance that has no pk assigned i get error:

m = Move()
m.forward_move
MultipleObjectsReturned at /move/add/ get() returned more than one Move -- it returned 87! Lookup parameters were {'return_move__pk': None}

There are 87 rows in the database that have NULL in return_move column.

Desired behaviour is for forward_move to return None --- since instance that is not saved in the databsae cant be referenced by models inside the database.

I use django 1.4, postgresql database and postgis.

[Andrew Godwin]

Valid bug, but I think that the SingleRelatedObjectDescriptor should return an error in this case, rather than None, as you can't assign to that descriptor in that state.

[David Hatch]

Perhaps for consistency with ForeignKeyField behavior it makes sense to raise a ObjectDoesNotExist unless the field is nullable, in which case return None.

[David Hatch]

The behavior I just described would break existing regression tests. I've written a patch for this such that SingleRelatedObjectDescriptor raises a DoesNotExist regardless of the null settings if the instance does not have a pk. Note: The behavior of raising DoesNotExist even when the field is nullable seems inconsistent with other components of the API.

ReverseSingleRelatedObjectDescriptor returns None when the field is null (see line 375 of django.db.models.fields.related). There is another open issue about this problem at #10227.

My patch is at ​https://github.com/dhatch/django/tree/ticket_18153. Will submit a pull request once it has been reviewed.

[Anssi Kääriäinen]

I vote for consistency with foreign key fields. It seems the right behavior to me. Although I have no idea how bad the breakage will be. There is always the possibility to claim this to be a bugfix and thus backwards compatibility does not apply... :)

As for the patch, it seems good to me. The question is if that is really the behavior we want to commit into.

[David Hatch]

Replying to akaariai:

I vote for consistency with foreign key fields. It seems the right behavior to me. Although I have no idea how bad the breakage will be. There is always the possibility to claim this to be a bugfix and thus backwards compatibility does not apply... :)

As for the patch, it seems good to me. The question is if that is really the behavior we want to commit into.

Agreed. Will modify the patch to be consistent with foreign keys, seems right to me as well.

The behavior before was as follows:

1) If no objects of the reverse one to one type existed in the database DoesNotExist was raised (regardless of null).

2) If one object of the reverse one to one type existed in the database, it was returned, regardless of the fact that the field was not set. (Clearly this scenario is a bug)

3) If multiple objects of the reverse type exist, MultipleObjectsReturned is raised.

After the patch the behavior for all these three cases will either be to raise DoesNotExist or return null if the field is nullable when the pk of the model is not set.

[Anssi Kääriäinen]

It is worth to keep the original patch also around. It might be we need to go that route if it turns out the backwards compatibility is likely to cause problems to users.

[David Hatch]

Just finished the new patch matching ReverseSingleRelatedObjectDescriptor. Located at ​https://github.com/dhatch/django/tree/ticket_18153_backwardsincompat.

[Aymeric Augustin]

#19089 was a duplicate with a patch for a subset of this problem (ie. only the lookup on a nullable one-to-one).

[Aymeric Augustin]

(1) I agree with Andrew that SingleRelatedObjectDescriptor should return an error in this case, and it's what I've implemented in the patch I attached to #19089.

(2) The asymmetry between forwards relations and backwards relations exists on purpose. The underlying relation in the database is asymmetric. So, at first sight, I disagree with the and not self.related.field.null part.

I'm willing to reconsider (2), which is tracked in #10227. We can continue that part of the discussion over there.

But I think we can fix (1), which is an obvious bug, without waiting for the decision on (2).

Since I independently wrote the same patch as dhatch, it's probably a reasonable solution. I plan combine both patches — mine doesn't break on legit values of pk such as '' or 0, dhaty's has more tests — and commit the result soon.

[Anssi Kääriäinen]

Looks like a good approach to me.

[Aymeric Augustin]

The first version of the patch deals with getting the reverse related object.

The second version of the patch also deals with setting it. A similar bug existed.

[Aymeric Augustin]

======================================================================
ERROR: test_o2o_cross_database_protection (regressiontests.multiple_database.tests.QueryTestCase)
Operations that involve sharing FK objects across databases raise an error
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/aaugustin/Documents/dev/django/tests/regressiontests/multiple_database/tests.py", line 664, in test_o2o_cross_database_protection
    charlie.userprofile = bob_profile
  File "/Users/aaugustin/Documents/dev/django/django/db/models/fields/related.py", line 311, in __set__
    (value, self.related.opts.object_name))
ValueError: Cannot assign "<UserProfile: UserProfile object>": "UserProfile" instance isn't saved in the database.

----------------------------------------------------------------------

[Aymeric Augustin]

I've created #19095 to discuss the test failure above.

That test contains the programming error that the second version of the patch attempts to prevent: setting a reverse relation on an unsaved objects.

[Aymeric Augustin <aymeric.augustin@…>]

In 3190abcd75b1fcd660353da4001885ef82cbc596:

Fixed #18153 -- Reverse OneToOne lookups on unsaved instances.

Thanks David Hatch and Anssi Kääriäinen for their inputs.

[Aymeric Augustin <aymeric.augustin@…>]

In 0a0fe8f71d54e8479e3050ef3bb9d545fd734a65:

Fix exception message from 3190abcd. Refs #18153.

Thanks Preston Holmes.