Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#18153 closed Bug (fixed)

Erorneous OneToOneField for instances that have pk unset

Reported by: jbzdak@… Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (2)

18153.patch (2.8 KB) - added by Aymeric Augustin 4 years ago.
18153-2.patch (4.3 KB) - added by Aymeric Augustin 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by Andrew Godwin

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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.

comment:2 Changed 4 years ago by 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.

comment:3 Changed 4 years ago by David Hatch

Has patch: set

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.

comment:4 Changed 4 years ago by David Hatch

Owner: changed from nobody to David Hatch
Status: newassigned

comment:5 Changed 4 years ago by 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.

comment:6 in reply to:  5 Changed 4 years ago by 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.

comment:7 Changed 4 years ago by David Hatch

Patch needs improvement: set

comment:8 Changed 4 years ago by 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.

comment:9 Changed 4 years ago by David Hatch

Patch needs improvement: unset

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

comment:10 Changed 4 years ago by 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).

Last edited 4 years ago by Aymeric Augustin (previous) (diff)

comment:11 Changed 4 years ago by 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.

comment:12 Changed 4 years ago by Anssi Kääriäinen

Looks like a good approach to me.

Changed 4 years ago by Aymeric Augustin

Attachment: 18153.patch added

Changed 4 years ago by Aymeric Augustin

Attachment: 18153-2.patch added

comment:13 Changed 4 years ago by 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.

comment:14 Changed 4 years ago by Aymeric Augustin

Owner: changed from David Hatch to Aymeric Augustin
Status: assignednew

comment:15 Changed 4 years ago by Aymeric Augustin

Patch needs improvement: set
======================================================================
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.

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

comment:16 Changed 4 years ago by 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.

comment:17 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: newclosed

In 3190abcd75b1fcd660353da4001885ef82cbc596:

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

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

comment:18 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In 0a0fe8f71d54e8479e3050ef3bb9d545fd734a65:

Fix exception message from 3190abcd. Refs #18153.

Thanks Preston Holmes.

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