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 e6ac9139fd7..2d9917dcd87 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 @@ -15,6 +15,12 @@ package com.google.firebase.firestore; import static com.google.common.truth.Truth.assertThat; +import static com.google.firebase.firestore.Filter.and; +import static com.google.firebase.firestore.Filter.equalTo; +import static com.google.firebase.firestore.Filter.greaterThan; +import static com.google.firebase.firestore.Filter.inArray; +import static com.google.firebase.firestore.Filter.notInArray; +import static com.google.firebase.firestore.Filter.or; import static com.google.firebase.firestore.testutil.Assert.assertThrows; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testAlternateFirestore; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollection; @@ -39,6 +45,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.Arrays; import java.util.Date; import java.util.List; import java.util.Map; @@ -815,6 +822,74 @@ public void queriesUsingInAndDocumentIdMustHaveProperDocumentReferencesInArray() reason); } + @Test + public void testInvalidQueryFilters() { + CollectionReference collection = testCollection(); + + // Multiple inequalities, one of which is inside a nested composite filter. + String reason = + "All where filters with an inequality (notEqualTo, notIn, lessThan, lessThanOrEqualTo, greaterThan, or greaterThanOrEqualTo) must be on the same field. But you have filters on 'c' and 'r'"; + expectError( + () -> + collection + .where( + or( + and(equalTo("a", "b"), greaterThan("c", "d")), + and(equalTo("e", "f"), equalTo("g", "h")))) + .where(greaterThan("r", "s")), + reason); + + // OrderBy and inequality on different fields. Inequality inside a nested composite filter. + reason = + "Invalid query. You have an inequality where filter (whereLessThan(), whereGreaterThan(), etc.) on field 'c' and so you must also have 'c' as your first orderBy() field, but your first orderBy() is currently on field 'r' instead."; + expectError( + () -> + collection + .where( + or( + and(equalTo("a", "b"), greaterThan("c", "d")), + and(equalTo("e", "f"), equalTo("g", "h")))) + .orderBy("r"), + reason); + + // Conflicting operations within a composite filter. + reason = "Invalid Query. You cannot use 'not_in' filters with 'in' filters."; + expectError( + () -> + collection.where( + or( + and(equalTo("a", "b"), inArray("c", Arrays.asList("d", "e"))), + and(equalTo("e", "f"), notInArray("c", Arrays.asList("f", "g"))))), + reason); + + // Conflicting operations between a field filter and a composite filter. + reason = "Invalid Query. You cannot use 'not_in' filters with 'in' filters."; + expectError( + () -> + collection + .where( + or( + and(equalTo("a", "b"), inArray("c", Arrays.asList("d", "e"))), + and(equalTo("e", "f"), equalTo("g", "h")))) + .where(notInArray("i", Arrays.asList("j", "k"))), + reason); + + // Conflicting operations between two composite filters. + reason = "Invalid Query. You cannot use 'not_in' filters with 'in' filters."; + expectError( + () -> + collection + .where( + or( + and(equalTo("a", "b"), inArray("c", Arrays.asList("d", "e"))), + and(equalTo("e", "f"), equalTo("g", "h")))) + .where( + or( + and(equalTo("i", "j"), notInArray("l", Arrays.asList("m", "n"))), + and(equalTo("o", "p"), equalTo("q", "r")))), + reason); + } + // Helpers /** Performs a write using each write API and makes sure it succeeds. */ 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 7c8f863a3b4..64de2b5783c 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 @@ -468,7 +468,7 @@ private com.google.firebase.firestore.core.Filter parseFilter(Filter filter) { } // TODO(orquery): This method will become public API. Change visibility and add documentation. - private Query where(Filter filter) { + Query where(Filter filter) { com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter); if (parsedFilter.getFilters().isEmpty()) { // Return the existing query if not adding any more filters (e.g. an empty composite filter). @@ -639,7 +639,7 @@ private void validateNewFilter(com.google.firebase.firestore.core.Filter filter) com.google.firebase.firestore.core.Query testQuery = query; for (FieldFilter subfilter : filter.getFlattenedFilters()) { validateNewFieldFilter(testQuery, subfilter); - testQuery = query.filter(subfilter); + testQuery = testQuery.filter(subfilter); } } @@ -651,10 +651,9 @@ private void validateNewFilter(com.google.firebase.firestore.core.Filter filter) private Operator findFilterWithOperator( List filters, List operators) { for (com.google.firebase.firestore.core.Filter filter : filters) { - if (filter instanceof FieldFilter) { - Operator filterOp = ((FieldFilter) filter).getOperator(); - if (operators.contains(filterOp)) { - return filterOp; + for (FieldFilter fieldFilter : filter.getFlattenedFilters()) { + if (operators.contains(fieldFilter.getOperator())) { + return fieldFilter.getOperator(); } } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java index 2aba7fcf219..ba5d7883e5f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java @@ -28,6 +28,10 @@ public class CompositeFilter extends Filter { private final List filters; private final Operator operator; + // Memoized list of all field filters that can be found by traversing the tree of filters + // contained in this composite filter. + private List memoizedFlattenedFilters; + public CompositeFilter(List filters, Operator operator) { this.filters = filters; this.operator = operator; @@ -44,12 +48,14 @@ public Operator getOperator() { @Override public List getFlattenedFilters() { - // TODO(orquery): memoize this result if this method is used more than once. - List result = new ArrayList<>(); + if (memoizedFlattenedFilters != null) { + return memoizedFlattenedFilters; + } + memoizedFlattenedFilters = new ArrayList<>(); for (Filter subfilter : filters) { - result.addAll(subfilter.getFlattenedFilters()); + memoizedFlattenedFilters.addAll(subfilter.getFlattenedFilters()); } - return result; + return memoizedFlattenedFilters; } /**