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.

Sign up for GitHub

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

Jump to bottom

Allow IN queries with arrays of documentIds #560

Merged
merged 4 commits into from
Jun 24, 2019
Merged

Allow IN queries with arrays of documentIds #560

merged 4 commits into from
Jun 24, 2019

Conversation

thebrianchen
Copy link

Notes for @mikelehen:

  • I'm not a huge fan of validateAndCalculateReferenceValue, but I'm not sure how else to refactor it without having almost identical code copied.
  • The error messages for IN queries are now slightly off. What is the best way to customize messages for IN queries using documentIds? At this point, if we want different error messages, is it better to just have separate code blocks instead of trying to force the logic into a refactored validateAndCalculateReferenceValue?

@googlebot googlebot added the cla: yes Override cla label Jun 21, 2019
mikelehen
mikelehen suggested changes Jun 22, 2019
View reviewed changes
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.

Some minor suggestions, but I think this basically looks good to me. There might be some clever better way to organize, but it didn't jump out at me.

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated
+ ").");
if (op == Operator.IN) {
validateDisjunctiveOperatorValueArray(value, op);
List referenceList = new ArrayList();
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer generic versions of List<> for type-safety, so this would be List<ReferenceValue>.

Copy link
Author

Choose a reason for hiding this comment

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

ArrayValue.fromList() requires List<FieldValue>, so I'm using that instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, if we wanted to make this work, we could change ArrayValue to wrap a List<? extends FieldValue> instead of List<FieldValue> (read https://dzone.com/articles/covariance-and-contravariance for context). Probably fine to leave as-is though.

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen Jun 22, 2019
thebrianchen
thebrianchen commented Jun 24, 2019
View reviewed changes
Copy link
Author

@thebrianchen thebrianchen left a comment

Choose a reason for hiding this comment

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

Once again, thanks for the detailed review and explanations. Learning a lot :)

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated
+ ").");
if (op == Operator.IN) {
validateDisjunctiveOperatorValueArray(value, op);
List referenceList = new ArrayList();
Copy link
Author

Choose a reason for hiding this comment

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

ArrayValue.fromList() requires List<FieldValue>, so I'm using that instead.

mikelehen
mikelehen suggested changes Jun 24, 2019
View reviewed changes
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.

Couple nits left.

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated
+ ").");
if (op == Operator.IN) {
validateDisjunctiveOperatorValueArray(value, op);
List referenceList = new ArrayList();
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, if we wanted to make this work, we could change ArrayValue to wrap a List<? extends FieldValue> instead of List<FieldValue> (read https://dzone.com/articles/covariance-and-contravariance for context). Probably fine to leave as-is though.

@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen Jun 24, 2019
@thebrianchen
Copy link
Author

I think I renamed validateDisjunctiveFilterElements but then escaped out of it. I still respect the review, for it is sacred.

@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen Jun 24, 2019
@thebrianchen thebrianchen merged commit 1eff916 into master Jun 24, 2019
@thebrianchen thebrianchen mentioned this pull request Jun 24, 2019
Merged
@thebrianchen thebrianchen deleted the bc/in-keys branch June 24, 2019 23:02
schmidt-sebastian pushed a commit that referenced this pull request Jun 27, 2019
@schmidt-sebastian
Allow IN queries with arrays of documentIds (#560)
afc4382
@thebrianchen thebrianchen mentioned this pull request Aug 2, 2019
Merged
@firebase firebase locked and limited conversation to collaborators Oct 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mikelehen mikelehen mikelehen approved these changes

Assignees

@thebrianchen thebrianchen

Labels
api: firestore cla: yes Override cla size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thebrianchen @mikelehen @googlebot @google-oss-bot