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 Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

If you could provide a patch based on what you have learned that would be great.

comment:2 by Josh Smeaton, 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 Joey Wilhelm, 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 Tim Graham, 8 years ago

Resolution: duplicate
Status: newclosed

It looks like formal support for Q objects in expressions will be added in #27021, so I'll close this as a duplicate.

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