Opened 9 years ago
Closed 8 years ago
#25258 closed Cleanup/optimization (duplicate)
ExpressionWrapper documentation should be expanded
Reported by: | Joey Wilhelm | Owned by: | nobody |
---|---|---|---|
Component: | Documentation | Version: | 1.8 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Currently, ExpressionWrapper has very limited documentation and is only mentioned for use specifically in conjunction with F().
I ran into a case, using what seems a fairly simple annotation on the surface, where I needed to use an ExpressionWrapper, but this was not at all clear from either the error message, any bug reports, or the documentation.
The following:
Category.objects.annotate( has_children=( Q(children__merchants__isnull=False) | Q(children__children__merchants__isnull=False) ) )
was producing the error:
AttributeError: 'WhereNode' object has no attribute 'output_field'
What I ultimately needed was:
Category.objects.annotate( has_children=ExpressionWrapper( Q(children__merchants__isnull=False) | Q(children__children__merchants__isnull=False), output_field=BooleanField() ) )
However, I only stumbled upon this solution completely by accident and on a hunch.
I think that the documentation for ExpressionWrapper() should be expanded to cover a more broad use case, and perhaps a more clear error message could be provided in cases such as the one I encountered.
Change History (4)
comment:1 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Bug → Cleanup/optimization |
comment:2 by , 9 years ago
Q()
objects aren't actually expressions, but they are made from similar base types, and support parts of the expression API by virtue of being required by Case Expressions
https://docs.djangoproject.com/en/1.8/ref/models/conditional-expressions/.
I've seen quite a few cases of people using Q()
objects as expressions lately, when they really should be using Conditional Expressions instead.
So, I agree that something here needs to be fixed or documented. But ExpressionWrapper
really is only meant to be used to wrap existing expressions, so I'm not too keen on documenting ExpressionWrapper further at this point.
I guess we could define the output_field of Q() objects as always returning a BooleanField but, again, they aren't exactly Expressions themselves although they are close.
I think the better options here are:
1) Make Q() objects actual expressions (by implementing the full expression API on them). Not sure if Q() expressions will always work as annotations though, conditional expressions had some extra work to do to be compliant cross database.
2) Highlight that Conditional Expressions should be used in the documentation and error messages where Q() objects are mentioned. Erroring early on direct Q() usage in annotations and guiding the user to Case/When is what I imagine here.
I'm open to further suggestions.
comment:3 by , 9 years ago
I would actually be a fan of the first option as, if I'm not mistaken, that would produce the expected behavior from my original snippet. I can see how a Case/When could be made to work in this case, but it doesn't seem as natural. Of course, this is also the first time I'm even hearing of those being available in Django.
comment:4 by , 8 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
It looks like formal support for Q
objects in expressions will be added in #27021, so I'll close this as a duplicate.
If you could provide a patch based on what you have learned that would be great.