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 by Josh Smeaton, 8 years ago

Easy pickings: set

comment:2 by Shekhar Singh Choudhary, 8 years ago

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

comment:3 by Josh Smeaton, 8 years ago

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 by Shekhar Singh Choudhary, 8 years ago

Owner: Shekhar Singh Choudhary removed
Status: assignednew

comment:5 by Kai Feldhoff, 8 years ago

Owner: set to Kai Feldhoff
Status: newassigned

comment:6 by Kai Feldhoff, 8 years ago

Has patch: set

comment:7 by Kai Feldhoff, 8 years ago

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

comment:8 by Tim Graham, 8 years ago

Easy pickings: unset
Patch needs improvement: set

Comments for improvement are on the pull request.

comment:9 by Kai Feldhoff, 8 years ago

Patch needs improvement: unset

comment:10 by Josh Smeaton, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by Josh Smeaton, 8 years ago

Triage Stage: Ready for checkinAccepted

comment:12 by Tim Graham, 8 years ago

Patch needs improvement: set

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

In baa8b0ec:

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

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

In 5ca08f7c:

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

comment:15 by Tim Graham, 8 years ago

Patch needs improvement: unset

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

Resolution: fixed
Status: assignedclosed

In 53361589:

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

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

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