Opened 7 years ago

Closed 7 years ago

#28731 closed Bug (fixed)

Passing an empty Q() to a When inside a Case causes an OperationError

Reported by: Tom van Bussel Owned by: Tim Martin
Component: Database layer (models, ORM) Version: 1.11
Severity: Normal Keywords:
Cc: Adam Johnson Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tom van Bussel)

The following code causes an OperationError:

q = Q() # Dynamically constructed, possibly empty
x = TestModel.objects.annotate(test_name=Case(When(q, then=Value(True)), default=Value(False), output_field=BooleanField())).all()
print(x)

The code above results in a SQL query which contains CASE WHEN THEN, with nothing between the WHEN and THEN.

Change History (17)

comment:1 by Tom van Bussel, 7 years ago

Description: modified (diff)

comment:2 by Tomer Chachamu, 7 years ago

Thanks for the bug report.

What did you expect to be the value of x[0].test_name?

We don't have any documentation on how a Q(**kwargs) object behaves when len(kwargs) == 0, and we have no tests for it. Perhaps it should be raising an exception (or initially a deprecation warning).

in reply to:  2 comment:3 by Tom van Bussel, 7 years ago

Replying to Tomer Chachamu:

What did you expect to be the value of x[0].test_name?

I expected x[0].test_name == True to hold.

We don't have any documentation on how a Q(**kwargs) object behaves when len(kwargs) == 0, and we have no tests for it. Perhaps it should be raising an exception (or initially a deprecation warning).

Currently TestModel.objects.filter(Q()).all() == TestModel.objects.all(), so it would be nice if that behavior is matched everywhere.

comment:4 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Adam Johnson, 7 years ago

it would be nice if that behavior is matched everywhere.

I agree, probably Q() should result in SQL something like 1=1.

comment:6 by Adam Johnson, 7 years ago

Cc: Adam Johnson added

comment:7 by Tim Martin, 7 years ago

Owner: changed from nobody to Tim Martin
Status: newassigned

comment:8 by Tomer Chachamu, 7 years ago

Currently Q() | Q(value=1) outputs the same SQL as Q(value=1) which will not be the case if Q() is effectively always true.

comment:9 by Tim Martin, 7 years ago

Worse, you get the same result for Q() & Q(value=1) as you do for Q() | Q(value=1), because empty Q() expressions are ignored when combining (query_utils.py:68). I don't think there's an easy way to make this consistent.

I'm guessing it should be possible to make the originally reported case not trigger invalid SQL, which is a step forward. Maybe it should report this as an invalid expression?

comment:10 by Adam Johnson, 7 years ago

Those are some good points Tim/Tomer. I agree that making When() raise an error when passed an empty Q would make sense.

comment:11 by Tomer Chachamu, 7 years ago

If the usage of Q() is deprecated and will be removed then there's no need for it to create valid SQL (in more situations than it does currently). We could add tests for the existing behaviour of Q() & Q(value=1) and of Q() | Q(value=1).

comment:12 by Tim Martin, 7 years ago

I don't think it's explicitly deprecated, we'd need to have a discussion on the mailing list to get consensus on that.

However, I think reporting an explicit "empty Q() object not supported" error is much better than sending invalid SQL to the database and getting it rejected. I don't think the DB layer should ever send invalid SQL.

comment:13 by Tim Martin, 7 years ago

After a bit of digging, I found the rationale for this behaviour in #28211. The argument seems to be that it's common to do something like:

filters = Q()
if condition:
   filters |= Q(x=1)
if other_condition:
   filters |= Q(y=2)

I'm not sure I agree that this is the right design but there is at least a rationale here, and it's been like this for many years now so I don't think changing it is viable. This rationale supports the idea that empty Q() objects are legitimate steps in building up a query, but shouldn't be used directly. I'll make this report an explicit error rather than invalid SQL.

comment:14 by Tim Martin, 7 years ago

Has patch: set

I've added a pull request that makes this raise an exception.

comment:15 by Tim Martin, 7 years ago

FWIW, this should be ready to re-review.

comment:16 by Simon Charette, 7 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In 5778b57:

Fixed #28731 -- Added an error message when using an empty Q() in a When expression.

Otherwise it generates invalid SQL.

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