Opened 3 months ago

Closed 2 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 Flavio Curella)

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 Flavio Curella, 3 months ago

Description: modified (diff)

comment:2 by Flavio Curella, 3 months ago

Description: modified (diff)

comment:3 by Simon Charette, 3 months ago

Triage Stage: UnreviewedAccepted
Type: Cleanup/optimizationBug

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.

in reply to:  3 comment:4 by Mariusz Felisiak, 3 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:5 by Flavio Curella, 3 months ago

should I change to check to if limit is True:?

in reply to:  5 comment:6 by Mariusz Felisiak, 3 months ago

Replying to Flavio Curella:

should I change to check to if limit is True:?

Sounds good +1.

comment:7 by Flavio Curella, 3 months ago

Done.

comment:8 by Mariusz Felisiak, 3 months ago

Owner: set to Flavio Curella
Status: newassigned
Triage Stage: AcceptedReady for checkin

comment:9 by Natalia Bidart, 3 months ago

Needs tests: set
Triage Stage: Ready for checkinAccepted

PR looks good but it needs a regression test.

in reply to:  9 comment:10 by Mariusz Felisiak, 3 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 Tim Graham, 3 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 Natalia Bidart, 2 months ago

Triage Stage: AcceptedReady for checkin

comment:13 by GitHub <noreply@…>, 2 months ago

Resolution: fixed
Status: assignedclosed

In f9bf616:

Fixed #35585 -- Corrected Query.exists() call in Query.has_results().

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