Skip to content

fix(firestore): ensure limit(0) is properly serialized in query requests#8076

Merged
dlarocque merged 2 commits intogoogleapis:mainfrom
liu-husong:main
May 4, 2026
Merged

fix(firestore): ensure limit(0) is properly serialized in query requests#8076
dlarocque merged 2 commits intogoogleapis:mainfrom
liu-husong:main

Conversation

@liu-husong
Copy link
Copy Markdown
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary). I’m not sure if this bugfix requires any doc updates — let me know if I’ve missed anything.

Fixes #7382 🦕

@liu-husong liu-husong requested a review from a team as a code owner April 20, 2026 09:47
@product-auto-label product-auto-label Bot added the api: firestore Issues related to the Firestore API. label Apr 20, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the Firestore query logic to correctly handle a limit of 0 by replacing falsy checks with explicit undefined checks, and adds a corresponding test case. Feedback suggests using nullish equality checks (== null and != null) instead of strict undefined checks to preserve existing behavior for null values and follow the Google TypeScript Style Guide.

Comment thread handwritten/firestore/dev/src/reference/query-util.ts
Comment thread handwritten/firestore/dev/src/reference/query.ts
@dlarocque
Copy link
Copy Markdown
Contributor

Hi @liu-husong thanks for the contribution! This looks good to me- I'll just let the workflows run and then I'll approve this PR.

@liu-husong
Copy link
Copy Markdown
Contributor Author

liu-husong commented Apr 28, 2026

Hi @liu-husong thanks for the contribution! This looks good to me- I'll just let the workflows run and then I'll approve this PR.

Thanks @dlarocque! Quick check — are the workflows manually triggered? If so, did you trigger them already? I’m asking because the status hasn’t updated in a while, and I’m wondering if they’ll progress without intervention.

Also a quick noob question: after you approve the PR, should I go ahead and click “merge,” or is that typically left to maintainers? First time contributing here, so want to make sure I follow the right process.

@dlarocque dlarocque added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 28, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 28, 2026
@dlarocque
Copy link
Copy Markdown
Contributor

Hi @liu-husong thanks for the contribution! This looks good to me- I'll just let the workflows run and then I'll approve this PR.

Thanks @dlarocque! Quick check — are the workflows manually triggered? If so, did you trigger them already? I’m asking because the status hasn’t updated in a while, and I’m wondering if they’ll progress without intervention.

I thought they'd go off manually once I approved the workflows, but it seems I had to manually trigger them using a label. They should run to completion now.

Also a quick noob question: after you approve the PR, should I go ahead and click “merge,” or is that typically left to maintainers? First time contributing here, so want to make sure I follow the right process.

Once CI passes, I'll approve the PR, then you can click "merge". Unless you find that you don't have permissions to do that, in which case I can merge it for you.

liu-husong added 2 commits May 1, 2026 11:58
Previously, calling .limit(0) on a Firestore Query did not serialize
the zero value to the wire protocol, causing queries to be sent without
a limit constraint. This change explicitly includes limit(0) in the
query proto.

Fixes googleapis#7382
@liu-husong
Copy link
Copy Markdown
Contributor Author

@dlarocque Hi — I think all checks have passed, but I’m not seeing the “Merge” button (likely due to permissions).

I tried rebasing to see if that would help, but it put the PR back into pending CI. Could you help apply the label to trigger CI and assist with merging?

Thanks!

@dlarocque dlarocque added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2026
@dlarocque
Copy link
Copy Markdown
Contributor

@dlarocque Hi — I think all checks have passed, but I’m not seeing the “Merge” button (likely due to permissions).

I tried rebasing to see if that would help, but it put the PR back into pending CI. Could you help apply the label to trigger CI and assist with merging?

Thanks!

Sorry about the painful process here. If the workflows pass again and you're still unable to merge, ping me and I'll merge it for you.

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2026
@dlarocque dlarocque merged commit 8631008 into googleapis:main May 4, 2026
27 checks passed
thiyaguk09 pushed a commit to thiyaguk09/google-cloud-node-fork that referenced this pull request May 5, 2026
…sts (googleapis#8076)

Previously, calling .limit(0) on a Firestore Query did not serialize
the zero value to the wire protocol, causing queries to be sent without
a limit constraint. This change explicitly includes limit(0) in the
query proto.

Fixes googleapis#7382
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the Firestore API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Firestore client library's limit API not handling limit(0) correctly

4 participants