Opened 4 years ago

Closed 8 months ago

Last modified 4 months ago

#14334 closed Bug (fixed)

Queries don't ensure that comparison objects are the correct type

Reported by: rpbarlow Owned by: anubhav9042
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: 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

This is best demonstrated by an example. Suppose you have the following models:

class A(models.Model):
    pass

class B(models.Model):
    pass

class C(models.Model):
    b = models.ForeignKey(B)

Then suppose you did this:

        a = models.A.objects.create()
        b = models.B.objects.create()
        # This works, because the following assertion is True
        self.assertEquals(a.id, b.id)
        c = models.C.objects.create(b=b)

        # Let's try to query for a c where b==b. Works as expected
        self.assertEquals(models.C.objects.get(b=b).b, b)

        # Uh oh, why can I query for b=a? This first one shouldn't pass
        self.assertEquals(models.C.objects.get(b=a).b, b)
        # This one should fail, but it should fail at the .get() by
        # raising an exception, I think
        self.assertEquals(models.C.objects.get(b=a).b, a)

My proposal is that Django should complain when I pass in an instance of A for comparison with an FK to B, since a isn't the same as b. The query seems to just compare their primary keys, and not also ensure that the types are what is expected.

I've created a small django project that demonstrates this problem.

Attachments (1)

test_project.tar.bz2 (3.8 KB) - added by rpbarlow 4 years ago.
Django project, demonstrating the problem

Download all attachments as: .zip

Change History (18)

Changed 4 years ago by rpbarlow

Django project, demonstrating the problem

comment:1 Changed 4 years ago by rpbarlow

  • Component changed from Uncategorized to Core framework
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The problem is demonstrated in the unit tests to the app. Thanks for your consideration!

comment:2 Changed 4 years ago by rpbarlow

For clarity, I propose that an exception should be raised by this code:

models.C.objects.get(b=a)

comment:3 Changed 4 years ago by ericholscher

  • Has patch set
  • Needs documentation set
  • Owner changed from nobody to ericholscher
  • Status changed from new to assigned

This seems like something that should either raise an exception, or at least be noted by a warning in the documentation.

comment:4 Changed 4 years ago by ericholscher

  • Owner ericholscher deleted
  • Status changed from assigned to new
  • Triage Stage changed from Unreviewed to Accepted

Bah, forgot "Accept ticket" doesn't set its state to Accepted, but makes me the owner :x

comment:5 Changed 4 years ago by lukeplant

The only tricky thing with this is determining what kind of type check to do. If, for example, the a object is actually any kind of proxy to an instance of models.B it should not be rejected (by the normal Pythonic principles of duck typing). So a simple isinstance(the_model_class_we_expect) will not do.

comment:6 Changed 4 years ago by julien

  • Component changed from Core framework to Database layer (models, ORM)

comment:7 Changed 4 years ago by graham_king

  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:10 Changed 14 months ago by anubhav9042

  • Owner set to anubhav9042
  • Status changed from new to assigned
Last edited 9 months ago by anubhav9042 (previous) (diff)

comment:11 Changed 9 months ago by anubhav9042

  • Has patch unset
  • Version changed from 1.2 to master

comment:13 Changed 9 months ago by anubhav9042

  • Needs documentation unset

comment:14 Changed 9 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

ORM changes could use a final review.

comment:15 Changed 8 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 34ba86706f0db33d9a0ab44e4abb78703e7262a9:

Fixed #14334 -- Query relation lookups now check object types.

Thanks rpbarlow for the suggestion; and loic, akaariai, and jorgecarleitao
for reviews.

comment:16 Changed 7 months ago by apollo13

This caused at least one issue as described in #23266

comment:17 Changed 4 months ago by Anssi Kääriäinen <akaariai@…>

In ae7cb992bca5d211c9456487feb21b84387006eb:

Fixed #23721 -- check_related_objects without calling iter

Refs #14334

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