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

[Artem Skoretskiy]

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()

[Russell Keith-Magee]

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

[anonymous]

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)

[Gabriel Hurley]

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

[Luke Plant]

Discussed on mailing list - ​http://groups.google.com/group/django-developers/browse_thread/thread/1cfdb71c47e9bc21

[Luke Plant]

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

[Artem Skoretskiy]

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.

[Luke Plant]

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.

[Victor van den Elzen <victor.van.den.elzen@…>]

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

[Victor van den Elzen <victor.van.den.elzen@…>]

updated patch

[Merijn Bertels]

Applied the patch from Victor and it work

[Jacob]

Milestone 1.3 deleted

[Luke Plant]

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.

[Luke Plant]

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.

[Aymeric Augustin]

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