Allow IN queries with arrays of documentIds (#560)

Brian Chen · web-flow · commit 1eff91632b1b · 2019-06-24T14:54:03.000-07:00
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
@@ -701,11 +701,6 @@ 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 "
@@ -726,6 +721,38 @@ public void queriesFilteredByDocumentIDMustUseStringsOrDocumentReferences() {
         () -> collection.whereArrayContainsAny(FieldPath.documentId(), asList(1, 2)), reason);
   }
 
+  @Test
+  public void queriesUsingInAndDocumentIdMustHaveProperDocumentReferencesInArray() {
+    CollectionReference collection = testCollection();
+    collection.whereIn(FieldPath.documentId(), asList(collection.getPath()));
+
+    String reason =
+        "Invalid query. When querying with FieldPath.documentId() you must provide "
+            + "a valid document ID, but it was an empty string.";
+    expectError(() -> collection.whereIn(FieldPath.documentId(), asList("")), reason);
+
+    reason =
+        "Invalid query. When querying a collection by FieldPath.documentId() you must provide "
+            + "a plain document ID, but 'foo/bar/baz' contains a '/' character.";
+    expectError(() -> collection.whereIn(FieldPath.documentId(), asList("foo/bar/baz")), reason);
+
+    reason =
+        "Invalid query. When querying with FieldPath.documentId() you must provide "
+            + "a valid String or DocumentReference, but it was of type: java.lang.Integer";
+    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 "
+            + "an odd number of segments (1).";
+    expectError(
+        () ->
+            testFirestore()
+                .collectionGroup("collection")
+                .whereIn(FieldPath.documentId(), asList("foo")),
+        reason);
+  }
+
   // Helpers
 
   /** Performs a write using each write API and makes sure it fails with the expected 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
@@ -39,6 +39,7 @@
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.ResourcePath;
+import com.google.firebase.firestore.model.value.ArrayValue;
 import com.google.firebase.firestore.model.value.FieldValue;
 import com.google.firebase.firestore.model.value.ReferenceValue;
 import com.google.firebase.firestore.model.value.ServerTimestampValue;
@@ -84,76 +85,6 @@ public FirebaseFirestore getFirestore() {
     return firestore;
   }
 
-  private void validateOrderByFieldMatchesInequality(
-      com.google.firebase.firestore.model.FieldPath orderBy,
-      com.google.firebase.firestore.model.FieldPath inequality) {
-    if (!orderBy.equals(inequality)) {
-      String inequalityString = inequality.canonicalString();
-      throw new IllegalArgumentException(
-          String.format(
-              "Invalid query. You have an inequality where filter (whereLessThan(), "
-                  + "whereGreaterThan(), etc.) on field '%s' and so you must also have '%s' as "
-                  + "your first orderBy() field, but your first orderBy() is currently on field "
-                  + "'%s' instead.",
-              inequalityString, inequalityString, orderBy.canonicalString()));
-    }
-  }
-
-  private void validateNewFilter(Filter filter) {
-    if (filter instanceof RelationFilter) {
-      Operator filterOp = ((RelationFilter) filter).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()) {
-        com.google.firebase.firestore.model.FieldPath existingInequality = query.inequalityField();
-        com.google.firebase.firestore.model.FieldPath newInequality = filter.getField();
-
-        if (existingInequality != null && !existingInequality.equals(newInequality)) {
-          throw new IllegalArgumentException(
-              String.format(
-                  "All where filters other than whereEqualTo() must be on the same field. But you "
-                      + "have filters on '%s' and '%s'",
-                  existingInequality.canonicalString(), newInequality.canonicalString()));
-        }
-        com.google.firebase.firestore.model.FieldPath firstOrderByField =
-            query.getFirstOrderByField();
-        if (firstOrderByField != null) {
-          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.
-        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.");
-          }
-        }
-      }
-    }
-  }
-
   /**
    * Creates and returns a new Query with the additional filter that documents must contain the
    * specified field and the value should be equal to the specified value.
@@ -421,66 +352,19 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu
             "Invalid query. You can't perform '"
                 + op.toString()
                 + "' queries on FieldPath.documentId().");
-      }
-      if (value instanceof String) {
-        String documentKey = (String) value;
-        if (documentKey.isEmpty()) {
-          throw new IllegalArgumentException(
-              "Invalid query. When querying with FieldPath.documentId() you must provide a valid "
-                  + "document ID, but it was an empty string.");
+      } else if (op == Operator.IN) {
+        validateDisjunctiveFilterElements(value, op);
+        List<FieldValue> referenceList = new ArrayList<>();
+        for (Object arrayValue : (List) value) {
+          referenceList.add(parseDocumentIdValue(arrayValue));
         }
-        if (!query.isCollectionGroupQuery() && documentKey.contains("/")) {
-          throw new IllegalArgumentException(
-              "Invalid query. When querying a collection by FieldPath.documentId() you must "
-                  + "provide a plain document ID, but '"
-                  + documentKey
-                  + "' contains a '/' character.");
-        }
-        ResourcePath path = query.getPath().append(ResourcePath.fromString(documentKey));
-        if (!DocumentKey.isDocumentKey(path)) {
-          throw new IllegalArgumentException(
-              "Invalid query. When querying a collection group by FieldPath.documentId(), the "
-                  + "value provided must result in a valid document path, but '"
-                  + path
-                  + "' is not because it has an odd number of segments ("
-                  + path.length()
-                  + ").");
-        }
-        fieldValue =
-            ReferenceValue.valueOf(this.getFirestore().getDatabaseId(), DocumentKey.fromPath(path));
-      } else if (value instanceof DocumentReference) {
-        DocumentReference ref = (DocumentReference) value;
-        fieldValue = ReferenceValue.valueOf(this.getFirestore().getDatabaseId(), ref.getKey());
+        fieldValue = ArrayValue.fromList(referenceList);
       } else {
-        throw new IllegalArgumentException(
-            "Invalid query. When querying with FieldPath.documentId() you must provide a valid "
-                + "String or DocumentReference, but it was of type: "
-                + Util.typeName(value));
+        fieldValue = parseDocumentIdValue(value);
       }
     } else {
       if (op == Operator.IN || op == Operator.ARRAY_CONTAINS_ANY) {
-        if (!(value instanceof List) || ((List) value).size() == 0) {
-          throw new IllegalArgumentException(
-              "Invalid Query. A non-empty array is required for '" + op.toString() + "' filters.");
-        }
-        if (((List) value).size() > 10) {
-          throw new IllegalArgumentException(
-              "Invalid Query. '"
-                  + 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.");
-        }
+        validateDisjunctiveFilterElements(value, op);
       }
       fieldValue = firestore.getDataConverter().parseQueryValue(value);
     }
@@ -497,6 +381,144 @@ private void validateOrderByField(com.google.firebase.firestore.model.FieldPath
     }
   }
 
+  /**
+   * Parses the given documentIdValue into a ReferenceValue, throwing appropriate errors if the
+   * value is anything other than a DocumentReference or String, or if the string is malformed.
+   */
+  private ReferenceValue parseDocumentIdValue(Object documentIdValue) {
+    if (documentIdValue instanceof String) {
+      String documentId = (String) documentIdValue;
+      if (documentId.isEmpty()) {
+        throw new IllegalArgumentException(
+            "Invalid query. When querying with FieldPath.documentId() you must provide a valid "
+                + "document ID, but it was an empty string.");
+      }
+      if (!query.isCollectionGroupQuery() && documentId.contains("/")) {
+        throw new IllegalArgumentException(
+            "Invalid query. When querying a collection by FieldPath.documentId() you must "
+                + "provide a plain document ID, but '"
+                + documentId
+                + "' contains a '/' character.");
+      }
+      ResourcePath path = query.getPath().append(ResourcePath.fromString(documentId));
+      if (!DocumentKey.isDocumentKey(path)) {
+        throw new IllegalArgumentException(
+            "Invalid query. When querying a collection group by FieldPath.documentId(), the "
+                + "value provided must result in a valid document path, but '"
+                + path
+                + "' is not because it has an odd number of segments ("
+                + path.length()
+                + ").");
+      }
+      return ReferenceValue.valueOf(
+          this.getFirestore().getDatabaseId(), DocumentKey.fromPath(path));
+    } else if (documentIdValue instanceof DocumentReference) {
+      DocumentReference ref = (DocumentReference) documentIdValue;
+      return ReferenceValue.valueOf(this.getFirestore().getDatabaseId(), ref.getKey());
+    } else {
+      throw new IllegalArgumentException(
+          "Invalid query. When querying with FieldPath.documentId() you must provide a valid "
+              + "String or DocumentReference, but it was of type: "
+              + Util.typeName(documentIdValue));
+    }
+  }
+
+  /** Validates that the value passed into a disjunctive filter satisfies all array requirements. */
+  private void validateDisjunctiveFilterElements(Object value, Operator op) {
+    if (!(value instanceof List) || ((List) value).size() == 0) {
+      throw new IllegalArgumentException(
+          "Invalid Query. A non-empty array is required for '" + op.toString() + "' filters.");
+    }
+    if (((List) value).size() > 10) {
+      throw new IllegalArgumentException(
+          "Invalid Query. '"
+              + 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.");
+    }
+  }
+
+  private void validateOrderByFieldMatchesInequality(
+      com.google.firebase.firestore.model.FieldPath orderBy,
+      com.google.firebase.firestore.model.FieldPath inequality) {
+    if (!orderBy.equals(inequality)) {
+      String inequalityString = inequality.canonicalString();
+      throw new IllegalArgumentException(
+          String.format(
+              "Invalid query. You have an inequality where filter (whereLessThan(), "
+                  + "whereGreaterThan(), etc.) on field '%s' and so you must also have '%s' as "
+                  + "your first orderBy() field, but your first orderBy() is currently on field "
+                  + "'%s' instead.",
+              inequalityString, inequalityString, orderBy.canonicalString()));
+    }
+  }
+
+  private void validateNewFilter(Filter filter) {
+    if (filter instanceof RelationFilter) {
+      Operator filterOp = ((RelationFilter) filter).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()) {
+        com.google.firebase.firestore.model.FieldPath existingInequality = query.inequalityField();
+        com.google.firebase.firestore.model.FieldPath newInequality = filter.getField();
+
+        if (existingInequality != null && !existingInequality.equals(newInequality)) {
+          throw new IllegalArgumentException(
+              String.format(
+                  "All where filters other than whereEqualTo() must be on the same field. But you "
+                      + "have filters on '%s' and '%s'",
+                  existingInequality.canonicalString(), newInequality.canonicalString()));
+        }
+        com.google.firebase.firestore.model.FieldPath firstOrderByField =
+            query.getFirstOrderByField();
+        if (firstOrderByField != null) {
+          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.
+        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.");
+          }
+        }
+      }
+    }
+  }
+
   /**
    * Creates and returns a new Query that's additionally sorted by the specified field.
    *
