#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 , 9 years ago
Cc: | added |
---|
comment:2 by , 9 years ago
Easy pickings: | set |
---|---|
Type: | Uncategorized → Cleanup/optimization |
comment:3 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 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 , 9 years ago
I have created a PR to update the docs: https://github.com/django/django/pull/5574
comment:6 by , 9 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready 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 , 9 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
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 , 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 , 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 , 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 , 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 , 9 years ago
Triage Stage: | Accepted → Ready 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 , 9 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
Left some comments for improvement. Please uncheck "Patch needs improvement" when you update the pull request.
comment:14 by , 9 years ago
Easy pickings: | unset |
---|
comment:18 by , 9 years ago
Summary: | Document how backends should monkey patch expressions → Document how database backends should monkey patch functions |
---|
(leaving the documentation of how to patch expressions to #25759)
It seems the "Technical Information" section of
docs/ref/models/expression.txt
is probably the place to add this.