Code

Opened 3 years ago

Closed 18 months ago

#14615 closed Bug (fixed)

Related objects manager returns related objects with null FKs for unsaved instances

Reported by: tonnzor Owned by:
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: tonnzor, <tonn81@…>, victor.van.den.elzen@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Let's say we have 2 models (one refer to another):

class User(Model):
    pass

class MyObject(Model):
    user = ForeignKey(User, null=True, blank=True)

In this case ORM works wrong in this case:

User().myobject_set.all() # that would return all MyObjects that have user=None

So None/null value is supposed to be a valid foreign key between objects with is obviously not. Only if foreign key is not null - then it should be used.

I use a simple workaround that may be useful for fixing the issue:

User().myobject_set.exclude(user=None).all()

Attachments (2)

14615.diff (4.8 KB) - added by lukeplant 3 years ago.
patch - throw exception rather than do a query for unsaved instances
14615-r16341.diff (5.1 KB) - added by Victor van den Elzen <victor.van.den.elzen@…> 3 years ago.
updated patch

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

I don't think the User().myobject_set.all() syntax you are proposing is at all intuitive. MyObject.objects.filter(user=None) makes much more sense as a query returning "all Myobjects with user=None".

comment:2 in reply to: ↑ 1 Changed 3 years ago by anonymous

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Replying to russellm:

I don't think the User().myobject_set.all() syntax you are proposing is at all intuitive. MyObject.objects.filter(user=None) makes much more sense as a query returning "all Myobjects with user=None".

That's exact point I meant.

The meaning of user.myobject_set is to return all MyObjects that are related to this user. So if user.id is None then user.myobject_set should return NO objects at all. Right now it works wrong - it returns all MyObjects with user=None. That should be fixed.

For getting MyObjects with user=None developers must use MyObject.objects.filter(user=None)

comment:3 Changed 3 years ago by tonnzor

  • Cc tonnzor, <tonn81@…> added

comment:4 Changed 3 years ago by gabrielhurley

  • Summary changed from ORM works for None values too to Related objects manager returns related objects with null FKs for unsaved instances
  • Triage Stage changed from Unreviewed to Accepted

Interesting... this bug (I think it's a bug) manifests itself only when you use the reverse relationship on an unsaved instance of the model, specifically User().my_objects.

All the same, I can confirm the problem using the OP's models and a test case that looks like this:

from django.test import TestCase
from testapp.models import User, MyObject

class MyTestCase(TestCase):
    def test_reverse_related(self):
        MyObject.objects.create()
        u = User.objects.create()
        
        # This passes
        self.assertQuerysetEqual(u.myobject_set.all(), [])
        
        # This one fails
        self.assertQuerysetEqual(User().myobject_set.all(), [])

Changed 3 years ago by lukeplant

patch - throw exception rather than do a query for unsaved instances

comment:6 Changed 3 years ago by tonnzor

My idea was to fix this in a very straight way - change ForeignKey behavior (the piece of code that generates many_set method) - so it doesn't take into account null.

Let me explain a bit more. If foreign key is null - then there's no link between objects. So *_set should return EMPTY set disregarding DB state.

Using object state (saved/not saved) makes this thing much more complicated and have unpredicted not covered cases. If user changed object PK and didn't save that into DB - that's his decision, we should not keep the state.

Another edge case is null PK (user used custom field as PK) - that is simply covered by initial code, but not by object state.

comment:7 Changed 3 years ago by lukeplant

It is precisely things like a null PK that your solution doesn't cover. For foreign key values, we don't know whether 'None' means 'No value has been set' or 'database NULL'. If we'd thought about this at the beginning we might have used separate values to indicate those two, but it is too late now.

Even without that problem, we don't want to fix this by silently returning an empty set of objects. It is always nonsense to ask the DB for the related objects of an unsaved object. So every time someone does that, it is a logical error in their program. Making sure that such code produces an empty set of results is not actually helpful.

comment:8 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 3 years ago by ramiro

  • Component changed from ORM aggregation to Database layer (models, ORM)
  • Easy pickings unset

comment:10 Changed 3 years ago by Victor van den Elzen <victor.van.den.elzen@…>

  • Cc victor.van.den.elzen@… added
  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin
  • UI/UX unset

lukeplant's patch updated to current SVN (r16341). Fully tested against sqlite.

Changed 3 years ago by Victor van den Elzen <victor.van.den.elzen@…>

updated patch

comment:11 Changed 3 years ago by Merino

Applied the patch from Victor and it work

comment:12 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:13 Changed 2 years ago by lukeplant

  • Triage Stage changed from Ready for checkin to Design decision needed

I don't think this is ready for checkin, because we never came to a conclusion about Carl's comments here: https://groups.google.com/forum/#!msg/django-developers/HP23HEfpvCE/EWVK317NkVAJ

So we are still on DDN really, because the two alternatives seem to be:

1) Use instance._state.adding to detect 'unset' primary keys (controversial)

2) Admit that we can't cope with this situation, and call the bug a developer error in which they get a silly answer because they've asked a silly question.

comment:14 Changed 2 years ago by lukeplant

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

According to wikipedia and this discussion: https://groups.google.com/forum/#!msg/django-developers/sCjjx6NmMw4/4VJ4D0vsA_kJ

...having a NULL primary key is not allowed. SQLite allows it, but doesn't guarantee it will moving forward. Therefore, we should not program for that edge case. Instead, we can detect unsaved instances using the existing 'pk is None' idiom.

(If we make a decision that NULL PKs *are* allowed, there are lots of bit of code that probably need updating, and that should be done separately - although that seems unlikely).

The instance._state.adding flag was needed for the edge case of doing uniqueness validation for explicitly set CharField PKs for new objects. That is not needed here, or wanted - if the user creates a Model instance with an explicitly set PK which corresponds to data in the database, we expect that doing the related lookups should return the records related to that PK e.g. we expect User(id=1).myobject_set.all() to return MyObject.objects.filter(user_id=1), even though the User instance was not saved.

Apologies for the confusion my patch added.

comment:15 Changed 18 months ago by aaugustin

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

Given Luke's last comment, I believe this is a duplicate #18153 which I fixed in 3190abcd75b1fcd660353da4001885ef82cbc596.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.