-
Notifications
You must be signed in to change notification settings - Fork 1.5k
DATAJPA-1544 - Delete totals.size() == 1 condition in count method #388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
class JpaQueryExecution. The totals.get(0) cannot always get the correct count result no matter whether there is a group by clause or not. There is also misunderstanding when we use getTotalElements method for paging.
This change causes a couple of test failures in @JIANGc can you take a look if this can be saved? |
Thanks @schauder . I made some updates and fixed unit tests. |
@@ -204,13 +210,33 @@ protected Object doExecute(final AbstractJpaQuery repositoryQuery, final Object[ | |||
} | |||
|
|||
private long count(AbstractJpaQuery repositoryQuery, Object[] values) { | |||
|
|||
StringBuilder builder = new StringBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I don't really grasp what is going on here.
I guess it starts with the previously existing version of the source code, which is bogus AFAIK since a count query must always return exactly one value.
Therefore this PR should be done on top of #400.
Any parsing of queries should happen where all the other parsing happens inside AstractJpaQuery
derivatives.
And finally and possibly most crucial: The problem is that the generation of the count query is broken so that is the location that should get fixed.
As stated, this needs to be redone, on top of #400. I think starting from there with a fresh PR, will do much better. |
The totals.get(0) cannot always get the correct count result no matter
whether there is a group by clause or not. There is also
misunderstanding when we use getTotalElements method for paging.