Skip to content

Add != and NOT_IN queries (not public) #1872

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

Merged
merged 3 commits into from
Aug 14, 2020
Merged

Add != and NOT_IN queries (not public) #1872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5ffda3
add != and NOT_IN support
Aug 10, 2020
025ef85
resolve gil comments
Aug 13, 2020
cc4397b
switch back to Lists.newArrayList() and remove assertSetEquals override
Aug 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 157 additions & 0 deletions firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@

import androidx.test.ext.junit.runners.AndroidJUnit4;
import com.google.android.gms.tasks.Task;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.firebase.firestore.Query.Direction;
import com.google.firebase.firestore.testutil.EventAccumulator;
import com.google.firebase.firestore.testutil.IntegrationTestUtil;
Expand All @@ -39,6 +41,7 @@
import java.util.Map;
import java.util.concurrent.Semaphore;
import org.junit.After;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;

Expand Down Expand Up @@ -483,6 +486,79 @@ public void testQueriesFireFromCacheWhenOffline() {
listener.remove();
}

// TODO(ne-queries): Re-enable once emulator support is added to CI.
@Ignore
@Test
public void testQueriesCanUseNotEqualFilters() {
Map<String, Object> docA = map("zip", 98101L);
Map<String, Object> docB = map("zip", 91102L);
Map<String, Object> docC = map("zip", "98101");
Map<String, Object> docD = map("zip", asList(98101L));
Map<String, Object> docE = map("zip", asList("98101", map("zip", 98101L)));
Map<String, Object> docF = map("zip", map("code", 500L));
Map<String, Object> docG = map("zip", asList(98101L, 98102L));
Map<String, Object> docH = map("code", 500L);
Map<String, Object> docI = map("zip", null);
Map<String, Object> docJ = map("zip", Double.NaN);

Map<String, Map<String, Object>> allDocs =
map(
"a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG, "h", docH,
"i", docI, "j", docJ);
CollectionReference collection = testCollectionWithDocs(allDocs);

// Search for zips not matching 98101.
Map<String, Map<String, Object>> expectedDocsMap = Maps.newHashMap(allDocs);
expectedDocsMap.remove("a");
expectedDocsMap.remove("h");
expectedDocsMap.remove("i");

QuerySnapshot snapshot = waitFor(collection.whereNotEqualTo("zip", 98101L).get());
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));

// With objects.
expectedDocsMap = Maps.newHashMap(allDocs);
expectedDocsMap.remove("f");
expectedDocsMap.remove("h");
expectedDocsMap.remove("i");
snapshot = waitFor(collection.whereNotEqualTo("zip", map("code", 500)).get());
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));

// With Null.
expectedDocsMap = Maps.newHashMap(allDocs);
expectedDocsMap.remove("h");
expectedDocsMap.remove("i");
snapshot = waitFor(collection.whereNotEqualTo("zip", null).get());
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));

// With NaN.
expectedDocsMap = Maps.newHashMap(allDocs);
expectedDocsMap.remove("h");
expectedDocsMap.remove("i");
expectedDocsMap.remove("j");
snapshot = waitFor(collection.whereNotEqualTo("zip", Double.NaN).get());
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
}

// TODO(ne-queries): Re-enable once emulator support is added to CI.
@Ignore
@Test
public void testQueriesCanUseNotEqualFiltersWithDocIds() {
Map<String, String> docA = map("key", "aa");
Map<String, String> docB = map("key", "ab");
Map<String, String> docC = map("key", "ba");
Map<String, String> docD = map("key", "bb");
Map<String, Map<String, Object>> testDocs =
map(
"aa", docA,
"ab", docB,
"ba", docC,
"bb", docD);
CollectionReference collection = testCollectionWithDocs(testDocs);
QuerySnapshot docs = waitFor(collection.whereNotEqualTo(FieldPath.documentId(), "aa").get());
assertEquals(asList(docB, docC, docD), querySnapshotToValues(docs));
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies for what's going to be an irritating question (since I didn't pick up on this earlier), but why do you assertSetEquals(x, expected) in the test above but assertEquals(asList(x), expected) here when we're testing the same underlying operator?

To me it seems like either the ordering matters or it doesn't. If ordering of query results is predictable, assertSetEquals seems wrong because it allows the result to be unordered when our customers really care that it's ordered. The reverse is also true: if ordering doesn't matter, then this test is too strict, and could create a false positive.

To my understanding, query results always have a well defined ordering (breaking ties on key ordering). This seems to point to assertSetEquals being wrong above.

What's your take on this? Why the discrepancy?

(The same concern applies to the whereNotIn, below).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching this actually! I originally ran into an issue where I was converting the snapshots into a HashSet in order to easily remove the unneeded documents, but converting them back into a List resulted in an ordering that didn't match the query snapshots from the query. Instead of trying to create the expectedDocsMap in the correct order, I took the "shortcut" of comparing them as sets to avoid the ordering errors without realizing that order matters in query results.

I took a look at the code, and realized that Using Maps.newHashMap() keeps the documents in the correct order. Switched back to using assertEquals with Lists.newArrayList().

}

@Test
public void testQueriesCanUseArrayContainsFilters() {
Map<String, Object> docA = map("array", asList(42L));
Expand Down Expand Up @@ -541,6 +617,87 @@ public void testQueriesCanUseInFiltersWithDocIds() {
assertEquals(asList(docA, docB), querySnapshotToValues(docs));
}

// TODO(ne-queries): Re-enable once emulator support is added to CI.
@Ignore
@Test
public void testQueriesCanUseNotInFilters() {
Map<String, Object> docA = map("zip", 98101L);
Map<String, Object> docB = map("zip", 91102L);
Map<String, Object> docC = map("zip", 98103L);
Map<String, Object> docD = map("zip", asList(98101L));
Map<String, Object> docE = map("zip", asList("98101", map("zip", 98101L)));
Map<String, Object> docF = map("zip", map("code", 500L));
Map<String, Object> docG = map("zip", asList(98101L, 98102L));
Map<String, Object> docH = map("code", 500L);
Map<String, Object> docI = map("zip", null);
Map<String, Object> docJ = map("zip", Double.NaN);

Map<String, Map<String, Object>> allDocs =
map(
"a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG, "h", docH,
"i", docI, "j", docJ);
CollectionReference collection = testCollectionWithDocs(allDocs);

// Search for zips not matching 98101, 98103, or [98101, 98102].
Map<String, Map<String, Object>> expectedDocsMap = Maps.newHashMap(allDocs);
expectedDocsMap.remove("a");
expectedDocsMap.remove("c");
expectedDocsMap.remove("g");
expectedDocsMap.remove("h");

QuerySnapshot snapshot =
waitFor(collection.whereNotIn("zip", asList(98101L, 98103L, asList(98101L, 98102L))).get());
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));

// With objects.
expectedDocsMap = Maps.newHashMap(allDocs);
expectedDocsMap.remove("f");
expectedDocsMap.remove("h");
snapshot = waitFor(collection.whereNotIn("zip", asList(map("code", 500L))).get());
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));

// With Null.
List<Object> nullArray = new ArrayList<>();
nullArray.add(null);
snapshot = waitFor(collection.whereNotIn("key", nullArray).get());
assertEquals(new ArrayList<>(), querySnapshotToValues(snapshot));

// With NaN.
expectedDocsMap = Maps.newHashMap(allDocs);
expectedDocsMap.remove("h");
expectedDocsMap.remove("j");
snapshot = waitFor(collection.whereNotIn("zip", asList(Double.NaN)).get());
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));

// With NaN and a number.
expectedDocsMap = Maps.newHashMap(allDocs);
expectedDocsMap.remove("a");
expectedDocsMap.remove("h");
expectedDocsMap.remove("j");
snapshot = waitFor(collection.whereNotIn("zip", asList(Float.NaN, 98101L)).get());
assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
}

// TODO(ne-queries): Re-enable once emulator support is added to CI.
@Ignore
@Test
public void testQueriesCanUseNotInFiltersWithDocIds() {
Map<String, String> docA = map("key", "aa");
Map<String, String> docB = map("key", "ab");
Map<String, String> docC = map("key", "ba");
Map<String, String> docD = map("key", "bb");
Map<String, Map<String, Object>> testDocs =
map(
"aa", docA,
"ab", docB,
"ba", docC,
"bb", docD);
CollectionReference collection = testCollectionWithDocs(testDocs);
QuerySnapshot docs =
waitFor(collection.whereNotIn(FieldPath.documentId(), asList("aa", "ab")).get());
assertEquals(asList(docC, docD), querySnapshotToValues(docs));
}

@Test
public void testQueriesCanUseArrayContainsAnyFilters() {
Map<String, Object> docA = map("array", asList(42L));
Expand Down
84 changes: 83 additions & 1 deletion firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,9 @@ public void queriesWithNullOrNaNFiltersOtherThanEqualityFail() {
expectError(
() -> collection.whereIn("a", null),
"Invalid Query. A non-empty array is required for 'in' filters.");
expectError(
() -> collection.whereNotIn("a", null),
"Invalid Query. A non-empty array is required for 'not_in' filters.");

expectError(
() -> collection.whereGreaterThan("a", Double.NaN),
Expand Down Expand Up @@ -557,6 +560,19 @@ public void queriesWithInequalityDifferentThanFirstOrderByFail() {
expectError(() -> collection.orderBy("y").whereGreaterThan("x", 32), reason);
expectError(() -> collection.whereGreaterThan("x", 32).orderBy("y").orderBy("x"), reason);
expectError(() -> collection.orderBy("y").orderBy("x").whereGreaterThan("x", 32), reason);
expectError(() -> collection.orderBy("y").orderBy("x").whereNotEqualTo("x", 32), reason);
}

@Test
public void queriesWithMultipleNotEqualAndInequalitiesFail() {
expectError(
() -> testCollection().whereNotEqualTo("x", 32).whereNotEqualTo("x", 33),
"Invalid Query. You cannot use more than one '!=' filter.");

expectError(
() -> testCollection().whereNotEqualTo("x", 32).whereGreaterThan("y", 33),
"All where filters other than whereEqualTo() must be on the same field. But you "
+ "have filters on 'x' and 'y'");
}

@Test
Expand All @@ -578,6 +594,21 @@ public void queriesWithMultipleArrayFiltersFail() {
.whereArrayContainsAny("foo", asList(1, 2))
.whereArrayContains("foo", 1),
"Invalid Query. You cannot use 'array_contains' filters with 'array_contains_any' filters.");

expectError(
() -> testCollection().whereNotIn("foo", asList(1, 2)).whereArrayContains("foo", 1),
"Invalid Query. You cannot use 'array_contains' filters with 'not_in' filters.");
}

@Test
public void queriesWithNotEqualAndNotInFiltersFail() {
expectError(
() -> testCollection().whereNotIn("foo", asList(1, 2)).whereNotEqualTo("foo", 1),
"Invalid Query. You cannot use '!=' filters with 'not_in' filters.");

expectError(
() -> testCollection().whereNotEqualTo("foo", 1).whereNotIn("foo", asList(1, 2)),
"Invalid Query. You cannot use 'not_in' filters with '!=' filters.");
}

@Test
Expand All @@ -586,6 +617,10 @@ public void queriesWithMultipleDisjunctiveFiltersFail() {
() -> testCollection().whereIn("foo", asList(1, 2)).whereIn("bar", asList(1, 2)),
"Invalid Query. You cannot use more than one 'in' filter.");

expectError(
() -> testCollection().whereNotIn("foo", asList(1, 2)).whereNotIn("bar", asList(1, 2)),
"Invalid Query. You cannot use more than one 'not_in' filter.");

expectError(
() ->
testCollection()
Expand All @@ -607,6 +642,28 @@ public void queriesWithMultipleDisjunctiveFiltersFail() {
.whereArrayContainsAny("foo", asList(1, 2)),
"Invalid Query. You cannot use 'array_contains_any' filters with 'in' filters.");

expectError(
() ->
testCollection()
.whereArrayContainsAny("foo", asList(1, 2))
.whereNotIn("bar", asList(1, 2)),
"Invalid Query. You cannot use 'not_in' filters with 'array_contains_any' filters.");

expectError(
() ->
testCollection()
.whereNotIn("bar", asList(1, 2))
.whereArrayContainsAny("foo", asList(1, 2)),
"Invalid Query. You cannot use 'array_contains_any' filters with 'not_in' filters.");

expectError(
() -> testCollection().whereNotIn("bar", asList(1, 2)).whereIn("foo", asList(1, 2)),
"Invalid Query. You cannot use 'in' filters with 'not_in' filters.");

expectError(
() -> testCollection().whereIn("bar", asList(1, 2)).whereNotIn("foo", asList(1, 2)),
"Invalid Query. You cannot use 'not_in' filters with 'in' filters.");

// This is redundant with the above tests, but makes sure our validation doesn't get confused.
expectError(
() ->
Expand All @@ -622,7 +679,23 @@ public void queriesWithMultipleDisjunctiveFiltersFail() {
.whereArrayContains("foo", 1)
.whereIn("bar", asList(1, 2))
.whereArrayContainsAny("foo", asList(1, 2)),
"Invalid Query. You cannot use 'array_contains_any' filters with 'in' filters.");
"Invalid Query. You cannot use 'array_contains_any' filters with 'array_contains' filters.");

expectError(
() ->
testCollection()
.whereNotIn("bar", asList(1, 2))
.whereArrayContains("foo", 1)
.whereArrayContainsAny("foo", asList(1, 2)),
"Invalid Query. You cannot use 'array_contains' filters with 'not_in' filters.");

expectError(
() ->
testCollection()
.whereArrayContains("foo", 1)
.whereIn("foo", asList(1, 2))
.whereNotIn("bar", asList(1, 2)),
"Invalid Query. You cannot use 'not_in' filters with 'array_contains' filters.");
}

@Test
Expand Down Expand Up @@ -653,6 +726,10 @@ public void queriesInAndArrayContainsAnyArrayRules() {
() -> testCollection().whereIn("bar", asList()),
"Invalid Query. A non-empty array is required for 'in' filters.");

expectError(
() -> testCollection().whereNotIn("bar", asList()),
"Invalid Query. A non-empty array is required for 'not_in' filters.");

expectError(
() -> testCollection().whereArrayContainsAny("bar", asList()),
"Invalid Query. A non-empty array is required for 'array_contains_any' filters.");
Expand All @@ -662,6 +739,11 @@ public void queriesInAndArrayContainsAnyArrayRules() {
() -> testCollection().whereIn("bar", asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 9, 9)),
"Invalid Query. 'in' filters support a maximum of 10 elements in the value array.");

expectError(
// The 10 element max includes duplicates.
() -> testCollection().whereNotIn("bar", asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 9, 9)),
"Invalid Query. 'not_in' filters support a maximum of 10 elements in the value array.");

expectError(
// The 10 element max includes duplicates.
() ->
Expand Down
Loading