Opened 12 years ago

Closed 11 years ago

Last modified 11 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 11 years ago.
18153-2.patch (4.3 KB ) - added by Aymeric Augustin 11 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 by Andrew Godwin, 12 years ago

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 by David Hatch, 12 years ago

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 by David Hatch, 12 years ago

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 by David Hatch, 12 years ago

Owner: changed from nobody to David Hatch
Status: newassigned

comment:5 by Anssi Kääriäinen, 12 years ago

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.

in reply to:  5 comment:6 by David Hatch, 12 years ago

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 by David Hatch, 12 years ago

Patch needs improvement: set

comment:8 by Anssi Kääriäinen, 12 years ago

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 by David Hatch, 12 years ago

Patch needs improvement: unset

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

comment:10 by Aymeric Augustin, 11 years ago

#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 11 years ago by Aymeric Augustin (previous) (diff)

comment:11 by Aymeric Augustin, 11 years ago

(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 by Anssi Kääriäinen, 11 years ago

Looks like a good approach to me.

by Aymeric Augustin, 11 years ago

Attachment: 18153.patch added

by Aymeric Augustin, 11 years ago

Attachment: 18153-2.patch added

comment:13 by Aymeric Augustin, 11 years ago

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 by Aymeric Augustin, 11 years ago

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

comment:15 by Aymeric Augustin, 11 years ago

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 by Aymeric Augustin, 11 years ago

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 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

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 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 0a0fe8f71d54e8479e3050ef3bb9d545fd734a65:

Fix exception message from 3190abcd. Refs #18153.

Thanks Preston Holmes.

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