Skip to content

Commit 025ef85

Browse files
author
Brian Chen
committed
resolve gil comments
1 parent c5ffda3 commit 025ef85

File tree

7 files changed

+43
-59
lines changed

7 files changed

+43
-59
lines changed

‎firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollectionWithDocs;
2121
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testFirestore;
2222
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.waitFor;
23-
import static com.google.firebase.firestore.testutil.TestUtil.assertArrayEquals;
23+
import static com.google.firebase.firestore.testutil.TestUtil.assertSetEquals;
2424
import static com.google.firebase.firestore.testutil.TestUtil.expectError;
2525
import static com.google.firebase.firestore.testutil.TestUtil.map;
2626
import static java.util.Arrays.asList;
@@ -32,7 +32,6 @@
3232

3333
import androidx.test.ext.junit.runners.AndroidJUnit4;
3434
import com.google.android.gms.tasks.Task;
35-
import com.google.common.collect.Lists;
3635
import com.google.common.collect.Maps;
3736
import com.google.firebase.firestore.Query.Direction;
3837
import com.google.firebase.firestore.testutil.EventAccumulator;
@@ -515,34 +514,30 @@ public void testQueriesCanUseNotEqualFilters() {
515514
expectedDocsMap.remove("i");
516515

517516
QuerySnapshot snapshot = waitFor(collection.whereNotEqualTo("zip", 98101L).get());
518-
assertArrayEquals(
519-
Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
517+
assertSetEquals(expectedDocsMap.values(), querySnapshotToValues(snapshot));
520518

521519
// With objects.
522520
expectedDocsMap = Maps.newHashMap(allDocs);
523521
expectedDocsMap.remove("f");
524522
expectedDocsMap.remove("h");
525523
expectedDocsMap.remove("i");
526524
snapshot = waitFor(collection.whereNotEqualTo("zip", map("code", 500)).get());
527-
assertArrayEquals(
528-
Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
525+
assertSetEquals(expectedDocsMap.values(), querySnapshotToValues(snapshot));
529526

530527
// With Null.
531528
expectedDocsMap = Maps.newHashMap(allDocs);
532529
expectedDocsMap.remove("h");
533530
expectedDocsMap.remove("i");
534531
snapshot = waitFor(collection.whereNotEqualTo("zip", null).get());
535-
assertArrayEquals(
536-
Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
532+
assertSetEquals(expectedDocsMap.values(), querySnapshotToValues(snapshot));
537533

538534
// With NaN.
539535
expectedDocsMap = Maps.newHashMap(allDocs);
540536
expectedDocsMap.remove("h");
541537
expectedDocsMap.remove("i");
542538
expectedDocsMap.remove("j");
543539
snapshot = waitFor(collection.whereNotEqualTo("zip", Double.NaN).get());
544-
assertArrayEquals(
545-
Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
540+
assertSetEquals(expectedDocsMap.values(), querySnapshotToValues(snapshot));
546541
}
547542

548543
// TODO(ne-queries): Re-enable once emulator support is added to CI.
@@ -652,14 +647,14 @@ public void testQueriesCanUseNotInFilters() {
652647

653648
QuerySnapshot snapshot =
654649
waitFor(collection.whereNotIn("zip", asList(98101L, 98103L, asList(98101L, 98102L))).get());
655-
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
650+
assertSetEquals(expectedDocsMap.values(), querySnapshotToValues(snapshot));
656651

657652
// With objects.
658653
expectedDocsMap = Maps.newHashMap(allDocs);
659654
expectedDocsMap.remove("f");
660655
expectedDocsMap.remove("h");
661656
snapshot = waitFor(collection.whereNotIn("zip", asList(map("code", 500L))).get());
662-
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
657+
assertSetEquals(expectedDocsMap.values(), querySnapshotToValues(snapshot));
663658

664659
// With Null.
665660
List<Object> nullArray = new ArrayList<>();
@@ -672,15 +667,15 @@ public void testQueriesCanUseNotInFilters() {
672667
expectedDocsMap.remove("h");
673668
expectedDocsMap.remove("j");
674669
snapshot = waitFor(collection.whereNotIn("zip", asList(Double.NaN)).get());
675-
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
670+
assertSetEquals(expectedDocsMap.values(), querySnapshotToValues(snapshot));
676671

677672
// With NaN and a number.
678673
expectedDocsMap = Maps.newHashMap(allDocs);
679674
expectedDocsMap.remove("a");
680675
expectedDocsMap.remove("h");
681676
expectedDocsMap.remove("j");
682677
snapshot = waitFor(collection.whereNotIn("zip", asList(Float.NaN, 98101L)).get());
683-
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
678+
assertSetEquals(expectedDocsMap.values(), querySnapshotToValues(snapshot));
684679
}
685680

686681
// TODO(ne-queries): Re-enable once emulator support is added to CI.

‎firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -523,13 +523,14 @@ private void validateOrderByFieldMatchesInequality(
523523
/**
524524
* Given an operator, returns the set of operators that cannot be used with it.
525525
*
526-
* <p>Operators in a query must adhere to the following set of rules: 1. Only one array operator
527-
* is allowed. 2. Only one disjunctive operator is allowed. 3. NOT_EQUAL cannot be used with
528-
* another NOT_EQUAL operator. 4. NOT_IN cannot be used with array, disjunctive, or NOT_EQUAL
529-
* operators.
526+
* <p>Operators in a query must adhere to the following set of rules:
527+
* <li>Only one array operator is allowed.
528+
* <li>Only one disjunctive operator is allowed.
529+
* <li>NOT_EQUAL cannot be used with another NOT_EQUAL operator.
530+
* <li>NOT_IN cannot be used with array, disjunctive, or NOT_EQUAL operators.
530531
*
531-
* <p>Array operators: ARRAY_CONTAINS, ARRAY_CONTAINS_ANY Disjunctive operators: IN,
532-
* ARRAY_CONTAINS_ANY, NOT_IN
532+
* <p>Array operators: ARRAY_CONTAINS, ARRAY_CONTAINS_ANY Disjunctive operators: IN,
533+
* ARRAY_CONTAINS_ANY, NOT_IN
533534
*/
534535
private List<Operator> conflictingOps(Operator op) {
535536
switch (op) {

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,14 +74,14 @@ public static FieldFilter create(FieldPath path, Operator operator, Value value)
7474
}
7575
} else if (Values.isNullValue(value)) {
7676
// TODO(ne-queries): Update error message to include != operator.
77-
if (operator != Filter.Operator.EQUAL && operator != Filter.Operator.NOT_EQUAL) {
77+
if (operator != Operator.EQUAL && operator != Operator.NOT_EQUAL) {
7878
throw new IllegalArgumentException(
7979
"Invalid Query. Null supports only equality comparisons (via whereEqualTo()).");
8080
}
8181
return new FieldFilter(path, operator, value);
8282
} else if (Values.isNanValue(value)) {
8383
// TODO(ne-queries): Update error message to include != operator.
84-
if (operator != Filter.Operator.EQUAL && operator != Filter.Operator.NOT_EQUAL) {
84+
if (operator != Operator.EQUAL && operator != Operator.NOT_EQUAL) {
8585
throw new IllegalArgumentException(
8686
"Invalid Query. NaN supports only equality comparisons (via whereEqualTo()).");
8787
}

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/KeyFieldInFilter.java

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,12 @@
1414

1515
package com.google.firebase.firestore.core;
1616

17-
import static com.google.firebase.firestore.util.Util.extractDocumentKeysFromArrayValue;
17+
import static com.google.firebase.firestore.util.Assert.hardAssert;
1818

1919
import com.google.firebase.firestore.model.Document;
2020
import com.google.firebase.firestore.model.DocumentKey;
2121
import com.google.firebase.firestore.model.FieldPath;
22+
import com.google.firebase.firestore.model.Values;
2223
import com.google.firestore.v1.Value;
2324
import java.util.ArrayList;
2425
import java.util.List;
@@ -36,4 +37,21 @@ public class KeyFieldInFilter extends FieldFilter {
3637
public boolean matches(Document doc) {
3738
return keys.contains(doc.getKey());
3839
}
40+
41+
static List<DocumentKey> extractDocumentKeysFromArrayValue(Operator operator, Value value) {
42+
hardAssert(
43+
operator == Operator.IN || operator == Operator.NOT_IN,
44+
"extractDocumentKeysFromArrayValue requires IN or NOT_IN operators");
45+
hardAssert(Values.isArray(value), "KeyFieldInFilter/KeyFieldNotInFilter expects an ArrayValue");
46+
List<DocumentKey> keys = new ArrayList<>();
47+
for (Value element : value.getArrayValue().getValuesList()) {
48+
hardAssert(
49+
Values.isReferenceValue(element),
50+
"Comparing on key with "
51+
+ operator.toString()
52+
+ ", but an array value was not a ReferenceValue");
53+
keys.add(DocumentKey.fromName(element.getReferenceValue()));
54+
}
55+
return keys;
56+
}
3957
}

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/KeyFieldNotInFilter.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2019 Google LLC
1+
// Copyright 2020 Google LLC
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// you may not use this file except in compliance with the License.
@@ -14,7 +14,7 @@
1414

1515
package com.google.firebase.firestore.core;
1616

17-
import static com.google.firebase.firestore.util.Util.extractDocumentKeysFromArrayValue;
17+
import static com.google.firebase.firestore.core.KeyFieldInFilter.extractDocumentKeysFromArrayValue;
1818

1919
import com.google.firebase.firestore.model.Document;
2020
import com.google.firebase.firestore.model.DocumentKey;

‎firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,6 @@
1414

1515
package com.google.firebase.firestore.util;
1616

17-
import static com.google.firebase.firestore.util.Assert.hardAssert;
18-
1917
import android.os.Handler;
2018
import android.os.Looper;
2119
import androidx.annotation.Nullable;
@@ -24,10 +22,6 @@
2422
import com.google.firebase.firestore.FieldPath;
2523
import com.google.firebase.firestore.FirebaseFirestoreException;
2624
import com.google.firebase.firestore.FirebaseFirestoreException.Code;
27-
import com.google.firebase.firestore.core.Filter.Operator;
28-
import com.google.firebase.firestore.model.DocumentKey;
29-
import com.google.firebase.firestore.model.Values;
30-
import com.google.firestore.v1.Value;
3125
import com.google.protobuf.ByteString;
3226
import io.grpc.Status;
3327
import io.grpc.StatusException;
@@ -240,22 +234,4 @@ public static int compareByteStrings(ByteString left, ByteString right) {
240234
}
241235
return Util.compareIntegers(left.size(), right.size());
242236
}
243-
244-
public static List<DocumentKey> extractDocumentKeysFromArrayValue(
245-
Operator operator, Value value) {
246-
hardAssert(
247-
operator == Operator.IN || operator == Operator.NOT_IN,
248-
"extractDocumentKeysFromArrayValue requires IN or NOT_IN operators");
249-
hardAssert(Values.isArray(value), "KeyFieldInFilter/KeyFieldNotInFilter expects an ArrayValue");
250-
List<DocumentKey> keys = new ArrayList<>();
251-
for (Value element : value.getArrayValue().getValuesList()) {
252-
hardAssert(
253-
Values.isReferenceValue(element),
254-
"Comparing on key with "
255-
+ operator.toString()
256-
+ ", but an array value was not a ReferenceValue");
257-
keys.add(DocumentKey.fromName(element.getReferenceValue()));
258-
}
259-
return keys;
260-
}
261237
}

‎firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -611,16 +611,10 @@ public static <T> void assertSetEquals(List<T> expected, Set<T> actual) {
611611
assertEquals(expectedSet, actual);
612612
}
613613

614-
/**
615-
* Asserts that the contents of actual list is equal to that of the expected one.
616-
*
617-
* @param expected A list of the expected contents of the set, in any order.
618-
* @param actual The list to compare against.
619-
* @param <T> The type of the values of in common between the expected list and actual set.
620-
*/
621-
// PORTING NOTE: JUnit and XCTest use reversed conventions on expected and actual values :-(.
622-
public static <T> void assertArrayEquals(List<T> expected, List<T> actual) {
623-
assertEquals(Sets.newHashSet(expected), Sets.newHashSet(actual));
614+
public static <T> void assertSetEquals(Iterable<T> expected, Iterable<T> actual) {
615+
Set<T> expectedSet = Sets.newHashSet(expected);
616+
Set<T> actualSet = Sets.newHashSet(actual);
617+
assertEquals(expectedSet, actualSet);
624618
}
625619

626620
/** Asserts that the given runnable block fails with an internal error. */

0 commit comments

Comments
 (0)