fix: Parse.Query.findAll returns empty array calling withCount#2854
fix: Parse.Query.findAll returns empty array calling withCount#2854dplewis wants to merge 6 commits intoparse-community:alphafrom
Parse.Query.findAll returns empty array calling withCount#2854Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughChange query APIs so Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #2854 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 6235 6240 +5
Branches 1477 1495 +18
=========================================
+ Hits 6235 6240 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/EventuallyQueue.ts (1)
345-353: Prefer unwrappingfind’s union result instead ofany
query.findnow returnsT[] | { results: T[]; count: number }, butresultsis typed asanyand used asresults[0]. That works today (nowithCount/jsonhere) but hides type issues if this ever changes.You can keep this future‑proof and typed without
any:Suggested unwrapping
- const results: any = await query.find({ sessionToken }); - if (results.length > 0) { - return EventuallyQueue.sendQueueCallback(results[0], queueObject); - } + const response = await query.find({ sessionToken }); + const results = Array.isArray(response) ? response : response.results || []; + if (results.length > 0) { + return EventuallyQueue.sendQueueCallback(results[0], queueObject); + }types/ParseQuery.d.ts (1)
228-231: Updatedfind/findAlltypings correctly model count-wrapped resultsThe new union return types for
findandfindAllalign with the runtime behavior whenwithCountis used and with the implementation insrc/ParseQuery.ts. This is a reasonable breaking typing change and matches the tests’ expectations.If you want to reduce duplication later, you could alias the wrapped shape (e.g.,
type FindWithCountResult<T> = { results: T[]; count: number }) and reuse it here, but that’s purely cosmetic.Also applies to: 247-250
types/tests.ts (1)
907-932: Minor mismatch betweenFieldTypeandnotGeopointExclude target
FieldTypenow includesParse.GeoPoint(notParse.GeoPoint[]), butnotGeopointis defined asExclude<FieldType, Parse.GeoPoint[]>. This still makes the$ExpectErrorcall behave as intended, but the type is misleading.You can tighten this to reflect the actual union:
Optional cleanup
- notGeopoint: Exclude<FieldType, Parse.GeoPoint[]>, + notGeopoint: Exclude<FieldType, Parse.GeoPoint>,
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
integration/test/ParseQueryTest.jssrc/EventuallyQueue.tssrc/ParseQuery.tstypes/ParseQuery.d.tstypes/tests.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/EventuallyQueue.ts (1)
src/ParseRelation.ts (1)
query(120-141)
integration/test/ParseQueryTest.js (2)
src/ParseRelation.ts (1)
query(120-141)src/ParseQuery.ts (1)
count(765-780)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (Node 24, 24.1.0)
- GitHub Check: build (Node 20, 20.19.0)
- GitHub Check: build (Node 22, 22.12.0)
🔇 Additional comments (7)
types/tests.ts (1)
2048-2053: Type test expectations forfind/findAllmatch new union returnThe updated
$ExpectTypeannotations forqueryUntyped.find(),queryUntyped.findAll(), andqueryTyped.find()correctly capture the newT[] | { results: T[]; count: number }return type shape and keep the typings in sync withParseQuery.d.tsand the implementation.Also applies to: 2063-2064
src/ParseQuery.ts (3)
678-721:findunion return type is consistent with runtime behaviorThe updated
findsignature (Promise<T[] | { results: T[]; count: number }>), plus the conditional{ results, count }wrapping whenresponse.countis present, matches how the method already behaves (including offline queries via_handleOfflineQuery). No issues spotted here.
739-748:findAllcorrectly returns{ results, count }whenwithCountis enabledThe new
findAllimplementation:
- Uses
eachBatchto load all pages.- Aggregates all objects into a single
resultsarray.- When
_countis true (fromwithCount(true)), returns{ results, count: results.length }; otherwise returnsresultsdirectly.This satisfies the desired behavior for
findAll+withCountwithout coupling to server-sidecount, and preserves existingjsonbehavior.
936-944: Unwrapping{ results, count }ineachBatchfixes the pagination bugThe new unwrapping:
const [response] = await Promise.all([query.find(findOptions), ...]); let results: any = response; if (results.results) { results = results.results; }ensures that
resultsis always an array before.lengthis used for pagination. This removes the previous failure mode wherewithCount(true)causedresultsto be an object andresults.lengthwasundefined, short-circuitingfindAllwith an empty result.integration/test/ParseQueryTest.js (3)
55-63: Raw JSON tests now exercise bothfindandfindAllUsing
let results = await query.find({ json: true });followed byresults = await query.findAll({ json: true });and asserting onresults[0]validates that both methods return raw JSON whenjson: trueandwithCountis not used. This aligns with the implementation and helps guard the json-path behavior after the union-type changes.
110-127: NewfindAll+withCounttest directly covers the original bug
it('can do findAll query with count', ...)exercises:
- Creating multiple matching objects.
- Calling
query.withCount(true);- Using
const { results, count } = await query.findAll();and asserts both
results.lengthandcountare 4. This is exactly the scenario that was previously returning an empty array due to the pagination bug, so this test is a solid regression guard.
2419-2429:explain(false)test confirms explicit disable of explain modeThe
it('can query with explain false', ...)case verifies that callingquery.explain(false);restores normalfind()behavior (array of 3 objects, not an explain plan). This matches theexplain(explain = true)implementation and ensures toggling the flag works as intended.
|
@mtrezza Is this good to merge? |
|
@coderabbitai, is such a change in return type replicable to Parse SDKs with strongly typed languages like Swift? To have a method return multiple types looks problematic. Do we have any examples of doing that in other places in this repo for public APIs of Parse.Query? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2922 🏁 Script executed: Length of output: 5360 🏁 Script executed: Length of output: 1237 🏁 Script executed: Length of output: 2853 🌐 Web query: 💡 Result: In Parse (including the iOS SDKs), a “
In the ObjC-based Parse iOS SDK (
Typical
|
|
This goes back to the old issue in the parse-server repo (can't find it right now) that it's a conceptual mistake to have an array as a response object. Parse Server does this at the moment and it's responsible for a bunch of limitations. Generally, we are moving towards type safety, so a type ambiguity as suggested here is contrary to that. If an API is only implementable in 1 SDK because it doesn't enforce types, while not being able to add it also to other strongly typed SDKs in the future, then there's no point in adding it as it proofs that it's conceptually wrong. People can always use the REST API, and it's also a somewhat superfluous option if count is equal the array length. Maybe we should deprecate the |
This is true for findAll() or if find() returns all the objects.
When a dev use |
|
.withCont additionally returns the total number of objects that match the query, regardless of skip and limit params? |
|
Yes, there are tests cases for it. |
|
Then this makes sense; but the issue is still, if we can't add it to other SDKs because of type issues, then there's no point in adding it only to the JS SDK. Maybe we need a new API for this? |
The other SDK's can do what the Swift SDK does for this. Instead of |
|
So withCount there is a distinct method, like |
|
Yeah |
|
For JS SDK that would be const query = new Parse.Query(TestObject);
const results = await query.withCount();instead of const query = new Parse.Query(TestObject);
query.withCount(true);
const results = await query.findAll();? |
|
Yeah but that would be a breaking change |
|
That would be fine as we avoid the type ambiguity. A breaking change should be acceptable, since it's unrelated to Parse Server and easy to roll out. We're really correcting this type ambiguity that has been introduced before. |
|
Cool, I’m going to close this |
|
Yes, I've summarized the discussion in a comment on the issue. |
|
@mtrezza You got the PR? |
|
What PR? |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/ParseQuery.ts`:
- Line 678: The change to ParseQuery.find from Promise<T[]> to Promise<T[] | {
results: T[]; count: number }> introduces a breaking discriminated union; revert
the find signature back to find(options?: QueryOptions): Promise<T[]> and
implement a new method (e.g., findWithCount(options?: QueryOptions): Promise<{
results: T[]; count: number }>) that returns the results + count wrapper,
updating the ParseQuery class methods accordingly (adjust implementations
currently in find to be reused by findWithCount) and update any
callers/tests/types to use the new findWithCount when they need the count.
- Around line 739-748: The current findAll in ParseQuery returns a fabricated
count (results.length) instead of the server-provided total; update findAll to
capture and return the server count supplied by the first batch response rather
than computing results.length. Specifically, change the eachBatch usage so the
batch callback (from eachBatch) returns or exposes server metadata (e.g.,
totalCount/ count from the server) and in findAll capture that value on the
first invocation (store in a local variable like serverCount) and, when
this._count is true, return { results, count: serverCount } instead of count:
results.length; adjust eachBatch (and its callback signature) to provide the
server count if it currently only yields objects.
- Around line 936-943: The cloned query used by eachBatch (created via
fromJSON(this.className, this.toJSON())) is preserving the internal _count flag
and causing every query.find call to send count=1; strip or delete the `_count`
property from the cloned query before paginating so paged requests don't force a
total-count compute. Also make the response unwrapping in the Promise.all result
more robust by detecting whether the response is an array (e.g., use
Array.isArray(response)) instead of relying on the presence of
`results.results`; update the logic around `query.find`, `response`, and
`results` to unwrap only when the response is not an Array.
🧹 Nitpick comments (1)
src/ParseQuery.ts (1)
740-742: Accumulating results with spread in a loop is O(n²).
results = [...results, ...objects]on every batch copies the entire accumulated array each time. For large datasets (which is the whole point offindAll), this becomes expensive. Usepushwith spread orArray.prototype.concatand reassign.Proposed fix
- let results: T[] = []; await this.eachBatch((objects: T[]) => { - results = [...results, ...objects]; + results.push(...objects); }, options);
| * the query completes. | ||
| */ | ||
| find(options?: QueryOptions): Promise<T[]> { | ||
| find(options?: QueryOptions): Promise<T[] | { results: T[]; count: number }> { |
There was a problem hiding this comment.
Union return type is a breaking change for TypeScript consumers.
Changing find() from Promise<T[]> to Promise<T[] | { results: T[]; count: number }> forces every caller to narrow the type before accessing array methods. This was already flagged in the PR discussion by @mtrezza as conceptually problematic and inconsistent across SDKs. Consider a separate method (e.g., findWithCount()) or always returning a wrapper to avoid a discriminated union.
🤖 Prompt for AI Agents
In `@src/ParseQuery.ts` at line 678, The change to ParseQuery.find from
Promise<T[]> to Promise<T[] | { results: T[]; count: number }> introduces a
breaking discriminated union; revert the find signature back to find(options?:
QueryOptions): Promise<T[]> and implement a new method (e.g.,
findWithCount(options?: QueryOptions): Promise<{ results: T[]; count: number }>)
that returns the results + count wrapper, updating the ParseQuery class methods
accordingly (adjust implementations currently in find to be reused by
findWithCount) and update any callers/tests/types to use the new findWithCount
when they need the count.
There was a problem hiding this comment.
I think we could actually introduce a method findWithCount() for this, to not mess up the types while providing some relieve for developers - what do you think @dplewis?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| async findAll(options?: BatchOptions): Promise<T[] | { results: T[], count: number}> { | ||
| let results: T[] = []; | ||
| await this.eachBatch((objects: T[]) => { | ||
| result = [...result, ...objects]; | ||
| results = [...results, ...objects]; | ||
| }, options); | ||
| return result; | ||
| if (this._count) { | ||
| return { results, count: results.length }; | ||
| } | ||
| return results; | ||
| } |
There was a problem hiding this comment.
findAll with withCount returns a fabricated count instead of the server-provided total.
count: results.length (line 745) is the length of the locally accumulated array, not the server's authoritative count. While these should converge after all pages are fetched, it diverges from the semantics of find().withCount() where the count is server-computed (and can differ from results.length if, e.g., ACLs filter rows at query time vs. cursor time).
If withCount support is kept in findAll, consider capturing the server count from the first batch response instead.
🤖 Prompt for AI Agents
In `@src/ParseQuery.ts` around lines 739 - 748, The current findAll in ParseQuery
returns a fabricated count (results.length) instead of the server-provided
total; update findAll to capture and return the server count supplied by the
first batch response rather than computing results.length. Specifically, change
the eachBatch usage so the batch callback (from eachBatch) returns or exposes
server metadata (e.g., totalCount/ count from the server) and in findAll capture
that value on the first invocation (store in a local variable like serverCount)
and, when this._count is true, return { results, count: serverCount } instead of
count: results.length; adjust eachBatch (and its callback signature) to provide
the server count if it currently only yields objects.
| const [response] = await Promise.all([ | ||
| query.find(findOptions), | ||
| Promise.resolve(previousResults.length > 0 && callback(previousResults)), | ||
| ]); | ||
| let results: any = response; | ||
| if (results.results) { | ||
| results = results.results; | ||
| } |
There was a problem hiding this comment.
Unnecessary count=1 sent on every batch request & fragile unwrapping.
Two concerns here:
-
Performance:
eachBatchclones the query viafromJSON(this.className, this.toJSON())(line 924), which preserves_count. Every paginated batch request sendscount=1to the server, forcing it to compute the total count on each page — completely wasted work. Strip_countfrom the cloned query used for pagination. -
Fragile duck-typing:
results.results(line 941) relies on the assumption that a plainT[]never has a.resultsproperty. This works today but is brittle. A more explicit check (e.g.,!Array.isArray(response)) would be safer.
Proposed fix
const query = ParseQuery.fromJSON(this.className, this.toJSON());
+ query._count = false;
query.ascending('objectId');
query._limit = options.batchSize || 100;And for the unwrapping:
- let results: any = response;
- if (results.results) {
- results = results.results;
- }
+ const results: T[] = Array.isArray(response)
+ ? response
+ : (response as { results: T[] }).results;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [response] = await Promise.all([ | |
| query.find(findOptions), | |
| Promise.resolve(previousResults.length > 0 && callback(previousResults)), | |
| ]); | |
| let results: any = response; | |
| if (results.results) { | |
| results = results.results; | |
| } | |
| const [response] = await Promise.all([ | |
| query.find(findOptions), | |
| Promise.resolve(previousResults.length > 0 && callback(previousResults)), | |
| ]); | |
| const results: T[] = Array.isArray(response) | |
| ? response | |
| : (response as { results: T[] }).results; |
🤖 Prompt for AI Agents
In `@src/ParseQuery.ts` around lines 936 - 943, The cloned query used by eachBatch
(created via fromJSON(this.className, this.toJSON())) is preserving the internal
_count flag and causing every query.find call to send count=1; strip or delete
the `_count` property from the cloned query before paginating so paged requests
don't force a total-count compute. Also make the response unwrapping in the
Promise.all result more robust by detecting whether the response is an array
(e.g., use Array.isArray(response)) instead of relying on the presence of
`results.results`; update the logic around `query.find`, `response`, and
`results` to unwrap only when the response is not an Array.
Pull Request
Issue
Closes: #2621
Approach
withCountSummary by CodeRabbit
New Features
Tests