Fix bug in count queries retrieving object data #678
Conversation
Codecov Report
@@ Coverage Diff @@
## master #678 +/- ##
=========================================
Coverage 52.62% 52.62%
Complexity 1680 1680
=========================================
Files 131 131
Lines 10120 10120
Branches 1409 1409
=========================================
Hits 5326 5326
Misses 4353 4353
Partials 441 441
Continue to review full report at Codecov.
|
|
Were you able to inspect the JSON response and verify only the counts being returned now? |
|
@Jawnnypoo yes, now the response is something like |
|
Wonder if this has been broken from day one? Good fix 👍 |
| } | ||
|
|
||
| if (count) { | ||
| parameters.put("count", Integer.toString(1)); | ||
| parameters.put(KEY_COUNT, Integer.toString(1)); |
There was a problem hiding this comment.
The default limit is -1 if not set.
https://github.com/parse-community/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ParseQuery.java#L347
So, if there is no limit in the query (will be assigned -1), then there should be a logic that sets limit to 0?
parameters.put(KEY_LIMIT, Integer.toString(0));
There was a problem hiding this comment.
Actually, we don't need to fetch any objects when counting, right?
So, the the limit should always be set to 0 here.
There was a problem hiding this comment.
Right. But with our exposed APIs in ParseQuery limit is already 0 for count actions https://github.com/parse-community/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ParseQuery.java#L1300 see other comment
| // but that logic is in ParseQuery class and we should not do that again here. | ||
| int limit = state.limit(); | ||
| if (limit >= 0) { | ||
| parameters.put(KEY_LIMIT, Integer.toString(limit)); | ||
| } |
There was a problem hiding this comment.
I think it can be moved back to original logic.
If count is true, always set limit to 0.
There was a problem hiding this comment.
There was a problem hiding this comment.
Your link is a good reference because it says
With a nonzero limit, that request would return results as well as the count.
So parse-server supports counting and finding altogether. We don’t currently support this with our exposed APIs, but we might want one day in the future.
So since parse-server supports it and ParseRESTQueryCommand is a low level interface to reach the API, I think it should allow any limit.
The important thing is that any call to ParseQuery.count() will use limit = 0. And that is already true https://github.com/parse-community/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ParseQuery.java#L1300 :
final State<T> state = copy.setLimit(0).build();
So the logic if (count) limit = 0 is already in the count() methods. The issue was that ParseRESTQueryCommand was ignoring limit = 0 imposed by ParseQuery.
There was a problem hiding this comment.
@natario1 Thanks for your explanation and I agree your opinion. Moving this logic out would be the minimum modification to fix this issue.
This fixes #495 , an issue with
count()queries that were fetching the whole objects as well as the count. I assume count queries will be much faster now.