Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#18153 closed Bug (fixed)

Erorneous OneToOneField for instances that have pk unset

Reported by: jbzdak@… Owned by: aaugustin
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 aaugustin 2 years ago.
18153-2.patch (4.3 KB) - added by aaugustin 2 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 3 years ago by andrewgodwin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

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 3 years ago by dhatch

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 3 years ago by dhatch

  • 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 3 years ago by dhatch

  • Owner changed from nobody to dhatch
  • Status changed from new to assigned

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

comment:6 in reply to: ↑ 5 Changed 3 years ago by dhatch

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 3 years ago by dhatch

  • Patch needs improvement set

comment:8 Changed 3 years ago by akaariai

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 3 years ago by dhatch

  • 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 2 years ago by aaugustin

#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 2 years ago by aaugustin (previous) (diff)

comment:11 Changed 2 years ago by aaugustin

(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 2 years ago by akaariai

Looks like a good approach to me.

Changed 2 years ago by aaugustin

Changed 2 years ago by aaugustin

comment:13 Changed 2 years ago by aaugustin

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 2 years ago by aaugustin

  • Owner changed from dhatch to aaugustin
  • Status changed from assigned to new

comment:15 Changed 2 years ago by aaugustin

  • 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 2 years ago by aaugustin

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

  • Resolution set to fixed
  • Status changed from new to closed

In 3190abcd75b1fcd660353da4001885ef82cbc596:

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

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

comment:18 Changed 2 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