Fix validation bug for composite filters. (#3609)

* Fix validation bug for composite filters.

* Address feedback.

* Remove unused import.

* Address feedback.

Author: ehsannas

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. */

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<com.google.firebase.firestore.core.Filter> filters, List<Operator> 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();
         }
       }
     }

firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java

@@ -28,6 +28,10 @@ public class CompositeFilter extends Filter {
   private final List<Filter> 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<FieldFilter> memoizedFlattenedFilters;
+
   public CompositeFilter(List<Filter> filters, Operator operator) {
     this.filters = filters;
     this.operator = operator;
@@ -44,12 +48,14 @@ public Operator getOperator() {
 
   @Override
   public List<FieldFilter> getFlattenedFilters() {
-    // TODO(orquery): memoize this result if this method is used more than once.
-    List<FieldFilter> 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;
   }
 
   /**