[Django] #36865: Query casting introduced in Django 5.2 makes queries miss DB indexes

[Django]

#36865: Query casting introduced in Django 5.2 makes queries miss DB indexes
-------------------------------------+-------------------------------------
Reporter: Mike Lissner | Type: Bug
Status: new | Component:
| contrib.admin
Version: 6.0 | Severity: Normal
Keywords: search, regression | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
https://code.djangoproject.com/ticket/26001 from 10 years ago was fixed in
PR https://github.com/django/django/pull/17885/, which was released in
Django 5.2.

Unfortunately, this fix casts all admin search queries to varchar, which
makes the query miss the DB index instead of using it. In large tables,
this results in the query timing out, as documented as a comment on the
original issue:

https://code.djangoproject.com/ticket/26001#comment:24

And in my project's issue tracker:

https://github.com/freelawproject/courtlistener/issues/6790

We're fixing this in our system by patching the `get_search_results`
method:

https://github.com/freelawproject/courtlistener/pull/6792

I'm not at all familiar with this part of Django, but I spent some time
with claude and I think I have a fix here:

https://github.com/django/django/pull/20538

I did my best to be surgical and to add tests, but, again, this is foreign
stuff to me. I hope this helps.
--
Ticket URL: <https://code.djangoproject.com/ticket/36865>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36865: Query casting introduced in Django 5.2 makes queries miss DB indexes

------------------------------------+------------------------------------
Reporter: Mike Lissner | Owner: (none)

Type: Bug | Status: new
Component: contrib.admin | Version: 6.0

Severity: Normal | Resolution:
Keywords: search_fields cast | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

------------------------------------+------------------------------------
Changes (by Simon Charette):

* keywords: search, regression => search_fields cast
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted

Comment:

[https://github.com/django/django/pull/18765#pullrequestreview-2414452529
This problem was discussed in a follow up MR] to #26001.

As pointed out on
[https://github.com/django/django/pull/20538/changes#r2694820830 the
proposed patch] (which you should prefix with #36865 to be automatically
linked here Mike) using either `field.to_python` (or
`field.formfield().to_python` as we're dealing with untrusted user input)
can likely be used as a signal to determine if a `cast` is necessary.

I'm also wondering why we even search against cast'ed to text integer
field when provided a string not composed of digits (AKA when it fails
`to_python`) when using the `exact` lookup. It seems like the logic could
be adjusted the other way around by avoiding exact lookups for search
terms that search fields fail `to_python` and we could drop the casting
altogether.

e.g.

{{{#!python
class Author(models.Model):
name = models.CharField()
dob = models.DateField()

class AuthorAdmin(models.ModelAdmin):
search_fields = ["name", "dob"]
}}}

then a search of the form `?q=foo 2010-01-15` should result in

{{{#!python
filter(
Q(name="foo")
| Q(name="2010-01-15")
| Q(dob=date(2010, 1, 15)
)
}}}

Where we don't even try `dob_str="foo"` as
`DateField.formfield().to_python("foo")` identity that the value is not
valid for this field and thus cannot be an exact match.
--
Ticket URL: <https://code.djangoproject.com/ticket/36865#comment:1>

[Django]

#36865: Query casting introduced in Django 5.2 makes queries miss DB indexes
------------------------------------+------------------------------------
Reporter: Mike Lissner | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 6.0
Severity: Normal | Resolution:
Keywords: search_fields cast | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
------------------------------------+------------------------------------

Comment (by Mike Lissner):

Thanks. I've updated the title of the PR so it links here (magic?) and
I've pushed a commit with the change you suggest here. I agree — much
better. Again, I've had a lot of help from Claude on this, so hopefully
the code is good. It's a change I wouldn't have dared a few months ago,
but I'm hopeful will be good enough now.

I see this ticket says it needs tests and that it needs improvement. I'm
assuming it's up to maintainers to uncheck those, yes?
--
Ticket URL: <https://code.djangoproject.com/ticket/36865#comment:2>

[Django]

#36865: Query casting introduced in Django 5.2 makes queries miss DB indexes

-------------------------------------+-------------------------------------
Reporter: Mike Lissner | Owner: Mike
| Lissner
Type: Bug | Status: assigned

Component: contrib.admin | Version: 6.0
Severity: Normal | Resolution:
Keywords: search_fields cast | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Antoliny):

* owner: (none) => Mike Lissner
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36865#comment:3>

[Django]

#36865: Query casting introduced in Django 5.2 makes queries miss DB indexes
-------------------------------------+-------------------------------------
Reporter: Mike Lissner | Owner: Mike
| Lissner
Type: Bug | Status: assigned
Component: contrib.admin | Version: 6.0
Severity: Normal | Resolution:
Keywords: search_fields cast | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Antoliny):

Replying to [comment:2 Mike Lissner]:

> I see this ticket says it needs tests and that it needs improvement. I'm
assuming it's up to maintainers to uncheck those, yes?

Thank you Mike!
Once you've completed your work, you can switch "needs tests" and "patch
needs improvement" to no.
When "needs tests", "needs patch improvement", and "needs documentation"
are all set to no, it will be added to the review queue and can proceed
after being reviewed by maintainers or reviewers.
I think this [https://docs.djangoproject.com/en/6.0/internals/contributing
/triaging-tickets/#triage-workflow document] might be helpful :)
--
Ticket URL: <https://code.djangoproject.com/ticket/36865#comment:4>

[Django]

#36865: Query casting introduced in Django 5.2 makes queries miss DB indexes
-------------------------------------+-------------------------------------
Reporter: Mike Lissner | Owner: Mike
| Lissner
Type: Bug | Status: assigned
Component: contrib.admin | Version: 6.0
Severity: Normal | Resolution:
Keywords: search_fields cast | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mike Lissner):

* needs_better_patch: 1 => 0
* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36865#comment:5>

[Django]

#36865: Query casting introduced in Django 5.2 makes queries miss DB indexes
-------------------------------------+-------------------------------------
Reporter: Mike Lissner | Owner: Mike
| Lissner
Type: Bug | Status: assigned
Component: contrib.admin | Version: 6.0
Severity: Normal | Resolution:
Keywords: search_fields cast | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mike Lissner):

This should be ready for review again.
--
Ticket URL: <https://code.djangoproject.com/ticket/36865#comment:6>