Refactor Filters

firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/testutil/TestUtil.java

@@ -33,7 +33,7 @@
 import com.google.firebase.firestore.DocumentReference;
 import com.google.firebase.firestore.TestAccessHelper;
 import com.google.firebase.firestore.UserDataConverter;
-import com.google.firebase.firestore.core.Filter;
+import com.google.firebase.firestore.core.FieldFilter;
 import com.google.firebase.firestore.core.Filter.Operator;
 import com.google.firebase.firestore.core.OrderBy;
 import com.google.firebase.firestore.core.OrderBy.Direction;
@@ -220,8 +220,8 @@ public static ImmutableSortedSet<DocumentKey> keySet(DocumentKey... keys) {
     return keySet;
   }
 
-  public static Filter filter(String key, String operator, Object value) {
-    return Filter.create(field(key), operatorFromString(operator), wrap(value));
+  public static FieldFilter filter(String key, String operator, Object value) {
+    return FieldFilter.create(field(key), operatorFromString(operator), wrap(value));
   }
 
   public static Operator operatorFromString(String s) {

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

@@ -429,10 +429,11 @@ public void testQueriesCanUseArrayContainsFilters() {
     // much of anything else interesting to test.
   }
 
-  // TODO(in-queries): Re-enable in prod once feature lands in backend.
   @Test
   public void testQueriesCanUseInFilters() {
+    // TODO(in-queries): Re-enable in prod once feature lands in backend.
     Assume.assumeTrue(isRunningAgainstEmulator());
+
     Map<String, Object> docA = map("zip", 98101L);
     Map<String, Object> docB = map("zip", 91102L);
     Map<String, Object> docC = map("zip", 98103L);
@@ -452,10 +453,32 @@ public void testQueriesCanUseInFilters() {
     assertEquals(asList(docF), querySnapshotToValues(snapshot));
   }
 
-  // TODO(in-queries): Re-enable in prod once feature lands in backend.
+  @Test
+  public void testQueriesCanUseInFiltersWithDocIds() {
+    // TODO(in-queries): Re-enable in prod once feature lands in backend.
+    Assume.assumeTrue(isRunningAgainstEmulator());
+
+    Map<String, String> docA = map("key", "aa");
+    Map<String, String> docB = map("key", "ab");
+    Map<String, String> docC = map("key", "ba");
+    Map<String, String> docD = map("key", "bb");
+    Map<String, Map<String, Object>> testDocs =
+        map(
+            "aa", docA,
+            "ab", docB,
+            "ba", docC,
+            "bb", docD);
+    CollectionReference collection = testCollectionWithDocs(testDocs);
+    QuerySnapshot docs =
+        waitFor(collection.whereIn(FieldPath.documentId(), asList("aa", "ab")).get());
+    assertEquals(asList(docA, docB), querySnapshotToValues(docs));
+  }
+
   @Test
   public void testQueriesCanUseArrayContainsAnyFilters() {
+    // TODO(in-queries): Re-enable in prod once feature lands in backend.
     Assume.assumeTrue(isRunningAgainstEmulator());
+
     Map<String, Object> docA = map("array", asList(42L));
     Map<String, Object> docB = map("array", asList("a", 42L, "c"));
     Map<String, Object> docC = map("array", asList(41.999, "42", map("a", asList(42))));

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

@@ -29,12 +29,12 @@
 import com.google.firebase.firestore.core.AsyncEventListener;
 import com.google.firebase.firestore.core.Bound;
 import com.google.firebase.firestore.core.EventManager.ListenOptions;
+import com.google.firebase.firestore.core.FieldFilter;
 import com.google.firebase.firestore.core.Filter;
 import com.google.firebase.firestore.core.Filter.Operator;
 import com.google.firebase.firestore.core.ListenerRegistrationImpl;
 import com.google.firebase.firestore.core.OrderBy;
 import com.google.firebase.firestore.core.QueryListener;
-import com.google.firebase.firestore.core.RelationFilter;
 import com.google.firebase.firestore.core.ViewSnapshot;
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
@@ -368,7 +368,7 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu
       }
       fieldValue = firestore.getDataConverter().parseQueryValue(value);
     }
-    Filter filter = Filter.create(fieldPath.getInternalPath(), op, fieldValue);
+    Filter filter = FieldFilter.create(fieldPath.getInternalPath(), op, fieldValue);
     validateNewFilter(filter);
     return new Query(query.filter(filter), firestore);
   }
@@ -465,15 +465,15 @@ private void validateOrderByFieldMatchesInequality(
   }
 
   private void validateNewFilter(Filter filter) {
-    if (filter instanceof RelationFilter) {
-      Operator filterOp = ((RelationFilter) filter).getOperator();
+    if (filter instanceof FieldFilter) {
+      FieldFilter fieldFilter = (FieldFilter) filter;
+      Operator filterOp = fieldFilter.getOperator();
       List<Operator> arrayOps = Arrays.asList(Operator.ARRAY_CONTAINS, Operator.ARRAY_CONTAINS_ANY);
       List<Operator> 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()) {
+      if (fieldFilter.isInequality()) {
         com.google.firebase.firestore.model.FieldPath existingInequality = query.inequalityField();
         com.google.firebase.firestore.model.FieldPath newInequality = filter.getField();
 
@@ -494,10 +494,10 @@ private void validateNewFilter(Filter filter) {
         // conflicts with an existing one.
         Operator conflictingOp = null;
         if (isDisjunctiveOp) {
-          conflictingOp = this.query.findOperatorFilter(disjunctiveOps);
+          conflictingOp = this.query.findFilterOperator(disjunctiveOps);
         }
         if (conflictingOp == null && isArrayOp) {
-          conflictingOp = this.query.findOperatorFilter(arrayOps);
+          conflictingOp = this.query.findFilterOperator(arrayOps);
         }
         if (conflictingOp != null) {
           // We special case when it's a duplicate op to give a slightly clearer error message.

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

@@ -0,0 +1,42 @@
+// 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.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.firebase.firestore.core;
+
+import com.google.firebase.firestore.model.Document;
+import com.google.firebase.firestore.model.FieldPath;
+import com.google.firebase.firestore.model.value.ArrayValue;
+import com.google.firebase.firestore.model.value.FieldValue;
+
+/** A Filter that implements the array-contains-any operator. */
+public class ArrayContainsAnyFilter extends FieldFilter {
+  ArrayContainsAnyFilter(FieldPath field, FieldValue value) {
+    super(field, Operator.ARRAY_CONTAINS_ANY, value);
+  }
+
+  @Override
+  public boolean matches(Document doc) {
+    ArrayValue arrayValue = (ArrayValue) getValue();
+    FieldValue other = doc.getField(getField());
+    if (!(other instanceof ArrayValue)) {
+      return false;
+    }
+    for (FieldValue val : ((ArrayValue) other).getInternalValue()) {
+      if (arrayValue.getInternalValue().contains(val)) {
+        return true;
+      }
+    }
+    return false;
+  }
+}

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

@@ -0,0 +1,34 @@
+// 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.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.firebase.firestore.core;
+
+import com.google.firebase.firestore.model.Document;
+import com.google.firebase.firestore.model.FieldPath;
+import com.google.firebase.firestore.model.value.ArrayValue;
+import com.google.firebase.firestore.model.value.FieldValue;
+
+/** A Filter that implements the array-contains operator. */
+public class ArrayContainsFilter extends FieldFilter {
+  ArrayContainsFilter(FieldPath field, FieldValue value) {
+    super(field, Operator.ARRAY_CONTAINS, value);
+  }
+
+  @Override
+  public boolean matches(Document doc) {
+    FieldValue other = doc.getField(getField());
+    return other instanceof ArrayValue
+        && ((ArrayValue) other).getInternalValue().contains(getValue());
+  }
+}

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

@@ -17,15 +17,17 @@
 import static com.google.firebase.firestore.util.Assert.hardAssert;
 
 import com.google.firebase.firestore.model.Document;
-import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.FieldPath;
 import com.google.firebase.firestore.model.value.ArrayValue;
+import com.google.firebase.firestore.model.value.DoubleValue;
 import com.google.firebase.firestore.model.value.FieldValue;
+import com.google.firebase.firestore.model.value.NullValue;
+import com.google.firebase.firestore.model.value.ReferenceValue;
 import com.google.firebase.firestore.util.Assert;
 import java.util.Arrays;
 
 /** Represents a filter to be applied to query. */
-public class RelationFilter extends Filter {
+public class FieldFilter extends Filter {
   private final Operator operator;
 
   private final FieldValue value;
@@ -36,7 +38,7 @@ public class RelationFilter extends Filter {
    * Creates a new filter that compares fields and values. Only intended to be called from
    * Filter.create().
    */
-  RelationFilter(FieldPath field, Operator operator, FieldValue value) {
+  protected FieldFilter(FieldPath field, Operator operator, FieldValue value) {
     this.field = field;
     this.operator = operator;
     this.value = value;
@@ -55,50 +57,67 @@ public FieldValue getValue() {
     return value;
   }
 
-  @Override
-  public boolean matches(Document doc) {
-    if (this.field.isKeyField()) {
-      Object refValue = value.value();
-      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.toString() + "' queries don't make sense on document keys.");
-      int comparison = DocumentKey.comparator().compare(doc.getKey(), (DocumentKey) refValue);
-      return matchesComparison(comparison);
-    } else {
-      FieldValue value = doc.getField(field);
-      return value != null && matchesValue(doc.getField(field));
-    }
-  }
-
-  private boolean matchesValue(FieldValue other) {
-    if (operator == Operator.ARRAY_CONTAINS) {
-      return other instanceof ArrayValue && ((ArrayValue) other).getInternalValue().contains(value);
+  /**
+   * Gets a Filter instance for the provided path, operator, and value.
+   *
+   * <p>Note that if the relation operator is EQUAL and the value is null or NaN, this will return
+   * the appropriate NullFilter or NaNFilter class instead of a FieldFilter.
+   */
+  public static FieldFilter create(FieldPath path, Operator operator, FieldValue value) {
+    if (path.isKeyField()) {
+      if (operator == Operator.IN) {
+        hardAssert(
+            value instanceof ArrayValue,
+            "Comparing on key with IN, but an array value was not a RefValue");
+        return new KeyFieldInFilter(path, (ArrayValue) value);
+      } else {
+        hardAssert(
+            value instanceof ReferenceValue,
+            "Comparing on key, but filter value not a ReferenceValue");
+        hardAssert(
+            operator != Operator.ARRAY_CONTAINS && operator != Operator.ARRAY_CONTAINS_ANY,
+            operator.toString() + "queries don't make sense on document keys");
+        return new KeyFieldFilter(path, operator, (ReferenceValue) value);
+      }
+    } else if (value.equals(NullValue.nullValue())) {
+      if (operator != Filter.Operator.EQUAL) {
+        throw new IllegalArgumentException(
+            "Invalid Query. You can only perform equality comparisons on null (via "
+                + "whereEqualTo()).");
+      }
+      return new FieldFilter(path, operator, value);
+    } else if (value.equals(DoubleValue.NaN)) {
+      if (operator != Filter.Operator.EQUAL) {
+        throw new IllegalArgumentException(
+            "Invalid Query. You can only perform equality comparisons on NaN (via "
+                + "whereEqualTo()).");
+      }
+      return new FieldFilter(path, operator, value);
+    } else if (operator == Operator.ARRAY_CONTAINS) {
+      return new ArrayContainsFilter(path, value);
     } else if (operator == Operator.IN) {
-      hardAssert(value instanceof ArrayValue, "'in' filter has invalid value: " + value);
-      return ((ArrayValue) value).getInternalValue().contains(other);
+      hardAssert(value instanceof ArrayValue, "IN filter has invalid value: " + value.toString());
+      return new InFilter(path, (ArrayValue) value);
     } else if (operator == Operator.ARRAY_CONTAINS_ANY) {
       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;
-          }
-        }
-      }
-      return false;
+          value instanceof ArrayValue,
+          "ARRAY_CONTAINS_ANY filter has invalid value: " + value.toString());
+      return new ArrayContainsAnyFilter(path, (ArrayValue) value);
     } else {
-      // Only compare types with matching backend order (such as double and int).
-      return value.typeOrder() == other.typeOrder()
-          && matchesComparison(other.compareTo(this.value));
+      return new FieldFilter(path, operator, value);
     }
   }
 
-  private boolean matchesComparison(int comp) {
+  @Override
+  public boolean matches(Document doc) {
+    FieldValue other = doc.getField(field);
+    // Only compare types with matching backend order (such as double and int).
+    return other != null
+        && value.typeOrder() == other.typeOrder()
+        && this.matchesComparison(other.compareTo(value));
+  }
+
+  protected boolean matchesComparison(int comp) {
     switch (operator) {
       case LESS_THAN:
         return comp < 0;
@@ -111,7 +130,7 @@ private boolean matchesComparison(int comp) {
       case GREATER_THAN_OR_EQUAL:
         return comp >= 0;
       default:
-        throw Assert.fail("Unknown operator: %s", operator);
+        throw Assert.fail("Unknown FieldFilter operator: %s", operator);
     }
   }
 
@@ -138,10 +157,10 @@ public String toString() {
 
   @Override
   public boolean equals(Object o) {
-    if (o == null || !(o instanceof RelationFilter)) {
+    if (o == null || !(o instanceof FieldFilter)) {
       return false;
     }
-    RelationFilter other = (RelationFilter) o;
+    FieldFilter other = (FieldFilter) o;
     return operator == other.operator && field.equals(other.field) && value.equals(other.value);
   }
 

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

@@ -16,9 +16,6 @@
 
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.FieldPath;
-import com.google.firebase.firestore.model.value.DoubleValue;
-import com.google.firebase.firestore.model.value.FieldValue;
-import com.google.firebase.firestore.model.value.NullValue;
 
 /** Interface used for all query filters. */
 public abstract class Filter {
@@ -44,32 +41,6 @@ public String toString() {
     }
   }
 
-  /**
-   * Gets a Filter instance for the provided path, operator, and value.
-   *
-   * <p>Note that if the relation operator is EQUAL and the value is null or NaN, this will return
-   * the appropriate NullFilter or NaNFilter class instead of a RelationFilter.
-   */
-  public static Filter create(FieldPath path, Operator operator, FieldValue value) {
-    if (value.equals(NullValue.nullValue())) {
-      if (operator != Filter.Operator.EQUAL) {
-        throw new IllegalArgumentException(
-            "Invalid Query. You can only perform equality comparisons on null (via "
-                + "whereEqualTo()).");
-      }
-      return new NullFilter(path);
-    } else if (value.equals(DoubleValue.NaN)) {
-      if (operator != Filter.Operator.EQUAL) {
-        throw new IllegalArgumentException(
-            "Invalid Query. You can only perform equality comparisons on NaN (via "
-                + "whereEqualTo()).");
-      }
-      return new NaNFilter(path);
-    } else {
-      return new RelationFilter(path, operator, value);
-    }
-  }
-
   /** Returns the field the Filter operates over. */
   public abstract FieldPath getField();