[Django] #35585: `Query.has_results` calls `.exists()` with wrong argument
23 views
Skip to first unread message
Django
Jul 8, 2024, 10:56:28 AM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Type:
| Cleanup/optimization
Status: new | Component: Database
| layer (models, ORM)
Version: dev | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
The `has_results` method of the `Query` class (in
`django/db/models/sql/query.py`), passes the `using` argument to the
`exists()` method:
```python
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:
```python
def exists(self, limit=True):
# ... snip ...
```
--
Ticket URL: <https://code.djangoproject.com/ticket/35585>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Type:
| Cleanup/optimization
Status: new | Component: Database
| layer (models, ORM)
Version: dev | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
The `has_results` method of the `Query` class (in
`django/db/models/sql/query.py`), passes the `using` argument to the
`exists()` method:
```python
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:
```python
def exists(self, limit=True):
# ... snip ...
```
--
Ticket URL: <https://code.djangoproject.com/ticket/35585>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Jul 8, 2024, 10:59:19 AM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Flavio Curella:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> The `has_results` method of the `Query` class (in
> `django/db/models/sql/query.py`), passes the `using` argument to the
> `exists()` method:
>
> ```python
> 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:
>
> ```python
> def exists(self, limit=True):
> # ... snip ...
> ```
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:
{{{
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 ...
}}}
--
--
# ... snip ...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:1>
Django
Jul 8, 2024, 10:59:41 AM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Flavio Curella:
Old description:
> The `has_results` method of the `Query` class (in
> `django/db/models/sql/query.py`), passes the `using` argument to the
> `exists()` method:
>
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Flavio Curella:
Old description:
> 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 ...
> }}}
New description:
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 ...
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:2>
> 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 ...
> }}}
New description:
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 ...
}}}
--
--
Django
Jul 8, 2024, 11:02:40 AM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: Bug | Status: new
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
* type: Cleanup/optimization => Bug
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:3>
Django
Jul 8, 2024, 12:49:45 PM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Replying to [comment:3 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
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.
default and we check if it's truthy in `Query.exists()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:4>
Django
Jul 8, 2024, 12:55:34 PM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Flavio Curella):
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
should I change to check to `if limit is True:`?
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:5>
Django
Jul 8, 2024, 1:16:58 PM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Replying to [comment:5 Flavio Curella]:
> should I change to check to `if limit is True:`?
Sounds good +1.
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:6>
Django
Jul 8, 2024, 1:19:22 PM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Flavio Curella):
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Done.
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:7>
Django
Jul 8, 2024, 1:46:09 PM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
-------------------------------------+-------------------------------------
| Curella
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Flavio Curella
* stage: Accepted => Ready for checkin
* status: new => assigned
Comment:
[https://github.com/django/django/pull/18353 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:8>
Django
Jul 8, 2024, 2:51:02 PM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):
-------------------------------------+-------------------------------------
* needs_tests: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
PR looks good but it needs a regression test.
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:9>
Django
Jul 8, 2024, 2:57:26 PM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Replying to [comment:9 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:10>
Django
Jul 8, 2024, 6:03:47 PM7/8/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:11>
Django
Jul 23, 2024, 8:21:43 AM7/23/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:12>
Django
Jul 23, 2024, 10:36:36 AM7/23/24
to django-...@googlegroups.com
#35585: `Query.has_results` calls `.exists()` with wrong argument
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Type: Bug | Status: closed
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Flavio
| Curella
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by GitHub <noreply@…>):
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"f9bf616597d56deac66d9d6bb753b028dd9634cc" f9bf616]:
{{{#!CommitTicketReference repository=""
revision="f9bf616597d56deac66d9d6bb753b028dd9634cc"
Fixed #35585 -- Corrected Query.exists() call in Query.has_results().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35585#comment:13>
Reply all
Reply to author
Forward
0 new messages