BuiltinLookup.as_sql breaks with params-as-a-tuple

[Stefan Bühler]

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.

[Clifford Gama]

Thanks. Leaving as has-patch since as you point out it is already fixed in the main branch but wasn't backported.

[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.

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.

[Stefan Bühler]

Replying to 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.

[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.)

[Clifford Gama]

Hello Jacob. Sorry, I didn't know about #36922 and hadn't thought to check for duplicates.

Sorry, it's not obvious to me. BuiltinLookup.process_lhs returns a list of params in Django 6.0. Where is a tuple returned? From a custom lookup?

I can't work on a reproduction now, but when I was working on something JSONNull related I ran into the problem (which is why I was able to spot it during the ​review). The issue is that if you have a custom lookup that overrides process_lhs(), does something with LHS params but does not override as_sql(), the latter will break because it expects a process_lhs to return a list ​source.

[Jacob Walls]

Thanks, I was beginning to miss the forest for the trees here.

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.

I agree -- we can backport the bit about replacing extend() with the unpacking operator without any risk of regressions. I'll prepare that.

[Jacob Walls]

​PR

[Jacob Walls <jacobtylerwalls@…>]

In f9b820f:

[6.0.x] Fixed #36934, Refs #35972 -- Coped with params in a tuple in BuiltinLookup.as_sql().

For custom lookups subclassing BuiltinLookup and following the advice in the release notes
to return params in a tuple, this change will obviate the need to audit as_sql() in addition to
process_lhs() to be "resilient against either tuples or lists" as described in the release note.

Regression in 8914f4703cf03e2a01683c4ba00f5ae7d3fa449d.

[Jacob Walls <jacobtylerwalls@…>]

In 96984b9:

Refs #36934, #35972 -- Forwardported release note for tolerating sequences in BuiltinLookup.as_sql().

Instead of cherry-picking a larger changeset (787cc96ef6197d73c7d4ad96f25500910c399603)
and removing changes unsuitable for a backport, a partial backport was applied directly
to stable/6.0.x to resolve #36934, so the release note needs to be forwardported.

Forwardport of f9b820f8ac50aad025949087e660a551691832e4 from stable/6.0.x.