Opened 13 years ago

Closed 10 years ago

Last modified 9 years ago

#14334 closed Bug (fixed)

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

Reported by: Randy Barlow Owned by: ANUBHAV JOSHI
Component: Database layer (models, ORM) Version: dev
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 Randy Barlow 13 years ago.
Django project, demonstrating the problem

Download all attachments as: .zip

Change History (18)

by Randy Barlow, 13 years ago

Attachment: test_project.tar.bz2 added

Django project, demonstrating the problem

comment:1 by Randy Barlow, 13 years ago

Component: UncategorizedCore framework

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

comment:2 by Randy Barlow, 13 years ago

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

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

comment:3 by Eric Holscher, 13 years ago

Has patch: set
Needs documentation: set
Owner: changed from nobody to Eric Holscher
Status: newassigned

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

comment:4 by Eric Holscher, 13 years ago

Owner: Eric Holscher removed
Status: assignednew
Triage Stage: UnreviewedAccepted

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

comment:5 by Luke Plant, 13 years ago

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 by Julien Phalip, 13 years ago

Component: Core frameworkDatabase layer (models, ORM)

comment:7 by Graham King, 13 years ago

Severity: Normal
Type: Bug

comment:8 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:10 by ANUBHAV JOSHI, 10 years ago

Owner: set to ANUBHAV JOSHI
Status: newassigned
Last edited 10 years ago by ANUBHAV JOSHI (previous) (diff)

comment:11 by ANUBHAV JOSHI, 10 years ago

Has patch: unset
Version: 1.2master

comment:13 by ANUBHAV JOSHI, 10 years ago

Needs documentation: unset

comment:14 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

ORM changes could use a final review.

comment:15 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

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 by Florian Apolloner, 10 years ago

This caused at least one issue as described in #23266

comment:17 by Anssi Kääriäinen <akaariai@…>, 9 years ago

In ae7cb992bca5d211c9456487feb21b84387006eb:

Fixed #23721 -- check_related_objects without calling iter

Refs #14334

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