Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#25759 closed New feature (fixed)

Add **kwargs to as_sql method of Expressions

Reported by: Josh Smeaton Owned by: Kai Feldhoff
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: expressions
Cc: josh.smeaton@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

A common pattern when overriding as_sql or implementing as_vendor with expressions is to modify the template or function attributes before rendering the expression. This isn't safe to do, because expressions can be shared between backends.

It'd be a good idea to add **kwargs to the method signature, so that overrides can add context to that specific method call without mutating the expression itself. For expressions like Func, it could mix the kwargs with self.extra during rendering. Unsure how **kwargs would affect other expression types, but that can be opt in per type.

Once this is done, the documentation written for https://github.com/django/django/pull/5574 for third party backend overrides will need to be updated to take advantage of the new kwargs.

Change History (17)

comment:1 Changed 8 years ago by Josh Smeaton

Easy pickings: set

comment:2 Changed 8 years ago by Shekhar Singh Choudhary

Owner: changed from nobody to Shekhar Singh Choudhary
Status: newassigned

comment:3 Changed 8 years ago by Josh Smeaton

Some notes for completing this ticket:

Start by adding kwargs to the method signature of as_sql for all expression types, including transforms and lookups. There are quite a few places.

For Func, get rid of the function and template key word arguments, and merge self.extra with kwargs in a local variable (so you don't modify self.extra), using that to generate the sql string.

Need to update docs to reference the new kwargs argument and change any tests that override as_sql for vendor specific tests so that they no longer modify self.template or self.function.

comment:4 Changed 8 years ago by Shekhar Singh Choudhary

Owner: Shekhar Singh Choudhary deleted
Status: assignednew

comment:5 Changed 8 years ago by Kai Feldhoff

Owner: set to Kai Feldhoff
Status: newassigned

comment:6 Changed 8 years ago by Kai Feldhoff

Has patch: set

comment:7 Changed 8 years ago by Kai Feldhoff

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:8 Changed 8 years ago by Tim Graham

Easy pickings: unset
Patch needs improvement: set

Comments for improvement are on the pull request.

comment:9 Changed 8 years ago by Kai Feldhoff

Patch needs improvement: unset

comment:10 Changed 8 years ago by Josh Smeaton

Triage Stage: AcceptedReady for checkin

comment:11 Changed 8 years ago by Josh Smeaton

Triage Stage: Ready for checkinAccepted

comment:12 Changed 8 years ago by Tim Graham

Patch needs improvement: set

comment:13 Changed 8 years ago by Tim Graham <timograham@…>

In baa8b0ec:

Refs #25759 -- Fixed some Funcs to work if different database backends are used.

comment:14 Changed 8 years ago by Tim Graham <timograham@…>

In 5ca08f7c:

Refs #25759 -- Documented customizing expressions' SQL on other databases.

comment:15 Changed 8 years ago by Tim Graham

Patch needs improvement: unset

comment:16 Changed 8 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 53361589:

Fixed #25759 -- Added keyword arguments to customize Expressions' as_sql().

comment:17 Changed 7 years ago by Tim Graham <timograham@…>

In d5977e49:

Refs #25759 -- Fixed some GIS Funcs if different database backends are used.

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