Fixed #24835 - exists() incorrect after annotation with Count() #4706
Conversation
|
I think the approach you've taken is mostly correct. It's unfortunate that similar processing has to be done in so many places, but that's a task for another time. I feel like the majority of the work should be performed by |
|
I second jarshwah's opinion - the logic should be in has_results(), not in exists(). (Seems like the ._values() logic should also be in sql.Query class, but this doesn't need to be solved here). We could even implement a dedicated helper method to be used both from ._values() and has_results(). |
|
Please also add 1.8.3 release notes. |
I moved it to has_results().
Done. |
| @@ -9,4 +9,7 @@ Django 1.8.3 fixes several bugs in 1.8.2. | |||
| Bugfixes | |||
| ======== | |||
|
|
|||
| * Fixed `exists()` returning incorrect results after annotation with `Count()` | |||
There was a problem hiding this comment.
Need double backticks to have an effect on the rendered docs.
|
Could you rebase and squash your commits? Thanks. |
QuerySet.exists() incorrectly handled query.group_by = True case (grouping by all select fields), causing GROUP BY expressions to be wiped along with select fields.
|
Rebased on master and squashed. As a rule, should I do that with every update or only at the end of review process? |
|
For small patches, squashing after each update is fine. For larger patches, it can be helpful to review only the changes. |
|
|
LGTM
| @@ -489,6 +489,9 @@ def has_filters(self): | |||
| def has_results(self, using): | |||
| q = self.clone() | |||
| if not q.distinct: | |||
| if q.group_by is True: | |||
There was a problem hiding this comment.
Yes, because it can also be "truthy" by containing a list of fields. True means all fields. FWIW I also tested objects.values().annotate().exists() and that was fine before and after this patch.
|
I've opened a new ticket for cleaning up the _values() method and potential factoring with has_results here: https://code.djangoproject.com/ticket/24854 |
|
merged in 801a84a, thanks! |
https://code.djangoproject.com/ticket/24835
I followed the same approach as in
QuerySet._values(). Seems hacky but I couldn't figure out a better way.