Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#25687 closed Cleanup/optimization (fixed)

Document how database backends should monkey patch functions

Reported by: Josh Smeaton Owned by: Kristof Claes
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: davidesousa@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

@dedsm at djangounderthehood brought up that the docs do not currently contain information for how 3rd party backends should be supporting expressions by monkey patching. Let's fix that.

Firebird backend (with connection.vendor == 'firebird') would change Sum like this:

from django.db.models import Sum

def backend_sum(self, compiler, connection)
    # do stuff

Sum.as_firebird = backend_sum

Change History (18)

comment:1 by David De Sousa, 9 years ago

Cc: davidesousa@… added

comment:2 by Tim Graham, 9 years ago

Easy pickings: set
Type: UncategorizedCleanup/optimization

It seems the "Technical Information" section of docs/ref/models/expression.txt is probably the place to add this.

comment:3 by Kristof Claes, 9 years ago

Owner: changed from nobody to Kristof Claes
Status: newassigned

comment:4 by David De Sousa, 9 years ago

wouldn't it be better if the BaseExpression had a public api for this instead of relying on third parties monkey patching by design?

Something like:

from django.db.models import Sum

def backend_sum(self, compiler, connection):
    #do stuff

Sum.add_backend('firebird', backend_sum)

comment:5 by Kristof Claes, 9 years ago

I have created a PR to update the docs: https://github.com/django/django/pull/5574

comment:6 by Michael Manfre, 9 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

I've been discussing with jarshwah and apollo13 about ways of allowing backends to provide vendor specific overrides. They've convinced me that monkey patching is a good approach until/unless this becomes a more common problem.

kirstofclaes, the documentation looks good to me. Thanks!

comment:7 by Carl Meyer, 9 years ago

Triage Stage: Ready for checkinAccepted

I don't think the code in the doc patch is quite right yet.

Also, I'm inclined to agree with @dedsm above -- what's the harm in providing a public API that is effectively equivalent to doing this same monkey-patching "under the hood"( ;-) ). Seems like there'd be no harm, and it would give us more flexibility in the future if this does "become a more common problem."

comment:8 by Michael Manfre, 9 years ago

Missed a mistake in the documentation example. Need to pass template to as_sql so that it is used.

comment:9 by Kristof Claes, 9 years ago

I have changed template to self.template. If I understand the code correctly, this should work as well? Or is passing template to as_sql the preferred solution?

comment:10 by Carl Meyer, 9 years ago

After discussion with jarshwah and manfre, we've agreed that none of us really care that much between simple monkeypatching vs a public API that does the monkeypatching internally. I don't think there's any practical benefit to the public API, even in terms of future flexibility: either way we're committed to the as_vendor method system, whether the method gets there via monkeypatching or otherwise is irrelevant. The only real benefit is that recommending monkeypatching in our documentation looks bad :-)

I think this doc patch should go ahead as-is. If I (or dedsm or anyone else) is motivated enough to submit a PR that adds the three-line public API (could also be a decorator) and modifies the docs accordingly, it would likely be accepted.

comment:11 by Kristof Claes, 9 years ago

I have updated my PR again so it uses template again and passes it to as_sql.

I've been force pushing these changes. If I'm not using the correct workflow here, please let me know. This is my first contribution to OSS ever.

comment:12 by Carl Meyer, 9 years ago

Triage Stage: AcceptedReady for checkin

Force-pushing on every change is fine for a smaller patch like this. Patch looks good to me, thanks for the contribution, and congratulations, welcome to the team :-)

comment:13 by Tim Graham, 9 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Left some comments for improvement. Please uncheck "Patch needs improvement" when you update the pull request.

comment:14 by Tim Graham, 9 years ago

Easy pickings: unset

comment:15 by Tim Graham, 9 years ago

Patch needs improvement: unset

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

Resolution: fixed
Status: assignedclosed

In 88034c99:

Fixed #25687 -- Documented how to add database function support to third-party backends.

Thanks Kristof Claes for the initial patch.

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

In 477b82cf:

[1.9.x] Fixed #25687 -- Documented how to add database function support to third-party backends.

Thanks Kristof Claes for the initial patch.

Backport of 88034c9938d92193d2104ecfe77999c69301dcc1 from master

comment:18 by Tim Graham, 9 years ago

Summary: Document how backends should monkey patch expressionsDocument how database backends should monkey patch functions

(leaving the documentation of how to patch expressions to #25759)

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