Opened 5 years ago

Closed 14 months ago

#16955 closed Bug (duplicate)

Querying on the reverse of a FK with the wrong class silently returns bad data

Reported by: Jeremy Dunck Owned by: David Gouldin
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: David Gouldin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

class Author(Model):
    pass

class Book(Model):
    author = ForeignKey(Author, related_name='books')

class Foo(Model):
    pass

>>> a = Author.objects.create()
>>> b  = Book.objects.create(author=a)
>>> foo = Foo.objects.create(pk=999)
>>> Author.objects.filter(books=foo)
[]

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.

This also affects ManyToManyField with through (effectively a reverse FK).

Attachments (3)

16955.diff (4.8 KB) - added by David Gouldin 5 years ago.
16955-2.patch (6.5 KB) - added by Nate Bragg 5 years ago.
Fixed PEP8ness and tests, per my previous comment
16955-2.2.patch (6.1 KB) - added by Nate Bragg 5 years ago.
Fixed tests only

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by Bas Peschier

Triage Stage: UnreviewedAccepted

Indeed, when using a Foo with an id also available in Author:

>>> foo2 = Foo.objects.create(pk=1)
>>> Author.objects.filter(books=foo2)
[<Author: Author object>]

comment:2 Changed 5 years ago by David Gouldin

Cc: David Gouldin added
Owner: changed from nobody to David Gouldin

Changed 5 years ago by David Gouldin

Attachment: 16955.diff added

comment:3 Changed 5 years ago by David Gouldin

Has patch: set

comment:4 Changed 5 years ago by David Gouldin

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.

comment:5 Changed 5 years ago by Nate Bragg

Patch needs improvement: set

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:

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?

Also, in the OneToOneTests.test_foreign_key edit, why delete that line, instead of change it from p to r?

In fact, that whole section looks like it always ought to have been closer to this:

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)

It was only passing by the fortuitous accident that self.p1.pk == self.r.pk. Fix that up, and this patch looks good to me.

Changed 5 years ago by Nate Bragg

Attachment: 16955-2.patch added

Fixed PEP8ness and tests, per my previous comment

comment:6 Changed 5 years ago by Nate Bragg

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Changed 5 years ago by Nate Bragg

Attachment: 16955-2.2.patch added

Fixed tests only

comment:7 Changed 5 years ago by Nate Bragg

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

comment:8 Changed 4 years ago by Anssi Kääriäinen

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

While looking how related field does its get_prep_lookup() I come up with this comment (models/fields/related.py:176-180):

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.

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.

My main concern is that It should be the field's responsibility to do the validation of the values. It does not belong into QuerySet.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 TypeError 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.

comment:9 Changed 4 years ago by Anssi Kääriäinen

Maybe a model instance method _get_field_value(field) 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 = Place.objects.get(somecond)
Chef.objects.filter(restaurant=p)

is a valid query if Place and Restaurant have the same primary key field.

comment:10 Changed 4 years ago by Claude Paroz

#19669 was a duplicate.

comment:11 Changed 4 years ago by Anssi Kääriäinen

This will be horrible to fix. At least the following work currently, also with multistep chains:

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

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.

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.

Another way is to disallow the OneToOneField related lookups. The issubclass checks are easy to do.

comment:12 Changed 4 years ago by Luke Plant

There is an argument for not fixing this: any time you add issubclass or isinstance checks you are breaking duck-typing.

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.

I'm also not entirely convinced that the removed/changed tests ought to be invalid use cases. This is certainly a backwards incompatible change.

comment:13 Changed 4 years ago by Anssi Kääriäinen

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.

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.

comment:14 Changed 4 years ago by Aymeric Augustin

#20141 was a duplicate.

comment:15 Changed 4 years ago by Anssi Kääriäinen

The patch to #19385 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...

comment:16 Changed 19 months ago by Collin Anderson

I believe this is fixed now.

comment:17 Changed 14 months ago by Tim Graham

Resolution: duplicate
Status: newclosed

Yes, duplicate of #14334.

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