#31486 closed Bug (fixed)
Deprecate passing unsaved objects to related filters.
Reported by: | Mapiarz | Owned by: | raydeal |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Consider this filter:
Foo.objects.filter(related_obj=bar)
Where 'bar' is an unsaved object instance. In Django 1.11, this would always return an empty QuerySet (since no Foo object is related to unsaved 'bar'). In Django 2.0 through 2.2, this is equivalent to doing (which can return a non-empty QuerySet):
Foo.objects.filter(related_obj=None)
I found a somewhat related issue that touches on this subject: https://code.djangoproject.com/ticket/27985
My questions:
- What is the intended behaviour? In the aforementioned issue Simon Charette suggests that unsaved objects should be prevented from being used in related filters. I agree with that.
- Is this documented anywhere? I couldn't find anything. At the very least this should be documented somewhere.
Change History (14)
comment:1 by , 5 years ago
Summary: | Related filters with unsaved objects → Deprecate passing unsaved objects to related filters. |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
Version: | 2.2 → master |
comment:2 by , 5 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:3 by , 5 years ago
Cc: | added |
---|---|
Patch needs improvement: | set |
follow-up: 9 comment:4 by , 5 years ago
As I commented on PR:
I don't know how to distinguish between p2.choice_set.all()
and Choice.objects.filter(poll=p2)
, It seems get_normalized_value
receives the same (value and lhs) for these both cases.
I've also tried to implement it in RelatedLookupMixin.get_prep_lookup
based on Mariusz comment on PR, but still same problem. the lhs
and rhs
have the same value in p2.choice_set.all()
and Choice.objects.filter(poll=p2)
cases.
comment:5 by , 4 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:6 by , 3 years ago
Could someone please look at this issue? It looks like related merge request is left unfinished. We recently had pretty severe bug because of this. This is really dangerous behavior.
comment:7 by , 3 years ago
PR was closed because of (Mariusz comment on PR) ticket #7488 - admin panel might use filtering with not saved model. But it is 14 years old ticket and it is worth to check, in current version, if admin still use that.
comment:8 by , 3 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
After investigation of this ticket and #7488 conclusion is:
Issue in the #7488 ticket is obsolete. It is about get_querystet
code that haven't existed since 2008.
Time line is as follow:
19-07-2008 newform-admin was merged with main branch
23-07-2008 #7488 ticket was closed
14-11-2008 the BaseInlineFormSet.get_queryset
code (forms/models.py) mentioned in #7488 was deleted in #9076 (not closed yet ???) (https://code.djangoproject.com/ticket/9076#comment:22) and queryset moved to
BaseInlineFormSet.__init__
https://github.com/django/django/commit/bca14cd3c8685f0c8d6a24583e3de33f94f8910b#diff-360e00ebf46ef996c61729321d5a59992d78be2ad6913fb546394c2817b3837a
28-12-2012 this code was changed #19524
20-11-2013 this code was changed #21472
Now the code is
if self.instance.pk is not None: qs = queryset.filter(**{self.fk.name: self.instance}) else: qs = queryset.none()
It means that passing of unsaved object to related filter in admin Inlines is already solved. Ticket #9076 might be closed.
Apart from this, the test https://github.com/django/django/blob/4f8c7fd9d91b35e2c2922de4bb50c8c8066cbbc6/tests/model_inheritance_regress/tests.py#L185 could be renamed and used as it was in PR
comment:9 by , 3 years ago
Replying to Hasan Ramezani:
As I commented on PR:
I don't know how to distinguish between
p2.choice_set.all()
andChoice.objects.filter(poll=p2)
, It seemsget_normalized_value
receives the same (value and lhs) for these both cases.
I've also tried to implement it in
RelatedLookupMixin.get_prep_lookup
based on Mariusz comment on PR, but still same problem. thelhs
andrhs
have the same value inp2.choice_set.all()
andChoice.objects.filter(poll=p2)
cases.
After looking at code and trying implement solution, my conclusion is: it is connected with #19580, if FK raises ValueError like M2M does it will open door to fix this without a struggle how to distinguish between p2.choice_set.all()
and Choice.objects.filter(poll=p2)
because first case will raise ValueError before filter
is evaluated.
comment:10 by , 3 years ago
Patch needs improvement: | unset |
---|
comment:11 by , 3 years ago
Agreed, test_issue_7488
is obsolete, removed in 18245b948bf7032a0b50d92a743eff822f5bc6a6.
comment:12 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Regarding this comment I still think that deprecating passing unsaved objects to related filters is worth doing so I'll accept this ticket on this basis.
Mapiarz, would you be interested in submitting a patch doing so? It might require a bit of adjustments in the suite but I think that warning on
obj.pk is None
is the way to go as it's less controversial thanobj._state.adding
.