Opened 5 years ago

Closed 5 years ago

Last modified 5 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: master
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 Changed 5 years ago by David De Sousa

Cc: davidesousa@… added

comment:2 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Kristof Claes

Owner: changed from nobody to Kristof Claes
Status: newassigned

comment:4 Changed 5 years ago by David De Sousa

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 Changed 5 years ago by Kristof Claes

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

comment:6 Changed 5 years ago by Michael Manfre

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 Changed 5 years ago by Carl Meyer

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 Changed 5 years ago by Michael Manfre

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

comment:9 Changed 5 years ago by Kristof Claes

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 Changed 5 years ago by Carl Meyer

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 Changed 5 years ago by Kristof Claes

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 Changed 5 years ago by Carl Meyer

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 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Tim Graham

Easy pickings: unset

comment:15 Changed 5 years ago by Tim Graham

Patch needs improvement: unset

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

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 Changed 5 years ago by Tim Graham <timograham@…>

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 Changed 5 years ago by Tim Graham

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