[Django] #36934: BuiltinLookup breaks with params-as-a-tuple in django 6.0

[Django]

#36934: BuiltinLookup breaks with params-as-a-tuple in django 6.0
-------------------------------------+-------------------------------------
Reporter: Stefan Bühler | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 6.0 | 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
-------------------------------------+-------------------------------------
Hi,

`BuiltinLookup` violates the new policy and returns a list of params from
`process_lhs` and expects a list in `as_sql` (uses `.extend`).

Lookup classes that inherit from (or mixed into) `BuiltinLookup` and
overwrite `process_lhs` in a way that doesn't return a list of params
(e.g. calling `Col.as_sql` through `compiler.compile(self.lhs)` are
broken.

Example: django-netfields: https://github.com/jimfunk/django-postgresql-
netfields/blob/f7529f2c97995dbc5b27ea7abcf0f2966269fd96/netfields/lookups.py#L27-L59

The new policy: https://docs.djangoproject.com/en/6.0/releases/6.0
/#custom-orm-expressions-should-return-params-as-a-tuple

https://github.com/django/django/pull/20005 looks like it should fix this,
and should be packported.
--
Ticket URL: <https://code.djangoproject.com/ticket/36934>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36934: BuiltinLookup breaks with params-as-a-tuple in django 6.0
-------------------------------------+-------------------------------------

Reporter: Stefan Bühler | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Clifford Gama):

* severity: Normal => Release blocker
* stage: Unreviewed => Accepted

Comment:

Thanks. Leaving as has-patch since as you point out it is already fixed in
the main branch but wasn't backported.
--
Ticket URL: <https://code.djangoproject.com/ticket/36934#comment:1>

[Django]

#36934: BuiltinLookup breaks with params-as-a-tuple in django 6.0
-------------------------------------+-------------------------------------
Reporter: Stefan Bühler | Owner: (none)

Type: Bug | Status: closed

Component: Database layer | Version: 6.0
(models, ORM) |

Severity: Release blocker | Resolution: needsinfo

Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* resolution: => needsinfo
* status: new => closed

Comment:

Hi, thanks for the report, and thanks Clifford for triage. Clifford, I
don't know if you saw the closely related #36922, which we closed since it
was the responsibility of the custom lookup to return params in a tuple.

We chose not to backport since this should only be an issue if you've
subclassed the lookups and added further customizations. Your link doesn't
go to a custom lookup, so I can't tell how you're affected. Could you
please include a stacktrace and the custom lookup?

We have to weigh the potential upside from backporting against the
downside of introducing unexpected problems in projects that already
tested against the 6.0 betas. Eager to see the fuller stacktrace & lookup,
and we can look again at that point. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/36934#comment:2>

[Django]

#36934: BuiltinLookup breaks with params-as-a-tuple in django 6.0
-------------------------------------+-------------------------------------
Reporter: Stefan Bühler | Owner: (none)

Type: Bug | Status: new

Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Stefan Bühler):

* resolution: needsinfo =>
* status: closed => new

Comment:

Replying to [comment:2 Jacob Walls]:

> Hi, thanks for the report, and thanks Clifford for triage. Clifford, I
don't know if you saw the closely related #36922, which we closed since it
was the responsibility of the custom lookup to return params in a tuple.

That is what is written in the release notes, but the reality is
overwrites of `BuiltinLookup.process_lhs` need to return a list (NOT a
tuple) or it breaks `BuiltinLookup.as_sql`.

> We chose not to backport since this should only be an issue if you've
subclassed the lookups and added further customizations. Your link doesn't
go to a custom lookup, so I can't tell how you're affected. Could you
please include a stacktrace and the custom lookup?

What do you mean "Your link doesn't go to a custom lookup"? The linked
django-netfields lines are obviously custom lookups, which are registered
with `CidrAddressField` and `InetAddressField` (I hope you don't need the
link for that), and they are used with something like this:
`CharFilter(field_name='cidr_address_field', lookup_expr='iregex')`.

> We have to weigh the potential upside from backporting against the
downside of introducing unexpected problems in projects that already
tested against the 6.0 betas. Eager to see the fuller stacktrace & lookup,
and we can look again at that point. Thanks.

The backtrace is not interesting at all, since it just points to
`params.extend(rhs_params)` in `BuiltinLookup.as_sql` (about 10 frames
deep in django); I traced it down to where the tuple params came from
(i.e. `netfields.lookups.IRegex`), and that is what I reported here. Also
it is just obvious from the code it can't work that way.

Now the linked pull-request not only replaces the broken `params.extend`
lines but also changes the return type from list to tuple in a few places
- I suppose the latter ones could be omitted in the backport, but
`BuiltinLookup.as_sql` (and `YearLookup.as_sql`) really needs to handle
tuple params from `process_lhs`.

If you still think this isn't your bug to fix I'll forward this to
netfields; I already replaced the lookup in my own code so I don't really
need a fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/36934#comment:3>

[Django]

#36934: BuiltinLookup breaks with params-as-a-tuple in django 6.0
-------------------------------------+-------------------------------------
Reporter: Stefan Bühler | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | 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 Jacob Walls):

> What do you mean "Your link doesn't go to a custom lookup"?

You're right that my statement lacked some precision. Your link goes to a
mixin that is inherited by custom lookups with no further customization,
which was not what I was expecting to see. I was expecting to see a custom
lookup that was manipulating parameters, like #36922. Instead, the mixin
only manipulates the SQL statements. So, if there is a bug here, it would
seem possible to construct it without django-netfields or any third party
package. That would be a slam-dunk case for backporting, no question about
it. This is why I wanted to see the stacktrace, including the original
queryset call that reproduced the issue. That would indeed be
"interesting". I'm even volunteering to reverse engineer the problematic
queryset call, but I need a place to start.

Until someone can provide that, I can't move this forward.

> Also it is just obvious from the code it can't work that way.

> BuiltinLookup.as_sql (and YearLookup.as_sql) really needs to handle
tuple params from process_lhs.

Sorry, it's not obvious to me. `BuiltinLookup.as_sql` returns a list of
params in Django 6.0. Where is a tuple returned? From a custom lookup?
(I'm begging someone to show me.)
--
Ticket URL: <https://code.djangoproject.com/ticket/36934#comment:4>