Skip to content
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

Backport updates from Android (multiple changes) #1912

Merged
merged 6 commits into from
Jun 26, 2019
Merged

Conversation

thebrianchen
Copy link

@thebrianchen thebrianchen commented Jun 24, 2019

Backporting changes from Android IN queries:

  • #519: filter for null and NaN and reorganize tests to match Android SDK
  • #550: update doc comments for clearPersistence() to include Rachel's edits
  • #560: Allow IN queries with field path and array of documentIds

This change introduces KeyFieldInFilter, which stacks on top of @wilhuff's refactor (#1894). This will be ported to Android when he refactors the Android client.

TODO: After this change goes in, I will update the error messages inside parseDocumentIdValue() to match the Android SDK, since those error messages are more generally applicable to both IN queries and KeyFieldFilter queries.

Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Thanks for back-porting everything! You can now call yourself a true Firestore SDK developer. 😛 This mostly looks great, but I found a potential issue (though maybe I'm confused since I thought tests should have caught it 🤔 )...

packages/firestore/src/core/query.ts Show resolved Hide resolved
packages/firestore/src/core/query.ts Outdated Show resolved Hide resolved
packages/firestore/src/core/query.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/validation.test.ts Outdated Show resolved Hide resolved
@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen Jun 25, 2019
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

LGTM with (optional) nit. Please back-back-port the new test to Android. 😁

packages/firestore/src/core/query.ts Show resolved Hide resolved
packages/firestore/src/core/query.ts Outdated Show resolved Hide resolved
@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen Jun 25, 2019
@thebrianchen
Copy link
Author

@mikelehen I just realized that the current implementation of matches in the Android SDK doesn't support the integration test for IN queries on doc IDs yet...

This means that Gil's refactor of filter PR needs to go in first, or else I'll be doing the logic twice. I'll submit this PR for now, and back-back-backport(?) the integration test once the Android refactor of Filter is done to include KeyFieldInFilter.

@mikelehen
Copy link
Contributor

👍

@thebrianchen thebrianchen merged commit 0551030 into master Jun 26, 2019
@thebrianchen thebrianchen deleted the bc/in-backport branch June 26, 2019 00:17
@firebase firebase locked and limited conversation to collaborators Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants