Opened 13 years ago

Closed 11 years ago

#14615 closed Bug (fixed)

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

Reported by: Artem Skoretskiy Owned by:
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: Artem Skoretskiy, <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 Luke Plant 13 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@…> 13 years ago.
updated patch

Download all attachments as: .zip

Change History (17)

comment:1 by Russell Keith-Magee, 13 years ago

Resolution: wontfix
Status: newclosed

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".

in reply to:  1 comment:2 by anonymous, 13 years ago

Resolution: wontfix
Status: closedreopened

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 by Artem Skoretskiy, 13 years ago

Cc: Artem Skoretskiy <tonn81@…> added

comment:4 by Gabriel Hurley, 13 years ago

Summary: ORM works for None values tooRelated objects manager returns related objects with null FKs for unsaved instances
Triage Stage: UnreviewedAccepted

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(), [])

by Luke Plant, 13 years ago

Attachment: 14615.diff added

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

comment:6 by Artem Skoretskiy, 13 years ago

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 by Luke Plant, 13 years ago

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 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:9 by Ramiro Morales, 13 years ago

Component: ORM aggregationDatabase layer (models, ORM)
Easy pickings: unset

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

Cc: victor.van.den.elzen@… added
Has patch: set
Triage Stage: AcceptedReady for checkin
UI/UX: unset

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

by Victor van den Elzen <victor.van.den.elzen@…>, 13 years ago

Attachment: 14615-r16341.diff added

updated patch

comment:11 by Merijn Bertels, 13 years ago

Applied the patch from Victor and it work

comment:12 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:13 by Luke Plant, 12 years ago

Triage Stage: Ready for checkinDesign 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 by Luke Plant, 12 years ago

Patch needs improvement: set
Triage Stage: Design decision neededAccepted

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

Resolution: fixed
Status: reopenedclosed

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

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