Django: Ticket #16955: Querying on the reverse of a FK with the wrong class silently returns bad datahttps://code.djangoproject.com/ticket/16955
<pre class="wiki">class Author(Model):
pass
class Book(Model):
author = ForeignKey(Author, related_name='books')
class Foo(Model):
pass
&gt;&gt;&gt; a = Author.objects.create()
&gt;&gt;&gt; b = Book.objects.create(author=a)
&gt;&gt;&gt; foo = Foo.objects.create(pk=999)
&gt;&gt;&gt; Author.objects.filter(books=foo)
[]
</pre><p>
I would expect this to throw an exception because a Foo is not a Book. Instead, the Foo is coerced to its ID, which is then used as the PK for the reverse query.
</p>
<p>
This also affects ManyToManyField with through (effectively a reverse FK).
</p>
en-usDjangohttps://www.djangoproject.com/s/img/site/hdr_logo.gifhttps://code.djangoproject.com/ticket/16955
Trac 1.0.7bpeschierThu, 29 Sep 2011 06:06:55 GMTstage changedhttps://code.djangoproject.com/ticket/16955#comment:1
https://code.djangoproject.com/ticket/16955#comment:1
<ul>
<li><strong>stage</strong>
changed from <em>Unreviewed</em> to <em>Accepted</em>
</li>
</ul>
<p>
Indeed, when using a Foo with an id also available in Author:
</p>
<pre class="wiki">&gt;&gt;&gt; foo2 = Foo.objects.create(pk=1)
&gt;&gt;&gt; Author.objects.filter(books=foo2)
[&lt;Author: Author object&gt;]
</pre>
TicketdgouldinThu, 29 Sep 2011 16:01:19 GMTowner changed; cc sethttps://code.djangoproject.com/ticket/16955#comment:2
https://code.djangoproject.com/ticket/16955#comment:2
<ul>
<li><strong>owner</strong>
changed from <em>nobody</em> to <em>dgouldin</em>
</li>
<li><strong>cc</strong>
<em>dgouldin</em> added
</li>
</ul>
TicketdgouldinThu, 29 Sep 2011 16:18:16 GMTattachment sethttps://code.djangoproject.com/ticket/16955
https://code.djangoproject.com/ticket/16955
<ul>
<li><strong>attachment</strong>
set to <em>16955.diff</em>
</li>
</ul>
TicketdgouldinThu, 29 Sep 2011 16:18:37 GMThas_patch changedhttps://code.djangoproject.com/ticket/16955#comment:3
https://code.djangoproject.com/ticket/16955#comment:3
<ul>
<li><strong>has_patch</strong>
set
</li>
</ul>
TicketdgouldinThu, 29 Sep 2011 16:21:01 GMThttps://code.djangoproject.com/ticket/16955#comment:4
https://code.djangoproject.com/ticket/16955#comment:4
<p>
The test which was removed is no longer a valid use case. Passing the wrong model type to a queryset filter kwarg should raise a TypeError, even in the instance where the PKs happen to always be the same. The proper way to perform such a test would be to use concrete model inheritance, in which case isinstance would return True and the check I've added would pass.
</p>
Ticketnate_bWed, 18 Jan 2012 03:22:43 GMTneeds_better_patch changedhttps://code.djangoproject.com/ticket/16955#comment:5
https://code.djangoproject.com/ticket/16955#comment:5
<ul>
<li><strong>needs_better_patch</strong>
set
</li>
</ul>
<p>
Your patch still applies cleanly, and there are no unexpected test failures. After careful consideration toying with alternatives, I agree with your solution. Two things though:
</p>
<p>
For the sake of PEP8ness, could you put your imports at the top? Or did you have a good reason to do your imports immediately preceeding your check?
</p>
<p>
Also, in the <tt>OneToOneTests.test_foreign_key</tt> edit, why delete that line, instead of change it from <tt>p</tt> to <tt>r</tt>?
</p>
<p>
In fact, that whole section looks like it always ought to have been closer to this:
</p>
<pre class="wiki">assert_filter_waiters(restaurant__exact=self.r.pk)
assert_filter_waiters(restaurant__exact=self.r)
assert_filter_waiters(restaurant__pk=self.r.pk)
assert_filter_waiters(restaurant=self.r.pk)
assert_filter_waiters(restaurant=self.r)
assert_filter_waiters(id__exact=self.r.pk)
assert_filter_waiters(pk=self.r.pk)
# Delete the restaurant; the waiter should also be removed
r = Restaurant.objects.get(pk=self.r.pk)
</pre><p>
It was only passing by the fortuitous accident that <tt>self.p1.pk == self.r.pk</tt>. Fix that up, and this patch looks good to me.
</p>
Ticketnate_bSun, 22 Jan 2012 00:57:53 GMTattachment sethttps://code.djangoproject.com/ticket/16955
https://code.djangoproject.com/ticket/16955
<ul>
<li><strong>attachment</strong>
set to <em>16955-2.patch</em>
</li>
</ul>
<p>
Fixed PEP8ness and tests, per my previous comment
</p>
Ticketnate_bSun, 22 Jan 2012 00:58:38 GMTneeds_better_patch, stage changedhttps://code.djangoproject.com/ticket/16955#comment:6
https://code.djangoproject.com/ticket/16955#comment:6
<ul>
<li><strong>needs_better_patch</strong>
unset
</li>
<li><strong>stage</strong>
changed from <em>Accepted</em> to <em>Ready for checkin</em>
</li>
</ul>
Ticketnate_bSun, 22 Jan 2012 02:42:19 GMTattachment sethttps://code.djangoproject.com/ticket/16955
https://code.djangoproject.com/ticket/16955
<ul>
<li><strong>attachment</strong>
set to <em>16955-2.2.patch</em>
</li>
</ul>
<p>
Fixed tests only
</p>
Ticketnate_bSun, 22 Jan 2012 02:44:41 GMThttps://code.djangoproject.com/ticket/16955#comment:7
https://code.djangoproject.com/ticket/16955#comment:7
<p>
Ignore my previous patch - those imports were in-function to avoid a circular dependency (which is what I get for not looking before leaping). This one just corrects the issues in <tt>test_foreign_key</tt>.
</p>
TicketakaariaiSat, 21 Apr 2012 20:24:35 GMTneeds_better_patch, stage changedhttps://code.djangoproject.com/ticket/16955#comment:8
https://code.djangoproject.com/ticket/16955#comment:8
<ul>
<li><strong>needs_better_patch</strong>
set
</li>
<li><strong>stage</strong>
changed from <em>Ready for checkin</em> to <em>Accepted</em>
</li>
</ul>
<p>
While looking how related field does its get_prep_lookup() I come up with this comment (models/fields/related.py:176-180):
</p>
<pre class="wiki">def pk_trace:
# Value may be a primary key, or an object held in a relation.
# If it is an object, then we need to get the primary key value for
# that object. In certain conditions (especially one-to-one relations),
# the primary key may itself be an object - so we need to keep drilling
# down until we hit a value that can be used for a comparison.
</pre><p>
So, it seems to me it is intentional that you can use o2o fields like in the one_to_one tests. The changes to o2o handling need further investigation.
</p>
<p>
My main concern is that It should be the field's responsibility to do the validation of the values. It does not belong into <tt>QuerySet</tt>.add_filter. It would be better to do "lookup, values = field.check_lookup_values(lookup, values)" at the point the check is in the patch. By default the method just returns lookup, values, but it could raise <tt>TypeError</tt> or do something to the values or lookup if needed. If this was done, the check could use the code of _pk_trace directly, and thus the check and actual implementation would be guaranteed to match.
</p>
TicketakaariaiFri, 11 Jan 2013 14:25:13 GMThttps://code.djangoproject.com/ticket/16955#comment:9
https://code.djangoproject.com/ticket/16955#comment:9
<p>
Maybe a model instance method <tt>_get_field_value(field)</tt> which checks that the field exists in the instance and then gets its value is the way to go. This way because we need to allow also using parent classes:
</p>
<pre class="wiki">p = Place.objects.get(somecond)
Chef.objects.filter(restaurant=p)
</pre><p>
is a valid query if Place and Restaurant have the same primary key field.
</p>
TicketclaudepFri, 25 Jan 2013 18:45:13 GMThttps://code.djangoproject.com/ticket/16955#comment:10
https://code.djangoproject.com/ticket/16955#comment:10
<p>
<a class="closed ticket" href="https://code.djangoproject.com/ticket/19669" title="Bug: Django doesn't check type on foreign key query (closed: duplicate)">#19669</a> was a duplicate.
</p>
TicketakaariaiFri, 25 Jan 2013 19:54:07 GMThttps://code.djangoproject.com/ticket/16955#comment:11
https://code.djangoproject.com/ticket/16955#comment:11
<p>
This will be horrible to fix. At least the following work currently, also with multistep chains:
</p>
<pre class="wiki">parent__in=list_of_childs
child__in=list_of_parents
class A:
pass
class B:
a = models.OneToOneField(A, primary_key=True)
a__in=list_of_B
b__in=list_of_A
</pre><p>
This means that we must check issubclass in two directions + primary key chain from value to the related model, and from related model to value.
</p>
<p>
The sanest fixes would be to precalculate a set of allowed models in Model._meta. The calculation of the set of allowed models will still be ugly, but at least checking would be somewhat fast... Patches welcome.
</p>
<p>
Another way is to disallow the OneToOneField related lookups. The issubclass checks are easy to do.
</p>
TicketlukeplantSat, 26 Jan 2013 13:02:40 GMThttps://code.djangoproject.com/ticket/16955#comment:12
https://code.djangoproject.com/ticket/16955#comment:12
<p>
There is an argument for not fixing this: any time you add issubclass or isinstance checks you are breaking duck-typing.
</p>
<p>
I have seen Django code where there were 'business' objects that wrapped model objects, and sometimes, IIRC, were used interchangeably. Fixing this bug might potentially break such code.
</p>
<p>
I'm also not entirely convinced that the removed/changed tests ought to be invalid use cases. This is certainly a backwards incompatible change.
</p>
TicketakaariaiSat, 26 Jan 2013 13:43:06 GMThttps://code.djangoproject.com/ticket/16955#comment:13
https://code.djangoproject.com/ticket/16955#comment:13
<p>
I am sure it is possible to do the validity checks in a way that doesn't break duck typing, and preserves the OneToOneField behaviour. The _pk_trace code could do validity checks only if the value is subclass of model. But doing the OneToOneField preservation will likely be ugly.
</p>
<p>
If a nice patch surfaces I do think it is a good idea to add the check. It is not a good idea to do a backwards compatibility break just for the check.
</p>
TicketaaugustinTue, 26 Mar 2013 19:36:57 GMThttps://code.djangoproject.com/ticket/16955#comment:14
https://code.djangoproject.com/ticket/16955#comment:14
<p>
<a class="closed ticket" href="https://code.djangoproject.com/ticket/20141" title="Uncategorized: QuerySet filter should check type (closed: duplicate)">#20141</a> was a duplicate.
</p>
TicketakaariaiWed, 27 Mar 2013 04:33:26 GMThttps://code.djangoproject.com/ticket/16955#comment:15
https://code.djangoproject.com/ticket/16955#comment:15
<p>
The patch to <a class="closed ticket" href="https://code.djangoproject.com/ticket/19385" title="New feature: Add support for multiple-column join (closed: fixed)">#19385</a> did extensive changes to how foreign key lookup value checks are done. This will very likely need a revisit. It is now impossible to use wrong model class in lookups, but the current way is very likely too restrictive. Lets find out problematic cases and fix them...
</p>
TicketcollinandersonSat, 14 Mar 2015 03:35:36 GMThttps://code.djangoproject.com/ticket/16955#comment:16
https://code.djangoproject.com/ticket/16955#comment:16
<p>
I believe this is fixed now.
</p>
Ticket