Opened 5 months ago
Closed 5 months ago
#35585 closed Bug (fixed)
`Query.has_results` calls `.exists()` with wrong argument
Reported by: | Flavio Curella | Owned by: | Flavio Curella |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Flavio Curella | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
The has_results
method of the Query
class (in django/db/models/sql/query.py
), passes the using
argument to the exists()
method:
def has_results(self, using): q = self.exists(using) compiler = q.get_compiler(using=using) return compiler.has_results()
but the signature of the exists
method does not accept an argument to select the db connection. It only accepts an argument to limit the rows it should fetch:
def exists(self, limit=True): # ... snip ...
Change History (13)
comment:1 by , 5 months ago
Description: | modified (diff) |
---|
comment:2 by , 5 months ago
Description: | modified (diff) |
---|
follow-up: 4 comment:3 by , 5 months ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Cleanup/optimization → Bug |
comment:4 by , 5 months ago
Replying to Simon Charette:
Regression caused by 3d734c09ff0138441dfe0a59010435871d17950f, I really wish we had type checking in this part of the code base to catch these kind of bugs that flew under the radar for almost two years.
Unfortunately, it hasn't change the behavior, because limit
is True
by default and we check if it's truthy in Query.exists()
.
comment:6 by , 5 months ago
comment:8 by , 5 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
Triage Stage: | Accepted → Ready for checkin |
follow-up: 10 comment:9 by , 5 months ago
Needs tests: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
PR looks good but it needs a regression test.
comment:10 by , 5 months ago
Replying to Natalia Bidart:
PR looks good but it needs a regression test.
IMO, it's not worth adding a regression test when the only doable option is to mock exists()
call in has_results()
and assert passed arguments.
comment:11 by , 5 months ago
I agree with Mariusz. Testing implementation details like this seems more likely to add a maintenance burden in future refactors than to add any value. As far as I can tell, this mistake didn't cause an actual problem.
comment:12 by , 5 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Regression caused by 3d734c09ff0138441dfe0a59010435871d17950f, I really wish we had type checking in this part of the code base to catch these kind of bugs that flew under the radar for almost two years.