From d9ebf3d96d1ecd8ba987c5abceabfc614454f2ae Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 12 Jun 2019 14:58:05 -0700 Subject: [PATCH 01/15] Adding proto changes --- .../src/proto/google/firestore/v1/query.proto | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/src/proto/google/firestore/v1/query.proto b/firebase-firestore/src/proto/google/firestore/v1/query.proto index dd89ef5132c..de695e6289b 100644 --- a/firebase-firestore/src/proto/google/firestore/v1/query.proto +++ b/firebase-firestore/src/proto/google/firestore/v1/query.proto @@ -1,4 +1,4 @@ -// Copyright 2018 Google LLC. +// Copyright 2019 Google LLC. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -103,6 +103,14 @@ message StructuredQuery { // Contains. Requires that the field is an array. ARRAY_CONTAINS = 7; + + // In. Requires that `value` is a non-empty ArrayValue with at most 10 + // values. + IN = 8; + + // Contains any. Requires that the field is an array and + // `value` is a non-empty ArrayValue with at most 10 values. + ARRAY_CONTAINS_ANY = 9; } // The field to filter by. From 30cb4ec2ce314724c6fd24a6d1fe63613423fc40 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 12 Jun 2019 14:58:20 -0700 Subject: [PATCH 02/15] Initial commit with first pass functionality no tests --- .../com/google/firebase/firestore/Query.java | 109 ++++++++++++++++-- .../firebase/firestore/core/Filter.java | 4 +- .../google/firebase/firestore/core/Query.java | 14 ++- .../firestore/core/RelationFilter.java | 24 +++- .../firestore/remote/RemoteSerializer.java | 8 ++ 5 files changed, 143 insertions(+), 16 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 4fd3eac0652..85cbe010d8e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -44,7 +44,10 @@ import com.google.firebase.firestore.model.value.ServerTimestampValue; import com.google.firebase.firestore.util.Executors; import com.google.firebase.firestore.util.Util; + +import java.lang.reflect.Array; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; @@ -100,6 +103,12 @@ private void validateOrderByFieldMatchesInequality( private void validateNewFilter(Filter filter) { if (filter instanceof RelationFilter) { + Operator filterOp = ((RelationFilter) filter).getOperator(); + List arrayOps = Arrays.asList(Operator.ARRAY_CONTAINS, Operator.ARRAY_CONTAINS_ANY); + List disjunctiveOps = Arrays.asList(Operator.ARRAY_CONTAINS_ANY, Operator.IN); + boolean isArrayOp = arrayOps.contains(filterOp); + boolean isDisjunctiveOp = disjunctiveOps.contains(filterOp); + RelationFilter relationFilter = (RelationFilter) filter; if (relationFilter.isInequality()) { com.google.firebase.firestore.model.FieldPath existingInequality = query.inequalityField(); @@ -117,10 +126,24 @@ private void validateNewFilter(Filter filter) { if (firstOrderByField != null) { validateOrderByFieldMatchesInequality(firstOrderByField, newInequality); } - } else if (relationFilter.getOperator() == Operator.ARRAY_CONTAINS) { - if (query.hasArrayContainsFilter()) { - throw new IllegalArgumentException( - "Invalid Query. Queries only support having a single array-contains filter."); + } else if (isDisjunctiveOp || isArrayOp) { + // You can have at most 1 disjunctive filter and 1 array filter. Check if the new filter conflicts with an existing one. + Operator conflictingOp = null; + if (isDisjunctiveOp) { + conflictingOp = this.query.findOperatorFilter(disjunctiveOps); + } + if (conflictingOp == null && isArrayOp) { + conflictingOp = this.query.findOperatorFilter(arrayOps); + } + if (conflictingOp != null) { + // We special case when it's a duplicate op to give a slightly clearer error message. + if (conflictingOp == filterOp) { + throw new IllegalArgumentException( + "Invalid Query. You cannot use more than one '" + filterOp.toString() + "' filter."); + } else { + throw new IllegalArgumentException( + "Invalid Query. You cannot use '" + filterOp.toString() + "' filters with '" + conflictingOp.toString() + "' filters."); + } } } } @@ -301,6 +324,71 @@ public Query whereArrayContains(@NonNull FieldPath fieldPath, @NonNull Object va return whereHelper(fieldPath, Operator.ARRAY_CONTAINS, value); } + /** + * Creates and returns a new Query with the additional filter that documents must contain the + * specified field, the value must be an array, and that the array must contain any values of the provided array. + * + *

A Query can have only one whereArrayContainsAny() filter. + * + * @param field The name of the field containing an array to search + * @param value The array that contains the values to match. + * @return The created Query. + */ + @NonNull + @PublicApi + public Query whereArrayContainsAny(@NonNull String field, @NonNull Object value) { + return whereHelper(FieldPath.fromDotSeparatedPath(field), Operator.ARRAY_CONTAINS_ANY, value); + } + + /** + * Creates and returns a new Query with the additional filter that documents must contain the + * specified field, the value must be an array, and that the array must contain any values of the provided + * array. + * + *

A Query can have only one whereArrayContainsAny() filter. + * + * @param fieldPath The path of the field containing an array to search + * @param value The array that contains the values to match. + * @return The created Query. + */ + @NonNull + @PublicApi + public Query whereArrayContainsAny(@NonNull FieldPath fieldPath, @NonNull List value) { + return whereHelper(fieldPath, Operator.ARRAY_CONTAINS_ANY, value); + } + + /** + * Creates and returns a new Query with the additional filter that documents must contain the + * specified field and that the field must equal any values of the provided array. + * + *

A Query can have only one whereIn() filter. + * + * @param field The name of the field containing an array to search + * @param value The array that contains the values to match. + * @return The created Query. + */ + @NonNull + @PublicApi + public Query whereIn(@NonNull String field, @NonNull Object value) { + return whereHelper(FieldPath.fromDotSeparatedPath(field), Operator.IN, value); + } + + /** + * Creates and returns a new Query with the additional filter that documents must contain the + * specified field and that the field must equal any values of the provided array. + * + *

A Query can have only one whereIn() filter. + * + * @param fieldPath The path of the field containing an array to search + * @param value The array that contains the values to match. + * @return The created Query. + */ + @NonNull + @PublicApi + public Query whereIn(@NonNull FieldPath fieldPath, @NonNull List value) { + return whereHelper(fieldPath, Operator.IN, value); + } + /** * Creates and returns a new Query with the additional filter that documents must contain the * specified field and the value should satisfy the relation constraint provided. @@ -316,10 +404,9 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu FieldValue fieldValue; com.google.firebase.firestore.model.FieldPath internalPath = fieldPath.getInternalPath(); if (internalPath.isKeyField()) { - if (op == Operator.ARRAY_CONTAINS) { + if (op == Operator.ARRAY_CONTAINS || op == Operator.ARRAY_CONTAINS_ANY || op == Operator.IN) { throw new IllegalArgumentException( - "Invalid query. You can't perform array-contains queries on FieldPath.documentId() " - + "since document IDs are not arrays."); + "Invalid query. You can't perform '" + op.toString() + "' queries on FieldPath.documentId()."); } if (value instanceof String) { String documentKey = (String) value; @@ -357,6 +444,14 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu + Util.typeName(value)); } } else { + if (op == Operator.IN || op == Operator.ARRAY_CONTAINS_ANY) { + if (!(value instanceof List) || Array.getLength(value) == 0) { + throw new IllegalArgumentException("Invalid Query. A non-empty array is required for '" + op.toString() + "' filters."); + } + if (Array.getLength(value) > 10) { + throw new IllegalArgumentException("Invalid Query. '" + op.toString() + "' filters support a maximum of 10 elements in the value array."); + } + } fieldValue = firestore.getDataConverter().parseQueryValue(value); } Filter filter = Filter.create(fieldPath.getInternalPath(), op, fieldValue); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java index 1687fc1ee4f..353ede8ed6d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java @@ -28,7 +28,9 @@ public enum Operator { EQUAL("=="), GREATER_THAN(">"), GREATER_THAN_OR_EQUAL(">="), - ARRAY_CONTAINS("array_contains"); + ARRAY_CONTAINS("array_contains"), + ARRAY_CONTAINS_ANY("array_contains_any"), + IN("in"); private final String text; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 9dac58d9c9a..1fffce0f8a3 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.core; +import android.graphics.Path; + import static com.google.firebase.firestore.util.Assert.hardAssert; import com.google.firebase.firestore.core.Filter.Operator; @@ -167,16 +169,18 @@ public FieldPath inequalityField() { return null; } - public boolean hasArrayContainsFilter() { + @Nullable + /** Checks if any of the provided RelationOps are included in the query and returns the first one that is, or null if none are. */ + public Operator findOperatorFilter(List filterOps) { for (Filter filter : filters) { if (filter instanceof RelationFilter) { - RelationFilter relationFilter = (RelationFilter) filter; - if (relationFilter.getOperator() == Operator.ARRAY_CONTAINS) { - return true; + Operator queryOp = ((RelationFilter) filter).getOperator(); + if (filterOps.contains(queryOp)) { + return queryOp; } } } - return false; + return null; } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java index 4a68b432220..855c2b795de 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java @@ -14,6 +14,9 @@ package com.google.firebase.firestore.core; +import android.graphics.Path; + +import static com.google.firebase.firestore.util.Assert.fail; import static com.google.firebase.firestore.util.Assert.hardAssert; import com.google.firebase.firestore.model.Document; @@ -23,6 +26,10 @@ import com.google.firebase.firestore.model.value.FieldValue; import com.google.firebase.firestore.util.Assert; +import java.lang.reflect.Array; +import java.util.Arrays; +import java.util.List; + /** Represents a filter to be applied to query. */ public class RelationFilter extends Filter { private final Operator operator; @@ -61,8 +68,8 @@ public boolean matches(Document doc) { hardAssert( refValue instanceof DocumentKey, "Comparing on key, but filter value not a DocumentKey"); hardAssert( - operator != Operator.ARRAY_CONTAINS, - "ARRAY_CONTAINS queries don't make sense on document keys."); + operator != Operator.ARRAY_CONTAINS && operator != Operator.ARRAY_CONTAINS_ANY && operator != Operator.IN, + "'" + operator.toString() + "' queries don't make sense on document keys."); int comparison = DocumentKey.comparator().compare(doc.getKey(), (DocumentKey) refValue); return matchesComparison(comparison); } else { @@ -74,6 +81,17 @@ public boolean matches(Document doc) { private boolean matchesValue(FieldValue other) { if (operator == Operator.ARRAY_CONTAINS) { return other instanceof ArrayValue && ((ArrayValue) other).getInternalValue().contains(value); + } else if (operator == Operator.IN) { + return value instanceof ArrayValue && ((ArrayValue) value).getInternalValue().contains(other); + } else if (operator == Operator.ARRAY_CONTAINS_ANY) { + if (other instanceof ArrayValue && value instanceof ArrayValue) { + for (FieldValue val : ((ArrayValue) other).getInternalValue()) { + if (((ArrayValue) value).getInternalValue().contains(val)) { + return true; + } + } + } + return false; } else { // Only compare types with matching backend order (such as double and int). return value.typeOrder() == other.typeOrder() @@ -99,7 +117,7 @@ private boolean matchesComparison(int comp) { } public boolean isInequality() { - return operator != Operator.EQUAL && operator != Operator.ARRAY_CONTAINS; + return Arrays.asList(Operator.LESS_THAN, Operator.LESS_THAN_OR_EQUAL, Operator.GREATER_THAN, Operator.GREATER_THAN_OR_EQUAL).contains(operator); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java index 8114491be96..fa9e213b5e9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java @@ -927,6 +927,10 @@ private FieldFilter.Operator encodeRelationFilterOperator(RelationFilter.Operato return FieldFilter.Operator.GREATER_THAN_OR_EQUAL; case ARRAY_CONTAINS: return FieldFilter.Operator.ARRAY_CONTAINS; + case IN: + return FieldFilter.Operator.IN; + case ARRAY_CONTAINS_ANY: + return FieldFilter.Operator.ARRAY_CONTAINS_ANY; default: throw fail("Unknown operator %d", operator); } @@ -946,6 +950,10 @@ private RelationFilter.Operator decodeRelationFilterOperator(FieldFilter.Operato return RelationFilter.Operator.GREATER_THAN; case ARRAY_CONTAINS: return RelationFilter.Operator.ARRAY_CONTAINS; + case IN: + return RelationFilter.Operator.IN; + case ARRAY_CONTAINS_ANY: + return RelationFilter.Operator.ARRAY_CONTAINS_ANY; default: throw fail("Unhandled FieldFilter.operator %d", operator); } From 7fc4e814fe001a615497b179cc610a326cff61cb Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 12 Jun 2019 16:40:39 -0700 Subject: [PATCH 03/15] adding validation tests --- .../firebase/firestore/ValidationTest.java | 41 +++++++++++++++++-- .../com/google/firebase/firestore/Query.java | 4 +- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java index 1990f7ccf06..15a96ed1eb3 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java @@ -38,6 +38,7 @@ import com.google.firebase.firestore.Transaction.Function; import com.google.firebase.firestore.testutil.IntegrationTestUtil; import com.google.firebase.firestore.util.Consumer; + import java.util.Date; import java.util.List; import java.util.Map; @@ -408,6 +409,12 @@ public void queriesWithNullOrNaNFiltersOtherThanEqualityFail() { expectError( () -> collection.whereArrayContains("a", null), "Invalid Query. You can only perform equality comparisons on null (via whereEqualTo())."); + expectError( + () -> collection.whereArrayContainsAny("a", null), + "Invalid Query. A non-empty array is required for 'array_contains_any' filters."); + expectError( + () -> collection.whereIn("a", null), + "Invalid Query. A non-empty array is required for 'in' filters."); expectError( () -> collection.whereGreaterThan("a", Double.NaN), @@ -539,10 +546,37 @@ public void queriesWithInequalityDifferentThanFirstOrderByFail() { } @Test - public void queriesWithMultipleArrayContainsFiltersFail() { + public void queriesWithMultipleArrayFiltersFail() { expectError( () -> testCollection().whereArrayContains("foo", 1).whereArrayContains("foo", 2), - "Invalid Query. Queries only support having a single array-contains filter."); + "Invalid Query. You cannot use more than one 'array_contains' filter."); + + expectError( + () -> testCollection().whereArrayContains("foo", 1).whereArrayContainsAny("foo", asList(1,2)), + "Invalid Query. You cannot use 'array_contains_any' filters with 'array_contains' filters."); + + expectError( + () -> testCollection().whereArrayContainsAny("foo", asList(1,2)).whereArrayContains("foo", 1), + "Invalid Query. You cannot use 'array_contains' filters with 'array_contains_any' filters."); + } + + @Test + public void queriesWithMultipleDisjunctiveFiltersFail() { + expectError( + () -> testCollection().whereIn("foo", asList(1,2)).whereIn("bar", asList(1,2)), + "Invalid Query. You cannot use more than one 'in' filter."); + + expectError( + () -> testCollection().whereArrayContainsAny("foo", asList(1,2)).whereArrayContainsAny("bar", asList(1,2)), + "Invalid Query. You cannot use more than one 'array_contains_any' filter."); + + expectError( + () -> testCollection().whereArrayContainsAny("foo", asList(1,2)).whereIn("bar", asList(1,2)), + "Invalid Query. You cannot use 'in' filters with 'array_contains_any' filters."); + + expectError( + () -> testCollection().whereIn("bar", asList(1,2)).whereArrayContainsAny("foo", asList(1,2)), + "Invalid Query. You cannot use 'array_contains_any' filters with 'in' filters."); } @Test @@ -592,8 +626,7 @@ public void queriesFilteredByDocumentIDMustUseStringsOrDocumentReferences() { reason); reason = - "Invalid query. You can't perform array-contains queries on FieldPath.documentId() since " - + "document IDs are not arrays."; + "Invalid query. You can't perform 'array_contains' queries on FieldPath.documentId()."; expectError(() -> collection.whereArrayContains(FieldPath.documentId(), 1), reason); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 85cbe010d8e..69c94336712 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -336,7 +336,7 @@ public Query whereArrayContains(@NonNull FieldPath fieldPath, @NonNull Object va */ @NonNull @PublicApi - public Query whereArrayContainsAny(@NonNull String field, @NonNull Object value) { + public Query whereArrayContainsAny(@NonNull String field, @NonNull List value) { return whereHelper(FieldPath.fromDotSeparatedPath(field), Operator.ARRAY_CONTAINS_ANY, value); } @@ -369,7 +369,7 @@ public Query whereArrayContainsAny(@NonNull FieldPath fieldPath, @NonNull List value) { return whereHelper(FieldPath.fromDotSeparatedPath(field), Operator.IN, value); } From 69997cc1822400ccc0c12e17144edbd3bfe3fa25 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 12 Jun 2019 17:43:18 -0700 Subject: [PATCH 04/15] finish validationtest and querytest --- .../firebase/firestore/ValidationTest.java | 120 +++++++++++++++--- .../com/google/firebase/firestore/Query.java | 38 ++++-- .../google/firebase/firestore/core/Query.java | 7 +- .../firestore/core/RelationFilter.java | 17 +-- .../firebase/firestore/core/QueryTest.java | 74 +++++++++++ .../firebase/firestore/testutil/TestUtil.java | 4 + 6 files changed, 217 insertions(+), 43 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java index 15a96ed1eb3..a88a282c849 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java @@ -38,7 +38,6 @@ import com.google.firebase.firestore.Transaction.Function; import com.google.firebase.firestore.testutil.IntegrationTestUtil; import com.google.firebase.firestore.util.Consumer; - import java.util.Date; import java.util.List; import java.util.Map; @@ -410,11 +409,11 @@ public void queriesWithNullOrNaNFiltersOtherThanEqualityFail() { () -> collection.whereArrayContains("a", null), "Invalid Query. You can only perform equality comparisons on null (via whereEqualTo())."); expectError( - () -> collection.whereArrayContainsAny("a", null), - "Invalid Query. A non-empty array is required for 'array_contains_any' filters."); + () -> collection.whereArrayContainsAny("a", null), + "Invalid Query. A non-empty array is required for 'array_contains_any' filters."); expectError( - () -> collection.whereIn("a", null), - "Invalid Query. A non-empty array is required for 'in' filters."); + () -> collection.whereIn("a", null), + "Invalid Query. A non-empty array is required for 'in' filters."); expectError( () -> collection.whereGreaterThan("a", Double.NaN), @@ -552,31 +551,107 @@ public void queriesWithMultipleArrayFiltersFail() { "Invalid Query. You cannot use more than one 'array_contains' filter."); expectError( - () -> testCollection().whereArrayContains("foo", 1).whereArrayContainsAny("foo", asList(1,2)), - "Invalid Query. You cannot use 'array_contains_any' filters with 'array_contains' filters."); + () -> + testCollection() + .whereArrayContains("foo", 1) + .whereArrayContainsAny("foo", asList(1, 2)), + "Invalid Query. You cannot use 'array_contains_any' filters with 'array_contains' filters."); expectError( - () -> testCollection().whereArrayContainsAny("foo", asList(1,2)).whereArrayContains("foo", 1), - "Invalid Query. You cannot use 'array_contains' filters with 'array_contains_any' filters."); + () -> + testCollection() + .whereArrayContainsAny("foo", asList(1, 2)) + .whereArrayContains("foo", 1), + "Invalid Query. You cannot use 'array_contains' filters with 'array_contains_any' filters."); } @Test public void queriesWithMultipleDisjunctiveFiltersFail() { expectError( - () -> testCollection().whereIn("foo", asList(1,2)).whereIn("bar", asList(1,2)), - "Invalid Query. You cannot use more than one 'in' filter."); + () -> testCollection().whereIn("foo", asList(1, 2)).whereIn("bar", asList(1, 2)), + "Invalid Query. You cannot use more than one 'in' filter."); + + expectError( + () -> + testCollection() + .whereArrayContainsAny("foo", asList(1, 2)) + .whereArrayContainsAny("bar", asList(1, 2)), + "Invalid Query. You cannot use more than one 'array_contains_any' filter."); + + expectError( + () -> + testCollection() + .whereArrayContainsAny("foo", asList(1, 2)) + .whereIn("bar", asList(1, 2)), + "Invalid Query. You cannot use 'in' filters with 'array_contains_any' filters."); + + expectError( + () -> + testCollection() + .whereIn("bar", asList(1, 2)) + .whereArrayContainsAny("foo", asList(1, 2)), + "Invalid Query. You cannot use 'array_contains_any' filters with 'in' filters."); + + // This is redundant with the above tests, but makes sure our validation doesn't get confused. + expectError( + () -> + testCollection() + .whereIn("bar", asList(1, 2)) + .whereArrayContains("foo", 1) + .whereArrayContainsAny("foo", asList(1, 2)), + "Invalid Query. You cannot use 'array_contains_any' filters with 'in' filters."); + + expectError( + () -> + testCollection() + .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."); + } + + @Test + public void queriesCanUseInWithArrayContains() { + testCollection().whereArrayContains("foo", 1).whereIn("bar", asList(1, 2)); + testCollection().whereIn("bar", asList(1, 2)).whereArrayContains("foo", 1); + + expectError( + () -> + testCollection() + .whereIn("bar", asList(1, 2)) + .whereArrayContains("foo", 1) + .whereArrayContains("foo", 1), + "Invalid Query. You cannot use more than one 'array_contains' filter."); expectError( - () -> testCollection().whereArrayContainsAny("foo", asList(1,2)).whereArrayContainsAny("bar", asList(1,2)), - "Invalid Query. You cannot use more than one 'array_contains_any' filter."); + () -> + testCollection() + .whereArrayContains("foo", 1) + .whereIn("bar", asList(1, 2)) + .whereIn("bar", asList(1, 2)), + "Invalid Query. You cannot use more than one 'in' filter."); + } + @Test + public void queriesInAndArrayContainsAnyArrayRules() { expectError( - () -> testCollection().whereArrayContainsAny("foo", asList(1,2)).whereIn("bar", asList(1,2)), - "Invalid Query. You cannot use 'in' filters with 'array_contains_any' filters."); + () -> testCollection().whereIn("bar", asList()), + "Invalid Query. A non-empty array is required for 'in' filters."); expectError( - () -> testCollection().whereIn("bar", asList(1,2)).whereArrayContainsAny("foo", asList(1,2)), - "Invalid Query. You cannot use 'array_contains_any' filters with 'in' filters."); + () -> testCollection().whereArrayContainsAny("bar", asList()), + "Invalid Query. A non-empty array is required for 'array_contains_any' filters."); + + expectError( + // The 10 element max includes duplicates. + () -> 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().whereArrayContainsAny("bar", asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 9, 9)), + "Invalid Query. 'array_contains_any' filters support a maximum of 10 elements in the value array."); } @Test @@ -625,9 +700,16 @@ public void queriesFilteredByDocumentIDMustUseStringsOrDocumentReferences() { .whereGreaterThanOrEqualTo(FieldPath.documentId(), "foo"), reason); - reason = - "Invalid query. You can't perform 'array_contains' queries on FieldPath.documentId()."; + reason = "Invalid query. You can't perform 'array_contains' queries on FieldPath.documentId()."; expectError(() -> collection.whereArrayContains(FieldPath.documentId(), 1), reason); + + reason = + "Invalid query. You can't perform 'array_contains_any' queries on FieldPath.documentId()."; + expectError( + () -> collection.whereArrayContainsAny(FieldPath.documentId(), asList(1, 2)), reason); + + reason = "Invalid query. You can't perform 'in' queries on FieldPath.documentId()."; + expectError(() -> collection.whereIn(FieldPath.documentId(), asList(1, 2)), reason); } // Helpers diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 69c94336712..c53c2d4acbd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -44,8 +44,6 @@ import com.google.firebase.firestore.model.value.ServerTimestampValue; import com.google.firebase.firestore.util.Executors; import com.google.firebase.firestore.util.Util; - -import java.lang.reflect.Array; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -127,7 +125,8 @@ private void validateNewFilter(Filter filter) { validateOrderByFieldMatchesInequality(firstOrderByField, newInequality); } } else if (isDisjunctiveOp || isArrayOp) { - // You can have at most 1 disjunctive filter and 1 array filter. Check if the new filter conflicts with an existing one. + // You can have at most 1 disjunctive filter and 1 array filter. Check if the new filter + // conflicts with an existing one. Operator conflictingOp = null; if (isDisjunctiveOp) { conflictingOp = this.query.findOperatorFilter(disjunctiveOps); @@ -139,10 +138,16 @@ private void validateNewFilter(Filter filter) { // We special case when it's a duplicate op to give a slightly clearer error message. if (conflictingOp == filterOp) { throw new IllegalArgumentException( - "Invalid Query. You cannot use more than one '" + filterOp.toString() + "' filter."); + "Invalid Query. You cannot use more than one '" + + filterOp.toString() + + "' filter."); } else { throw new IllegalArgumentException( - "Invalid Query. You cannot use '" + filterOp.toString() + "' filters with '" + conflictingOp.toString() + "' filters."); + "Invalid Query. You cannot use '" + + filterOp.toString() + + "' filters with '" + + conflictingOp.toString() + + "' filters."); } } } @@ -326,7 +331,8 @@ public Query whereArrayContains(@NonNull FieldPath fieldPath, @NonNull Object va /** * Creates and returns a new Query with the additional filter that documents must contain the - * specified field, the value must be an array, and that the array must contain any values of the provided array. + * specified field, the value must be an array, and that the array must contain any values of the + * provided array. * *

A Query can have only one whereArrayContainsAny() filter. * @@ -342,8 +348,8 @@ public Query whereArrayContainsAny(@NonNull String field, @NonNull List /** * Creates and returns a new Query with the additional filter that documents must contain the - * specified field, the value must be an array, and that the array must contain any values of the provided - * array. + * specified field, the value must be an array, and that the array must contain any values of the + * provided array. * *

A Query can have only one whereArrayContainsAny() filter. * @@ -406,7 +412,9 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu if (internalPath.isKeyField()) { if (op == Operator.ARRAY_CONTAINS || op == Operator.ARRAY_CONTAINS_ANY || op == Operator.IN) { throw new IllegalArgumentException( - "Invalid query. You can't perform '" + op.toString() + "' queries on FieldPath.documentId()."); + "Invalid query. You can't perform '" + + op.toString() + + "' queries on FieldPath.documentId()."); } if (value instanceof String) { String documentKey = (String) value; @@ -445,11 +453,15 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu } } else { if (op == Operator.IN || op == Operator.ARRAY_CONTAINS_ANY) { - if (!(value instanceof List) || Array.getLength(value) == 0) { - throw new IllegalArgumentException("Invalid Query. A non-empty array is required for '" + op.toString() + "' filters."); + if (!(value instanceof List) || ((List) value).size() == 0) { + throw new IllegalArgumentException( + "Invalid Query. A non-empty array is required for '" + op.toString() + "' filters."); } - if (Array.getLength(value) > 10) { - throw new IllegalArgumentException("Invalid Query. '" + op.toString() + "' filters support a maximum of 10 elements in the value array."); + if (((List) value).size() > 10) { + throw new IllegalArgumentException( + "Invalid Query. '" + + op.toString() + + "' filters support a maximum of 10 elements in the value array."); } } fieldValue = firestore.getDataConverter().parseQueryValue(value); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 1fffce0f8a3..8b5243972e1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -14,8 +14,6 @@ package com.google.firebase.firestore.core; -import android.graphics.Path; - import static com.google.firebase.firestore.util.Assert.hardAssert; import com.google.firebase.firestore.core.Filter.Operator; @@ -170,7 +168,10 @@ public FieldPath inequalityField() { } @Nullable - /** Checks if any of the provided RelationOps are included in the query and returns the first one that is, or null if none are. */ + /** + * Checks if any of the provided RelationOps are included in the query and returns the first one + * that is, or null if none are. + */ public Operator findOperatorFilter(List filterOps) { for (Filter filter : filters) { if (filter instanceof RelationFilter) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java index 855c2b795de..168290946ed 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java @@ -14,9 +14,6 @@ package com.google.firebase.firestore.core; -import android.graphics.Path; - -import static com.google.firebase.firestore.util.Assert.fail; import static com.google.firebase.firestore.util.Assert.hardAssert; import com.google.firebase.firestore.model.Document; @@ -25,10 +22,7 @@ import com.google.firebase.firestore.model.value.ArrayValue; import com.google.firebase.firestore.model.value.FieldValue; import com.google.firebase.firestore.util.Assert; - -import java.lang.reflect.Array; import java.util.Arrays; -import java.util.List; /** Represents a filter to be applied to query. */ public class RelationFilter extends Filter { @@ -68,7 +62,9 @@ public boolean matches(Document doc) { hardAssert( refValue instanceof DocumentKey, "Comparing on key, but filter value not a DocumentKey"); hardAssert( - operator != Operator.ARRAY_CONTAINS && operator != Operator.ARRAY_CONTAINS_ANY && operator != Operator.IN, + operator != Operator.ARRAY_CONTAINS + && operator != Operator.ARRAY_CONTAINS_ANY + && operator != Operator.IN, "'" + operator.toString() + "' queries don't make sense on document keys."); int comparison = DocumentKey.comparator().compare(doc.getKey(), (DocumentKey) refValue); return matchesComparison(comparison); @@ -117,7 +113,12 @@ private boolean matchesComparison(int comp) { } public boolean isInequality() { - return Arrays.asList(Operator.LESS_THAN, Operator.LESS_THAN_OR_EQUAL, Operator.GREATER_THAN, Operator.GREATER_THAN_OR_EQUAL).contains(operator); + return Arrays.asList( + Operator.LESS_THAN, + Operator.LESS_THAN_OR_EQUAL, + Operator.GREATER_THAN, + Operator.GREATER_THAN_OR_EQUAL) + .contains(operator); } @Override diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java index acc22a4846c..e72ce1c12cc 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java @@ -159,6 +159,80 @@ public void testArrayContainsFiltersWithObjectValues() { assertTrue(query.matches(document)); } + @Test + public void testInFilters() { + Query query = + Query.atPath(ResourcePath.fromString("collection")) + .filter(filter("zip", "in", asList(12345))); + + Document document = doc("collection/1", 0, map("zip", 12345)); + assertTrue(query.matches(document)); + + // Value matches in array. + document = doc("collection/1", 0, map("zip", asList(12345))); + assertFalse(query.matches(document)); + + // Non-type match. + document = doc("collection/1", 0, map("zip", "12345")); + assertFalse(query.matches(document)); + + // Nested match. + document = doc("collection/1", 0, map("zip", asList("12345", map("zip", 12345)))); + assertFalse(query.matches(document)); + } + + @Test + public void testInFiltersWithObjectValues() { + Query query = + Query.atPath(ResourcePath.fromString("collection")) + .filter(filter("zip", "in", asList(map("a", asList(42))))); + + // Containing object in array. + Document document = doc("collection/1", 0, map("zip", asList(map("a", asList(42))))); + assertFalse(query.matches(document)); + + // Containing object. + document = doc("collection/1", 0, map("zip", map("a", asList(42)))); + assertTrue(query.matches(document)); + } + + @Test + public void testArrayContainsAnyFilters() { + Query query = + Query.atPath(ResourcePath.fromString("collection")) + .filter(filter("zip", "array-contains-any", asList(12345))); + + Document document = doc("collection/1", 0, map("zip", asList(12345))); + assertTrue(query.matches(document)); + + // Value matches in non-array. + document = doc("collection/1", 0, map("zip", 12345)); + assertFalse(query.matches(document)); + + // Non-type match. + document = doc("collection/1", 0, map("zip", asList("12345"))); + assertFalse(query.matches(document)); + + // Nested match. + document = doc("collection/1", 0, map("zip", asList("12345", map("zip", asList(12345))))); + assertFalse(query.matches(document)); + } + + @Test + public void testArrayContainsAnyFiltersWithObjectValues() { + Query query = + Query.atPath(ResourcePath.fromString("collection")) + .filter(filter("zip", "array-contains-any", asList(map("a", asList(42))))); + + // Containing object in array. + Document document = doc("collection/1", 0, map("zip", asList(map("a", asList(42))))); + assertTrue(query.matches(document)); + + // Containing object. + document = doc("collection/1", 0, map("zip", map("a", asList(42)))); + assertFalse(query.matches(document)); + } + @Test public void testNaNFilter() { Query query = diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index 2960fb2699e..a7205249dc3 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -237,6 +237,10 @@ public static Operator operatorFromString(String s) { return Operator.GREATER_THAN_OR_EQUAL; } else if (s.equals("array-contains")) { return Operator.ARRAY_CONTAINS; + } else if (s.equals("in")) { + return Operator.IN; + } else if (s.equals("array-contains-any")) { + return Operator.ARRAY_CONTAINS_ANY; } else { throw new IllegalStateException("Unknown operator: " + s); } From 255bb002d3ad486f50be0eee874dc2301f25eb23 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 12 Jun 2019 17:58:08 -0700 Subject: [PATCH 05/15] Added database test --- .../firebase/firestore/FirestoreTest.java | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 6d089255ac0..58389b70483 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -509,26 +509,35 @@ public void testQueriesAreValidatedOnClient() { // NOTE: Failure cases are validated in ValidationTest. CollectionReference collection = testCollection(); final Query query = collection.whereGreaterThanOrEqualTo("x", 32); - // Same inequality field works; + // Same inequality field works. query.whereLessThanOrEqualTo("x", "cat"); - // Equality on different field works; + // Equality on different field works. query.whereEqualTo("y", "cat"); - // Array contains on different field works; + // Array contains on different field works. query.whereArrayContains("y", "cat"); + // Array contains any on different field works. + query.whereArrayContainsAny("y", Arrays.asList("cat")); + // In on different field works. + query.whereIn("y", Arrays.asList("cat")); // Ordering by inequality field succeeds. query.orderBy("x"); collection.orderBy("x").whereGreaterThanOrEqualTo("x", 32); - // inequality same as first order by works + // Inequality same as first order by works. query.orderBy("x").orderBy("y"); collection.orderBy("x").orderBy("y").whereGreaterThanOrEqualTo("x", 32); collection.orderBy("x", Direction.DESCENDING).whereEqualTo("y", "true"); - // Equality different than orderBy works + // Equality different than orderBy works. collection.orderBy("x").whereEqualTo("y", "cat"); - // Array contains different than orderBy works + // Array contains different than orderBy works. collection.orderBy("x").whereArrayContains("y", "cat"); + // Array contains any different than orderBy works. + collection.orderBy("x").whereArrayContainsAny("y", Arrays.asList("cat")); + // In different than orderBy works. + collection.orderBy("x").whereIn("y", Arrays.asList("cat")); + } @Test From 1f20c7fc5e0080040f7aced58280f557b64ba867 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 13 Jun 2019 11:48:36 -0700 Subject: [PATCH 06/15] add query integration tests and serializer test --- .../firebase/firestore/FirestoreTest.java | 1 - .../google/firebase/firestore/QueryTest.java | 47 +++++++++- .../remote/RemoteSerializerTest.java | 91 +++++++++++++++++-- 3 files changed, 131 insertions(+), 8 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 58389b70483..83655041b6d 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -537,7 +537,6 @@ public void testQueriesAreValidatedOnClient() { collection.orderBy("x").whereArrayContainsAny("y", Arrays.asList("cat")); // In different than orderBy works. collection.orderBy("x").whereIn("y", Arrays.asList("cat")); - } @Test diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 53f8c113b91..95af064cb33 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -420,13 +420,58 @@ public void testQueriesCanUseArrayContainsFilters() { testCollectionWithDocs(map("a", docA, "b", docB, "c", docC, "d", docD)); // Search for "array" to contain 42 - QuerySnapshot snapshot = waitFor(collection.whereArrayContains("array", 42L).get()); + QuerySnapshot snapshot = waitFor(collection.whereArrayContains("arrays", 42L).get()); assertEquals(asList(docA, docB, docD), querySnapshotToValues(snapshot)); // NOTE: The backend doesn't currently support null, NaN, objects, or arrays, so there isn't // much of anything else interesting to test. } + @Test + public void testQueriesCanUseInFilters() { + Map docA = map("zip", 98101); + Map docB = map("zip", 91102); + Map docC = map("zip", 98103); + Map docD = map("zip", asList(98101)); + Map docE = map("zip", asList("98101", map("zip", 98101))); + Map docF = map("zip", map("code", 500)); + CollectionReference collection = + testCollectionWithDocs( + map("a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF)); + + // Search for zips matching [98101, 98103]. + QuerySnapshot snapshot = waitFor(collection.whereIn("zip", asList(98101, 98103)).get()); + assertEquals(asList(docA, docC), querySnapshotToValues(snapshot)); + + // With objects. + snapshot = waitFor(collection.whereIn("zip", asList(map("code", 500))).get()); + assertEquals(asList(docF), querySnapshotToValues(snapshot)); + } + + @Test + public void testQueriesCanUseArrayContainsAnyFilters() { + Map docA = map("array", asList(42L)); + Map docB = map("array", asList("a", 42L, "c")); + Map docC = map("array", asList(41.999, "42", map("a", asList(42)))); + Map docD = map("array", asList(42L), "array2", asList("bingo")); + Map docE = map("array", asList(43L)); + Map docF = map("array", asList(map("a", 42L))); + Map docG = map("array", 42L); + + CollectionReference collection = + testCollectionWithDocs( + map("a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF)); + + // Search for "array" to contain [42, 43]. + QuerySnapshot snapshot = + waitFor(collection.whereArrayContainsAny("array", asList(42L, 43L)).get()); + assertEquals(asList(docA, docB, docD, docE), querySnapshotToValues(snapshot)); + + // With objects. + snapshot = waitFor(collection.whereArrayContainsAny("array", asList(map("a", 42L))).get()); + assertEquals(asList(docF), querySnapshotToValues(snapshot)); + } + @Test public void testCollectionGroupQueries() { FirebaseFirestore db = testFirestore(); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java index e2c1b1b7dd3..2741c0838ae 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java @@ -577,8 +577,13 @@ public void testEncodesMultipleFiltersOnDeeperCollections() { Query.atPath(ResourcePath.fromString("rooms/1/messages/10/attachments")) .filter(filter("prop", "<", 42)) .filter(filter("author", "==", "dimond")) - .filter(filter("tags", "array-contains", "pending")); + .filter(filter("tags", "array-contains", "pending")) + .filter(filter("tags", "in", asList("pending", "dimond"))); Target actual = serializer.encodeTarget(wrapQueryData(q)); + ArrayValue.Builder array = + ArrayValue.newBuilder() + .addValues(valueBuilder().setStringValue("pending")) + .addValues(valueBuilder().setStringValue("dimond")); StructuredQuery.Builder structuredQueryBuilder = StructuredQuery.newBuilder() @@ -611,7 +616,80 @@ public void testEncodesMultipleFiltersOnDeeperCollections() { .setField( FieldReference.newBuilder().setFieldPath("tags")) .setOp(Operator.ARRAY_CONTAINS) - .setValue(valueBuilder().setStringValue("pending")))))) + .setValue(valueBuilder().setStringValue("pending")))) + .addFilters( + Filter.newBuilder() + .setFieldFilter( + FieldFilter.newBuilder() + .setField( + FieldReference.newBuilder().setFieldPath("tags")) + .setOp(Operator.IN) + .setValue(valueBuilder().setArrayValue(array)))))) + .addOrderBy( + Order.newBuilder() + .setField(FieldReference.newBuilder().setFieldPath("prop")) + .setDirection(Direction.ASCENDING)) + .addOrderBy(defaultKeyOrder()); + QueryTarget.Builder queryBuilder = + QueryTarget.newBuilder() + .setParent("projects/p/databases/d/documents/rooms/1/messages/10") + .setStructuredQuery(structuredQueryBuilder); + Target expected = + Target.newBuilder() + .setQuery(queryBuilder) + .setTargetId(1) + .setResumeToken(ByteString.EMPTY) + .build(); + + assertEquals(expected, actual); + assertEquals(serializer.decodeQueryTarget(serializer.encodeQueryTarget(q)), q); + } + + @Test + public void testEncodesDisjunctiveFiltersOnDeeperCollections() { + Query q = + Query.atPath(ResourcePath.fromString("rooms/1/messages/10/attachments")) + .filter(filter("prop", "<", 42)) + .filter(filter("author", "==", "dimond")) + .filter(filter("tags", "array-contains-any", asList("pending", "dimond"))); + Target actual = serializer.encodeTarget(wrapQueryData(q)); + ArrayValue.Builder array = + ArrayValue.newBuilder() + .addValues(valueBuilder().setStringValue("pending")) + .addValues(valueBuilder().setStringValue("dimond")); + + StructuredQuery.Builder structuredQueryBuilder = + StructuredQuery.newBuilder() + .addFrom(CollectionSelector.newBuilder().setCollectionId("attachments")) + .setWhere( + Filter.newBuilder() + .setCompositeFilter( + StructuredQuery.CompositeFilter.newBuilder() + .setOp(CompositeFilter.Operator.AND) + .addFilters( + Filter.newBuilder() + .setFieldFilter( + FieldFilter.newBuilder() + .setField( + FieldReference.newBuilder().setFieldPath("prop")) + .setOp(Operator.LESS_THAN) + .setValue(valueBuilder().setIntegerValue(42)))) + .addFilters( + Filter.newBuilder() + .setFieldFilter( + FieldFilter.newBuilder() + .setField( + FieldReference.newBuilder().setFieldPath("author")) + .setOp(Operator.EQUAL) + .setValue(valueBuilder().setStringValue("dimond")))) + .addFilters( + Filter.newBuilder() + .setFieldFilter( + FieldFilter.newBuilder() + .setField( + FieldReference.newBuilder().setFieldPath("tags")) + .setOp(Operator.ARRAY_CONTAINS_ANY) + .setValue(valueBuilder().setArrayValue(array)))))) .addOrderBy( Order.newBuilder() .setField(FieldReference.newBuilder().setFieldPath("prop")) @@ -632,10 +710,11 @@ public void testEncodesMultipleFiltersOnDeeperCollections() { assertEquals(serializer.decodeQueryTarget(serializer.encodeQueryTarget(q)), q); } - // PORTING NOTE: Isolated array-contains filter test omitted since we seem to have omitted - // isolated filter tests on Android (and the encodeRelationFilter() / decodeRelationFilter() - // serializer methods are private) in favor of relying on the larger tests. array-contains - // encoding / decoding is covered by testEncodesMultipleFiltersOnDeeperCollections(). + // PORTING NOTE: Isolated array-contains, array-contains-any, and in filter test omitted since we + // seem to have omitted isolated filter tests on Android (and the encodeRelationFilter() / + // decodeRelationFilter() serializer methods are private) in favor of relying on the larger tests. + // array-contains, array-contains-any, and in encoding / decoding is covered by + // testEncodesMultipleFiltersOnDeeperCollections(). @Test public void testEncodesNullFilter() { From 0955e99a9f490c6758f8b3c3055d5e174ec71738 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 13 Jun 2019 11:52:07 -0700 Subject: [PATCH 07/15] fix arrayContains query test --- .../java/com/google/firebase/firestore/QueryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 95af064cb33..9e3b206255f 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -420,7 +420,7 @@ public void testQueriesCanUseArrayContainsFilters() { testCollectionWithDocs(map("a", docA, "b", docB, "c", docC, "d", docD)); // Search for "array" to contain 42 - QuerySnapshot snapshot = waitFor(collection.whereArrayContains("arrays", 42L).get()); + QuerySnapshot snapshot = waitFor(collection.whereArrayContains("array", 42L).get()); assertEquals(asList(docA, docB, docD), querySnapshotToValues(snapshot)); // NOTE: The backend doesn't currently support null, NaN, objects, or arrays, so there isn't From 26e2d0b201efb4b5e51a1fbc22caf0cb2ae9f69f Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Fri, 14 Jun 2019 14:17:22 -0700 Subject: [PATCH 08/15] resolve michael's comments --- .../com/google/firebase/firestore/Query.java | 42 ++++--- .../google/firebase/firestore/core/Query.java | 6 +- .../firestore/core/RelationFilter.java | 7 +- .../remote/RemoteSerializerTest.java | 103 +++++++++--------- 4 files changed, 81 insertions(+), 77 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index c53c2d4acbd..f98a7377605 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -300,9 +300,10 @@ public Query whereGreaterThanOrEqualTo(@NonNull FieldPath fieldPath, @NonNull Ob * specified field, the value must be an array, and that the array must contain the provided * value. * - *

A Query can have only one whereArrayContains() filter. + *

A Query can have only one whereArrayContains() filter and it cannot be combined with + * whereArrayContainsAny(). * - * @param field The name of the field containing an array to search + * @param field The name of the field containing an array to search. * @param value The value that must be contained in the array * @return The created Query. */ @@ -317,9 +318,10 @@ public Query whereArrayContains(@NonNull String field, @NonNull Object value) { * specified field, the value must be an array, and that the array must contain the provided * value. * - *

A Query can have only one whereArrayContains() filter. + *

A Query can have only one whereArrayContains() filter and it cannot be combined with + * whereArrayContainsAny(). * - * @param fieldPath The path of the field containing an array to search + * @param fieldPath The path of the field containing an array to search. * @param value The value that must be contained in the array * @return The created Query. */ @@ -331,12 +333,13 @@ public Query whereArrayContains(@NonNull FieldPath fieldPath, @NonNull Object va /** * Creates and returns a new Query with the additional filter that documents must contain the - * specified field, the value must be an array, and that the array must contain any values of the - * provided array. + * specified field, the value must be an array, and that the array must contain at least one value + * from the provided array. * - *

A Query can have only one whereArrayContainsAny() filter. + *

A Query can have only one whereArrayContainsAny() filter and it cannot be combined with + * whereArrayContains() or whereIn(). * - * @param field The name of the field containing an array to search + * @param field The name of the field containing an array to search. * @param value The array that contains the values to match. * @return The created Query. */ @@ -348,12 +351,13 @@ public Query whereArrayContainsAny(@NonNull String field, @NonNull List /** * Creates and returns a new Query with the additional filter that documents must contain the - * specified field, the value must be an array, and that the array must contain any values of the - * provided array. + * specified field, the value must be an array, and that the array must contain at least one value + * from the provided array. * - *

A Query can have only one whereArrayContainsAny() filter. + *

A Query can have only one whereArrayContainsAny() filter and it cannot be combined with + * whereArrayContains() or whereIn().. * - * @param fieldPath The path of the field containing an array to search + * @param fieldPath The path of the field containing an array to search. * @param value The array that contains the values to match. * @return The created Query. */ @@ -365,11 +369,12 @@ public Query whereArrayContainsAny(@NonNull FieldPath fieldPath, @NonNull ListA Query can have only one whereIn() filter. + *

A Query can have only one whereIn() filter it cannot be combined with + * whereArrayContainsAny(). * - * @param field The name of the field containing an array to search + * @param field The name of the field containing an array to search. * @param value The array that contains the values to match. * @return The created Query. */ @@ -381,11 +386,12 @@ public Query whereIn(@NonNull String field, @NonNull List value) { /** * Creates and returns a new Query with the additional filter that documents must contain the - * specified field and that the field must equal any values of the provided array. + * specified field and the value must equal at least one value from the provided array. * - *

A Query can have only one whereIn() filter. + *

A Query can have only one whereIn() filter it cannot be combined with + * whereArrayContainsAny(). * - * @param fieldPath The path of the field containing an array to search + * @param fieldPath The path of the field containing an array to search. * @param value The array that contains the values to match. * @return The created Query. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 8b5243972e1..3267accf7f7 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -167,11 +167,11 @@ public FieldPath inequalityField() { return null; } - @Nullable /** - * Checks if any of the provided RelationOps are included in the query and returns the first one - * that is, or null if none are. + * Checks if any of the provided filter operatorsrelation are included in the query and returns + * the first one that is, or null if none are. */ + @Nullable public Operator findOperatorFilter(List filterOps) { for (Filter filter : filters) { if (filter instanceof RelationFilter) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java index 168290946ed..a67a01040ee 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java @@ -78,9 +78,12 @@ private boolean matchesValue(FieldValue other) { if (operator == Operator.ARRAY_CONTAINS) { return other instanceof ArrayValue && ((ArrayValue) other).getInternalValue().contains(value); } else if (operator == Operator.IN) { - return value instanceof ArrayValue && ((ArrayValue) value).getInternalValue().contains(other); + hardAssert(value instanceof ArrayValue, "'in' filter has invalid value: " + value); + return ((ArrayValue) value).getInternalValue().contains(other); } else if (operator == Operator.ARRAY_CONTAINS_ANY) { - if (other instanceof ArrayValue && value instanceof ArrayValue) { + hardAssert( + value instanceof ArrayValue, "'array_contains_any' filter has invalid value: " + value); + if (other instanceof ArrayValue) { for (FieldValue val : ((ArrayValue) other).getInternalValue()) { if (((ArrayValue) value).getInternalValue().contains(val)) { return true; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java index 2741c0838ae..f2ced9a66b2 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java @@ -578,12 +578,7 @@ public void testEncodesMultipleFiltersOnDeeperCollections() { .filter(filter("prop", "<", 42)) .filter(filter("author", "==", "dimond")) .filter(filter("tags", "array-contains", "pending")) - .filter(filter("tags", "in", asList("pending", "dimond"))); Target actual = serializer.encodeTarget(wrapQueryData(q)); - ArrayValue.Builder array = - ArrayValue.newBuilder() - .addValues(valueBuilder().setStringValue("pending")) - .addValues(valueBuilder().setStringValue("dimond")); StructuredQuery.Builder structuredQueryBuilder = StructuredQuery.newBuilder() @@ -616,15 +611,7 @@ public void testEncodesMultipleFiltersOnDeeperCollections() { .setField( FieldReference.newBuilder().setFieldPath("tags")) .setOp(Operator.ARRAY_CONTAINS) - .setValue(valueBuilder().setStringValue("pending")))) - .addFilters( - Filter.newBuilder() - .setFieldFilter( - FieldFilter.newBuilder() - .setField( - FieldReference.newBuilder().setFieldPath("tags")) - .setOp(Operator.IN) - .setValue(valueBuilder().setArrayValue(array)))))) + .setValue(valueBuilder().setStringValue("pending")))))) .addOrderBy( Order.newBuilder() .setField(FieldReference.newBuilder().setFieldPath("prop")) @@ -646,14 +633,49 @@ public void testEncodesMultipleFiltersOnDeeperCollections() { } @Test - public void testEncodesDisjunctiveFiltersOnDeeperCollections() { + public void testInSerialization() { + Query q = + Query.atPath(ResourcePath.fromString("rooms/1/messages/10/attachments")) + .filter(filter("tags", "in", asList("pending", "dimond"))); + Target actual = serializer.encodeTarget(wrapQueryData(q)); + ArrayValue.Builder inFilterValue = + ArrayValue.newBuilder() + .addValues(valueBuilder().setStringValue("pending")) + .addValues(valueBuilder().setStringValue("dimond")); + + StructuredQuery.Builder structuredQueryBuilder = + StructuredQuery.newBuilder() + .addFrom(CollectionSelector.newBuilder().setCollectionId("attachments")) + .setWhere( + Filter.newBuilder() + .setFieldFilter( + FieldFilter.newBuilder() + .setField(FieldReference.newBuilder().setFieldPath("tags")) + .setOp(Operator.IN) + .setValue(valueBuilder().setArrayValue(inFilterValue)))) + .addOrderBy(defaultKeyOrder()); + QueryTarget.Builder queryBuilder = + QueryTarget.newBuilder() + .setParent("projects/p/databases/d/documents/rooms/1/messages/10") + .setStructuredQuery(structuredQueryBuilder); + Target expected = + Target.newBuilder() + .setQuery(queryBuilder) + .setTargetId(1) + .setResumeToken(ByteString.EMPTY) + .build(); + + assertEquals(expected, actual); + assertEquals(serializer.decodeQueryTarget(serializer.encodeQueryTarget(q)), q); + } + + @Test + public void testArrayContainsAnySerialization() { Query q = Query.atPath(ResourcePath.fromString("rooms/1/messages/10/attachments")) - .filter(filter("prop", "<", 42)) - .filter(filter("author", "==", "dimond")) .filter(filter("tags", "array-contains-any", asList("pending", "dimond"))); Target actual = serializer.encodeTarget(wrapQueryData(q)); - ArrayValue.Builder array = + ArrayValue.Builder arrayContainsAnyFilterValue = ArrayValue.newBuilder() .addValues(valueBuilder().setStringValue("pending")) .addValues(valueBuilder().setStringValue("dimond")); @@ -663,37 +685,11 @@ public void testEncodesDisjunctiveFiltersOnDeeperCollections() { .addFrom(CollectionSelector.newBuilder().setCollectionId("attachments")) .setWhere( Filter.newBuilder() - .setCompositeFilter( - StructuredQuery.CompositeFilter.newBuilder() - .setOp(CompositeFilter.Operator.AND) - .addFilters( - Filter.newBuilder() - .setFieldFilter( - FieldFilter.newBuilder() - .setField( - FieldReference.newBuilder().setFieldPath("prop")) - .setOp(Operator.LESS_THAN) - .setValue(valueBuilder().setIntegerValue(42)))) - .addFilters( - Filter.newBuilder() - .setFieldFilter( - FieldFilter.newBuilder() - .setField( - FieldReference.newBuilder().setFieldPath("author")) - .setOp(Operator.EQUAL) - .setValue(valueBuilder().setStringValue("dimond")))) - .addFilters( - Filter.newBuilder() - .setFieldFilter( - FieldFilter.newBuilder() - .setField( - FieldReference.newBuilder().setFieldPath("tags")) - .setOp(Operator.ARRAY_CONTAINS_ANY) - .setValue(valueBuilder().setArrayValue(array)))))) - .addOrderBy( - Order.newBuilder() - .setField(FieldReference.newBuilder().setFieldPath("prop")) - .setDirection(Direction.ASCENDING)) + .setFieldFilter( + FieldFilter.newBuilder() + .setField(FieldReference.newBuilder().setFieldPath("tags")) + .setOp(Operator.ARRAY_CONTAINS_ANY) + .setValue(valueBuilder().setArrayValue(arrayContainsAnyFilterValue)))) .addOrderBy(defaultKeyOrder()); QueryTarget.Builder queryBuilder = QueryTarget.newBuilder() @@ -710,11 +706,10 @@ public void testEncodesDisjunctiveFiltersOnDeeperCollections() { assertEquals(serializer.decodeQueryTarget(serializer.encodeQueryTarget(q)), q); } - // PORTING NOTE: Isolated array-contains, array-contains-any, and in filter test omitted since we - // seem to have omitted isolated filter tests on Android (and the encodeRelationFilter() / - // decodeRelationFilter() serializer methods are private) in favor of relying on the larger tests. - // array-contains, array-contains-any, and in encoding / decoding is covered by - // testEncodesMultipleFiltersOnDeeperCollections(). + // PORTING NOTE: Isolated array-contains filter test omitted since we seem to have omitted + // isolated filter tests on Android (and the encodeRelationFilter() / decodeRelationFilter() + // serializer methods are private) in favor of relying on the larger tests. array-contains + // encoding / decoding is covered by testEncodesMultipleFiltersOnDeeperCollections(). @Test public void testEncodesNullFilter() { From 9ecf3ba3d72649af316d801fcfc17b9ab285a1f5 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Mon, 17 Jun 2019 15:19:17 -0700 Subject: [PATCH 09/15] add checks for null and NaN in arrays --- firebase-firestore/ktx/ktx.gradle | 4 ++-- .../firebase/firestore/ValidationTest.java | 17 +++++++++++++++++ .../com/google/firebase/firestore/Query.java | 12 ++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/ktx/ktx.gradle b/firebase-firestore/ktx/ktx.gradle index cad7aece2d9..31df428d3e9 100644 --- a/firebase-firestore/ktx/ktx.gradle +++ b/firebase-firestore/ktx/ktx.gradle @@ -33,8 +33,8 @@ android { main.java.srcDirs += 'src/main/kotlin' test.java { srcDir 'src/test/kotlin' - srcDir '../src/testUtil/java' - srcDir '../src/roboUtil/java' +// srcDir '../src/testUtil/java' +// srcDir '../src/roboUtil/java' } } testOptions.unitTests.includeAndroidResources = true diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java index a88a282c849..81ba7b6081e 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java @@ -652,6 +652,23 @@ public void queriesInAndArrayContainsAnyArrayRules() { () -> testCollection().whereArrayContainsAny("bar", asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 9, 9)), "Invalid Query. 'array_contains_any' filters support a maximum of 10 elements in the value array."); + + expectError( + () -> testCollection().whereIn("bar", asList("foo", null)), + "Invalid Query. 'in' filters cannot contain 'null' in the value array."); + + expectError( + () -> testCollection().whereArrayContainsAny("bar", asList("foo", null)), + "Invalid Query. 'array_contains_any' filters cannot contain 'null' in the value array."); + + expectError( + () -> testCollection().whereIn("bar", asList("foo", Double.NaN)), + "Invalid Query. 'in' filters cannot contain 'NaN' in the value array."); + + expectError( + () -> testCollection().whereArrayContainsAny("bar", asList("foo", Float.NaN)), + "Invalid Query. 'array_contains_any' filters cannot contain 'NaN' in the value array."); + } @Test diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index f98a7377605..a842eb8dd0e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -469,6 +469,18 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu + op.toString() + "' filters support a maximum of 10 elements in the value array."); } + if (((List) value).contains(null)) { + throw new IllegalArgumentException( + "Invalid Query. '" + + op.toString() + + "' filters cannot contain 'null' in the value array."); + } + if (((List) value).contains(Double.NaN) || ((List) value).contains(Float.NaN)) { + throw new IllegalArgumentException( + "Invalid Query. '" + + op.toString() + + "' filters cannot contain 'NaN' in the value array."); + } } fieldValue = firestore.getDataConverter().parseQueryValue(value); } From a78e562fa8e32e703854dda4d125ba812a8e98ea Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Mon, 17 Jun 2019 20:07:42 -0700 Subject: [PATCH 10/15] allow IN queries for document keys --- .../src/main/java/com/google/firebase/firestore/Query.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index a842eb8dd0e..bbcc842f1a8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -416,7 +416,7 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu FieldValue fieldValue; com.google.firebase.firestore.model.FieldPath internalPath = fieldPath.getInternalPath(); if (internalPath.isKeyField()) { - if (op == Operator.ARRAY_CONTAINS || op == Operator.ARRAY_CONTAINS_ANY || op == Operator.IN) { + if (op == Operator.ARRAY_CONTAINS || op == Operator.ARRAY_CONTAINS_ANY) { throw new IllegalArgumentException( "Invalid query. You can't perform '" + op.toString() From 2228a0836e9454a6ca60a26b94ad360a4770296c Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 19 Jun 2019 10:24:10 -0700 Subject: [PATCH 11/15] fix typo --- .../google/firebase/firestore/remote/RemoteSerializerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java index f2ced9a66b2..430d3f08ca9 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java @@ -577,7 +577,7 @@ public void testEncodesMultipleFiltersOnDeeperCollections() { Query.atPath(ResourcePath.fromString("rooms/1/messages/10/attachments")) .filter(filter("prop", "<", 42)) .filter(filter("author", "==", "dimond")) - .filter(filter("tags", "array-contains", "pending")) + .filter(filter("tags", "array-contains", "pending")); Target actual = serializer.encodeTarget(wrapQueryData(q)); StructuredQuery.Builder structuredQueryBuilder = From 70bdedc1112f91885ab4b1a97b8acab6bb21e0dd Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 19 Jun 2019 13:21:39 -0700 Subject: [PATCH 12/15] pass tests without emulator --- .../google/firebase/firestore/QueryTest.java | 5 ++ .../firebase/firestore/ValidationTest.java | 26 ++++++----- .../com/google/firebase/firestore/Query.java | 12 ++--- .../remote/RemoteSerializerTest.java | 46 +++++++++---------- 4 files changed, 48 insertions(+), 41 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 9e3b206255f..fc49a64599a 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -38,6 +38,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; @@ -427,7 +428,9 @@ public void testQueriesCanUseArrayContainsFilters() { // much of anything else interesting to test. } + // TODO(in-queries): Re-enable once emulator support is added to travis. @Test + @Ignore public void testQueriesCanUseInFilters() { Map docA = map("zip", 98101); Map docB = map("zip", 91102); @@ -448,7 +451,9 @@ public void testQueriesCanUseInFilters() { assertEquals(asList(docF), querySnapshotToValues(snapshot)); } + // TODO(in-queries): Re-enable once emulator support is added to travis. @Test + @Ignore public void testQueriesCanUseArrayContainsAnyFilters() { Map docA = map("array", asList(42L)); Map docB = map("array", asList("a", 42L, "c")); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java index d46bcfa3ebf..9c5893d5feb 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java @@ -650,21 +650,20 @@ public void queriesInAndArrayContainsAnyArrayRules() { "Invalid Query. 'array_contains_any' filters support a maximum of 10 elements in the value array."); expectError( - () -> testCollection().whereIn("bar", asList("foo", null)), - "Invalid Query. 'in' filters cannot contain 'null' in the value array."); + () -> testCollection().whereIn("bar", asList("foo", null)), + "Invalid Query. 'in' filters cannot contain 'null' in the value array."); expectError( - () -> testCollection().whereArrayContainsAny("bar", asList("foo", null)), - "Invalid Query. 'array_contains_any' filters cannot contain 'null' in the value array."); + () -> testCollection().whereArrayContainsAny("bar", asList("foo", null)), + "Invalid Query. 'array_contains_any' filters cannot contain 'null' in the value array."); expectError( - () -> testCollection().whereIn("bar", asList("foo", Double.NaN)), - "Invalid Query. 'in' filters cannot contain 'NaN' in the value array."); + () -> testCollection().whereIn("bar", asList("foo", Double.NaN)), + "Invalid Query. 'in' filters cannot contain 'NaN' in the value array."); expectError( - () -> testCollection().whereArrayContainsAny("bar", asList("foo", Float.NaN)), - "Invalid Query. 'array_contains_any' filters cannot contain 'NaN' in the value array."); - + () -> testCollection().whereArrayContainsAny("bar", asList("foo", Float.NaN)), + "Invalid Query. 'array_contains_any' filters cannot contain 'NaN' in the value array."); } @Test @@ -702,6 +701,12 @@ public void queriesFilteredByDocumentIDMustUseStringsOrDocumentReferences() { + "a valid String or DocumentReference, but it was of type: java.lang.Integer"; expectError(() -> collection.whereGreaterThanOrEqualTo(FieldPath.documentId(), 1), reason); + reason = + "Invalid query. When querying with FieldPath.documentId() you must provide " + + "a valid String or DocumentReference, but it was of type: java.util.Arrays$ArrayList"; + ; + expectError(() -> collection.whereIn(FieldPath.documentId(), asList(1, 2)), reason); + reason = "Invalid query. When querying a collection group by FieldPath.documentId(), the value " + "provided must result in a valid document path, but 'foo' is not because it has " @@ -720,9 +725,6 @@ public void queriesFilteredByDocumentIDMustUseStringsOrDocumentReferences() { "Invalid query. You can't perform 'array_contains_any' queries on FieldPath.documentId()."; expectError( () -> collection.whereArrayContainsAny(FieldPath.documentId(), asList(1, 2)), reason); - - reason = "Invalid query. You can't perform 'in' queries on FieldPath.documentId()."; - expectError(() -> collection.whereIn(FieldPath.documentId(), asList(1, 2)), reason); } // Helpers diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index bbcc842f1a8..7ecb466a742 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -471,15 +471,15 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu } if (((List) value).contains(null)) { throw new IllegalArgumentException( - "Invalid Query. '" - + op.toString() - + "' filters cannot contain 'null' in the value array."); + "Invalid Query. '" + + op.toString() + + "' filters cannot contain 'null' in the value array."); } if (((List) value).contains(Double.NaN) || ((List) value).contains(Float.NaN)) { throw new IllegalArgumentException( - "Invalid Query. '" - + op.toString() - + "' filters cannot contain 'NaN' in the value array."); + "Invalid Query. '" + + op.toString() + + "' filters cannot contain 'NaN' in the value array."); } } fieldValue = firestore.getDataConverter().parseQueryValue(value); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java index 430d3f08ca9..346d9323d6f 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java @@ -635,35 +635,35 @@ public void testEncodesMultipleFiltersOnDeeperCollections() { @Test public void testInSerialization() { Query q = - Query.atPath(ResourcePath.fromString("rooms/1/messages/10/attachments")) - .filter(filter("tags", "in", asList("pending", "dimond"))); + Query.atPath(ResourcePath.fromString("rooms/1/messages/10/attachments")) + .filter(filter("tags", "in", asList("pending", "dimond"))); Target actual = serializer.encodeTarget(wrapQueryData(q)); ArrayValue.Builder inFilterValue = - ArrayValue.newBuilder() - .addValues(valueBuilder().setStringValue("pending")) - .addValues(valueBuilder().setStringValue("dimond")); + ArrayValue.newBuilder() + .addValues(valueBuilder().setStringValue("pending")) + .addValues(valueBuilder().setStringValue("dimond")); StructuredQuery.Builder structuredQueryBuilder = - StructuredQuery.newBuilder() - .addFrom(CollectionSelector.newBuilder().setCollectionId("attachments")) - .setWhere( - Filter.newBuilder() - .setFieldFilter( - FieldFilter.newBuilder() - .setField(FieldReference.newBuilder().setFieldPath("tags")) - .setOp(Operator.IN) - .setValue(valueBuilder().setArrayValue(inFilterValue)))) - .addOrderBy(defaultKeyOrder()); + StructuredQuery.newBuilder() + .addFrom(CollectionSelector.newBuilder().setCollectionId("attachments")) + .setWhere( + Filter.newBuilder() + .setFieldFilter( + FieldFilter.newBuilder() + .setField(FieldReference.newBuilder().setFieldPath("tags")) + .setOp(Operator.IN) + .setValue(valueBuilder().setArrayValue(inFilterValue)))) + .addOrderBy(defaultKeyOrder()); QueryTarget.Builder queryBuilder = - QueryTarget.newBuilder() - .setParent("projects/p/databases/d/documents/rooms/1/messages/10") - .setStructuredQuery(structuredQueryBuilder); + QueryTarget.newBuilder() + .setParent("projects/p/databases/d/documents/rooms/1/messages/10") + .setStructuredQuery(structuredQueryBuilder); Target expected = - Target.newBuilder() - .setQuery(queryBuilder) - .setTargetId(1) - .setResumeToken(ByteString.EMPTY) - .build(); + Target.newBuilder() + .setQuery(queryBuilder) + .setTargetId(1) + .setResumeToken(ByteString.EMPTY) + .build(); assertEquals(expected, actual); assertEquals(serializer.decodeQueryTarget(serializer.encodeQueryTarget(q)), q); From a10d50d4114d0a76b2d47c33898d1b50e0154b66 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 19 Jun 2019 17:03:55 -0700 Subject: [PATCH 13/15] resolve michael's comments and add simpler serialization test --- .../firebase/firestore/ValidationTest.java | 1 - .../com/google/firebase/firestore/Query.java | 14 +-- .../google/firebase/firestore/core/Query.java | 4 +- .../firestore/remote/RemoteSerializer.java | 4 +- .../remote/RemoteSerializerTest.java | 90 ++++++------------- 5 files changed, 41 insertions(+), 72 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java index 9c5893d5feb..460de9e37b7 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java @@ -704,7 +704,6 @@ public void queriesFilteredByDocumentIDMustUseStringsOrDocumentReferences() { reason = "Invalid query. When querying with FieldPath.documentId() you must provide " + "a valid String or DocumentReference, but it was of type: java.util.Arrays$ArrayList"; - ; expectError(() -> collection.whereIn(FieldPath.documentId(), asList(1, 2)), reason); reason = diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 7ecb466a742..195d1869bbe 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -355,7 +355,7 @@ public Query whereArrayContainsAny(@NonNull String field, @NonNull List * from the provided array. * *

A Query can have only one whereArrayContainsAny() filter and it cannot be combined with - * whereArrayContains() or whereIn().. + * whereArrayContains() or whereIn(). * * @param fieldPath The path of the field containing an array to search. * @param value The array that contains the values to match. @@ -369,12 +369,12 @@ public Query whereArrayContainsAny(@NonNull FieldPath fieldPath, @NonNull ListA Query can have only one whereIn() filter it cannot be combined with + *

A Query can have only one whereIn() filter, and it cannot be combined with * whereArrayContainsAny(). * - * @param field The name of the field containing an array to search. + * @param field The name of the field to search. * @param value The array that contains the values to match. * @return The created Query. */ @@ -386,12 +386,12 @@ public Query whereIn(@NonNull String field, @NonNull List value) { /** * Creates and returns a new Query with the additional filter that documents must contain the - * specified field and the value must equal at least one value from the provided array. + * specified field and the value must equal one of the values from the provided array. * - *

A Query can have only one whereIn() filter it cannot be combined with + *

A Query can have only one whereIn() filter, and it cannot be combined with * whereArrayContainsAny(). * - * @param fieldPath The path of the field containing an array to search. + * @param fieldPath The path of the field to search. * @param value The array that contains the values to match. * @return The created Query. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 3267accf7f7..b59971de074 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -168,8 +168,8 @@ public FieldPath inequalityField() { } /** - * Checks if any of the provided filter operatorsrelation are included in the query and returns - * the first one that is, or null if none are. + * Checks if any of the provided filter operators are included in the query and returns the first + * one that is, or null if none are. */ @Nullable public Operator findOperatorFilter(List filterOps) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java index fa9e213b5e9..e2696c25bf5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java @@ -18,6 +18,7 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import com.google.firebase.Timestamp; import com.google.firebase.firestore.Blob; import com.google.firebase.firestore.GeoPoint; @@ -867,7 +868,8 @@ private List decodeFilters(StructuredQuery.Filter proto) { return result; } - private StructuredQuery.Filter encodeRelationFilter(RelationFilter filter) { + @VisibleForTesting + StructuredQuery.Filter encodeRelationFilter(RelationFilter filter) { FieldFilter.Builder proto = FieldFilter.newBuilder(); proto.setField(encodeFieldPath(filter.getField())); proto.setOp(encodeRelationFilterOperator(filter.getOperator())); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java index 346d9323d6f..87b43decd46 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java @@ -36,6 +36,7 @@ import com.google.firebase.firestore.GeoPoint; import com.google.firebase.firestore.core.Bound; import com.google.firebase.firestore.core.Query; +import com.google.firebase.firestore.core.RelationFilter; import com.google.firebase.firestore.local.QueryData; import com.google.firebase.firestore.local.QueryPurpose; import com.google.firebase.firestore.model.DatabaseId; @@ -634,76 +635,43 @@ public void testEncodesMultipleFiltersOnDeeperCollections() { @Test public void testInSerialization() { - Query q = - Query.atPath(ResourcePath.fromString("rooms/1/messages/10/attachments")) - .filter(filter("tags", "in", asList("pending", "dimond"))); - Target actual = serializer.encodeTarget(wrapQueryData(q)); - ArrayValue.Builder inFilterValue = - ArrayValue.newBuilder() - .addValues(valueBuilder().setStringValue("pending")) - .addValues(valueBuilder().setStringValue("dimond")); + StructuredQuery.Filter filter = + serializer.encodeRelationFilter(((RelationFilter) filter("field", "in", asList(42)))); - StructuredQuery.Builder structuredQueryBuilder = - StructuredQuery.newBuilder() - .addFrom(CollectionSelector.newBuilder().setCollectionId("attachments")) - .setWhere( - Filter.newBuilder() - .setFieldFilter( - FieldFilter.newBuilder() - .setField(FieldReference.newBuilder().setFieldPath("tags")) - .setOp(Operator.IN) - .setValue(valueBuilder().setArrayValue(inFilterValue)))) - .addOrderBy(defaultKeyOrder()); - QueryTarget.Builder queryBuilder = - QueryTarget.newBuilder() - .setParent("projects/p/databases/d/documents/rooms/1/messages/10") - .setStructuredQuery(structuredQueryBuilder); - Target expected = - Target.newBuilder() - .setQuery(queryBuilder) - .setTargetId(1) - .setResumeToken(ByteString.EMPTY) + ArrayValue.Builder inFilterValue = + ArrayValue.newBuilder().addValues(valueBuilder().setIntegerValue(42)); + StructuredQuery.Filter expectedFilter = + Filter.newBuilder() + .setFieldFilter( + FieldFilter.newBuilder() + .setField(FieldReference.newBuilder().setFieldPath("field")) + .setOp(Operator.IN) + .setValue(valueBuilder().setArrayValue(inFilterValue)) + .build()) .build(); - assertEquals(expected, actual); - assertEquals(serializer.decodeQueryTarget(serializer.encodeQueryTarget(q)), q); + assertEquals(filter, expectedFilter); } @Test public void testArrayContainsAnySerialization() { - Query q = - Query.atPath(ResourcePath.fromString("rooms/1/messages/10/attachments")) - .filter(filter("tags", "array-contains-any", asList("pending", "dimond"))); - Target actual = serializer.encodeTarget(wrapQueryData(q)); - ArrayValue.Builder arrayContainsAnyFilterValue = - ArrayValue.newBuilder() - .addValues(valueBuilder().setStringValue("pending")) - .addValues(valueBuilder().setStringValue("dimond")); + StructuredQuery.Filter filter = + serializer.encodeRelationFilter( + ((RelationFilter) filter("field", "array-contains-any", asList(42)))); - StructuredQuery.Builder structuredQueryBuilder = - StructuredQuery.newBuilder() - .addFrom(CollectionSelector.newBuilder().setCollectionId("attachments")) - .setWhere( - Filter.newBuilder() - .setFieldFilter( - FieldFilter.newBuilder() - .setField(FieldReference.newBuilder().setFieldPath("tags")) - .setOp(Operator.ARRAY_CONTAINS_ANY) - .setValue(valueBuilder().setArrayValue(arrayContainsAnyFilterValue)))) - .addOrderBy(defaultKeyOrder()); - QueryTarget.Builder queryBuilder = - QueryTarget.newBuilder() - .setParent("projects/p/databases/d/documents/rooms/1/messages/10") - .setStructuredQuery(structuredQueryBuilder); - Target expected = - Target.newBuilder() - .setQuery(queryBuilder) - .setTargetId(1) - .setResumeToken(ByteString.EMPTY) + ArrayValue.Builder arrayContainsAnyFilterValue = + ArrayValue.newBuilder().addValues(valueBuilder().setIntegerValue(42)); + StructuredQuery.Filter expectedFilter = + Filter.newBuilder() + .setFieldFilter( + FieldFilter.newBuilder() + .setField(FieldReference.newBuilder().setFieldPath("field")) + .setOp(Operator.ARRAY_CONTAINS_ANY) + .setValue(valueBuilder().setArrayValue(arrayContainsAnyFilterValue)) + .build()) .build(); - - assertEquals(expected, actual); - assertEquals(serializer.decodeQueryTarget(serializer.encodeQueryTarget(q)), q); + + assertEquals(filter, expectedFilter); } // PORTING NOTE: Isolated array-contains filter test omitted since we seem to have omitted From a023941bd4df181be9cf8133e2c4e93a0183104c Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 19 Jun 2019 17:05:48 -0700 Subject: [PATCH 14/15] add porting note --- .../firebase/firestore/remote/RemoteSerializerTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java index 87b43decd46..bb2f072dec7 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java @@ -670,14 +670,13 @@ public void testArrayContainsAnySerialization() { .setValue(valueBuilder().setArrayValue(arrayContainsAnyFilterValue)) .build()) .build(); - + assertEquals(filter, expectedFilter); } - // PORTING NOTE: Isolated array-contains filter test omitted since we seem to have omitted - // isolated filter tests on Android (and the encodeRelationFilter() / decodeRelationFilter() - // serializer methods are private) in favor of relying on the larger tests. array-contains - // encoding / decoding is covered by testEncodesMultipleFiltersOnDeeperCollections(). + // TODO(PORTING NOTE): Android currently tests most filter serialization (for equals, greater + // than, array-contains, etc.) only in testEncodesMultipleFiltersOnDeeperCollections and lacks + // isolated filter tests like the other platforms have. We should fix this. @Test public void testEncodesNullFilter() { From a532b287aae6eb9380f67988b00c8595ecac56c8 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 19 Jun 2019 18:04:25 -0700 Subject: [PATCH 15/15] hide API from public --- .../com/google/firebase/firestore/Query.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 195d1869bbe..92e15387128 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -343,9 +343,9 @@ public Query whereArrayContains(@NonNull FieldPath fieldPath, @NonNull Object va * @param value The array that contains the values to match. * @return The created Query. */ + // TODO(in-queries): Expose to public once backend is ready. @NonNull - @PublicApi - public Query whereArrayContainsAny(@NonNull String field, @NonNull List value) { + Query whereArrayContainsAny(@NonNull String field, @NonNull List value) { return whereHelper(FieldPath.fromDotSeparatedPath(field), Operator.ARRAY_CONTAINS_ANY, value); } @@ -361,9 +361,9 @@ public Query whereArrayContainsAny(@NonNull String field, @NonNull List * @param value The array that contains the values to match. * @return The created Query. */ + // TODO(in-queries): Expose to public once backend is ready. @NonNull - @PublicApi - public Query whereArrayContainsAny(@NonNull FieldPath fieldPath, @NonNull List value) { + Query whereArrayContainsAny(@NonNull FieldPath fieldPath, @NonNull List value) { return whereHelper(fieldPath, Operator.ARRAY_CONTAINS_ANY, value); } @@ -378,9 +378,9 @@ public Query whereArrayContainsAny(@NonNull FieldPath fieldPath, @NonNull List value) { + Query whereIn(@NonNull String field, @NonNull List value) { return whereHelper(FieldPath.fromDotSeparatedPath(field), Operator.IN, value); } @@ -395,9 +395,9 @@ public Query whereIn(@NonNull String field, @NonNull List value) { * @param value The array that contains the values to match. * @return The created Query. */ + // TODO(in-queries): Expose to public once backend is ready. @NonNull - @PublicApi - public Query whereIn(@NonNull FieldPath fieldPath, @NonNull List value) { + Query whereIn(@NonNull FieldPath fieldPath, @NonNull List value) { return whereHelper(fieldPath, Operator.IN, value); }