#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)
Change History (18)
by , 15 years ago
| Attachment: | test_project.tar.bz2 added | 
|---|
comment:1 by , 15 years ago
| Component: | Uncategorized → Core framework | 
|---|
The problem is demonstrated in the unit tests to the app. Thanks for your consideration!
comment:2 by , 15 years ago
For clarity, I propose that an exception should be raised by this code:
 models.C.objects.get(b=a) 
comment:3 by , 15 years ago
| Has patch: | set | 
|---|---|
| Needs documentation: | set | 
| Owner: | changed from to | 
| Status: | new → assigned | 
This seems like something that should either raise an exception, or at least be noted by a warning in the documentation. 
comment:4 by , 15 years ago
| Owner: | removed | 
|---|---|
| Status: | assigned → new | 
| Triage Stage: | Unreviewed → Accepted | 
Bah, forgot "Accept ticket" doesn't set its state to Accepted, but makes me the owner :x
comment:5 by , 15 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 , 15 years ago
| Component: | Core framework → Database layer (models, ORM) | 
|---|
comment:7 by , 15 years ago
| Severity: | → Normal | 
|---|---|
| Type: | → Bug | 
comment:10 by , 12 years ago
| Owner: | set to | 
|---|---|
| Status: | new → assigned | 
comment:11 by , 11 years ago
| Has patch: | unset | 
|---|---|
| Version: | 1.2 → master | 
comment:13 by , 11 years ago
| Needs documentation: | unset | 
|---|
comment:14 by , 11 years ago
| Triage Stage: | Accepted → Ready for checkin | 
|---|
ORM changes could use a final review.
comment:15 by , 11 years ago
| Resolution: | → fixed | 
|---|---|
| Status: | assigned → closed | 
Django project, demonstrating the problem