Backport updates from Android (multiple changes) (#1912)

Brian Chen · web-flow · commit 0551030be679 · 2019-06-25T17:17:43.000-07:00
diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts
@@ -6182,20 +6182,20 @@ declare namespace firebase.firestore {
      * Clears the persistent storage. This includes pending writes and cached
      * documents.
      *
-     * Must be called while the firestore instance is not started (after the app is
-     * shutdown or when the app is first initialized). On startup, this method
-     * must be called before other methods (other than settings()). If the
-     * firestore instance is still running, the promise will be rejected with
-     * the error code of `failed-precondition`.
+     * Must be called while the firestore instance is not started (after the app
+     * is shutdown or when the app is first initialized). On startup, this
+     * method must be called before other methods (other than settings()). If
+     * the firestore instance is still running, the promise will be rejected
+     * with the error code of `failed-precondition`.
      *
      * Note: clearPersistence() is primarily intended to help write reliable
-     * tests that use Firestore. It uses the most efficient mechanism possible
-     * for dropping existing data but does not attempt to securely overwrite or
+     * tests that use Cloud Firestore. It uses an efficient mechanism for
+     * dropping existing data but does not attempt to securely overwrite or
      * otherwise make cached data unrecoverable. For applications that are
-     * sensitive to the disclosure of cache data in between user sessions we
-     * strongly recommend not to enable persistence in the first place.
+     * sensitive to the disclosure of cached data in between user sessions, we
+     * strongly recommend not enabling persistence at all.
      *
-     * @return A promise that is resolved once the persistent storage has been
+     * @return A promise that is resolved when the persistent storage is
      * cleared. Otherwise, the promise is rejected with an error.
      */
     clearPersistence(): Promise<void>;
diff --git a/packages/firestore-types/index.d.ts b/packages/firestore-types/index.d.ts
@@ -240,20 +240,20 @@ export class FirebaseFirestore {
    * Clears the persistent storage. This includes pending writes and cached
    * documents.
    *
-   * Must be called while the firestore instance is not started (after the app is
-   * shutdown or when the app is first initialized). On startup, this method
-   * must be called before other methods (other than settings()). If the
-   * firestore instance is still running, the promise will be rejected with
-   * the error code of `failed-precondition`.
+   * Must be called while the firestore instance is not started (after the app
+   * is shutdown or when the app is first initialized). On startup, this
+   * method must be called before other methods (other than settings()). If
+   * the firestore instance is still running, the promise will be rejected
+   * with the error code of `failed-precondition`.
    *
    * Note: clearPersistence() is primarily intended to help write reliable
-   * tests that use Firestore. It uses the most efficient mechanism possible
-   * for dropping existing data but does not attempt to securely overwrite or
+   * tests that use Cloud Firestore. It uses an efficient mechanism for
+   * dropping existing data but does not attempt to securely overwrite or
    * otherwise make cached data unrecoverable. For applications that are
-   * sensitive to the disclosure of cache data in between user sessions we
-   * strongly recommend not to enable persistence in the first place.
+   * sensitive to the disclosure of cached data in between user sessions, we
+   * strongly recommend not enabling persistence at all.
    *
-   * @return A promise that is resolved once the persistent storage has been
+   * @return A promise that is resolved when the persistent storage is
    * cleared. Otherwise, the promise is rejected with an error.
    */
   clearPersistence(): Promise<void>;
diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts
@@ -1428,87 +1428,35 @@ export class Query implements firestore.Query {
       validateStringEnum('Query.where', whereFilterOpEnums, 2, opStr);
     }
 
-    let fieldValue;
+    let fieldValue: FieldValue;
     const fieldPath = fieldPathFromArgument('Query.where', field);
     const operator = Operator.fromString(opStr);
     if (fieldPath.isKeyField()) {
       if (
         operator === Operator.ARRAY_CONTAINS ||
-        operator === Operator.ARRAY_CONTAINS_ANY ||
-        operator === Operator.IN
+        operator === Operator.ARRAY_CONTAINS_ANY
       ) {
         throw new FirestoreError(
           Code.INVALID_ARGUMENT,
           `Invalid Query. You can't perform '${operator.toString()}' ` +
             'queries on FieldPath.documentId().'
         );
-      }
-      if (typeof value === 'string') {
-        if (value === '') {
-          throw new FirestoreError(
-            Code.INVALID_ARGUMENT,
-            'Function Query.where() requires its third parameter to be a ' +
-              'valid document ID if the first parameter is ' +
-              'FieldPath.documentId(), but it was an empty string.'
-          );
+      } else if (operator === Operator.IN) {
+        this.validateDisjunctiveFilterElements(value, operator);
+        const referenceList: FieldValue[] = [];
+        for (const arrayValue of value as FieldValue[]) {
+          referenceList.push(this.parseDocumentIdValue(arrayValue));
         }
-        if (
-          !this._query.isCollectionGroupQuery() &&
-          value.indexOf('/') !== -1
-        ) {
-          throw new FirestoreError(
-            Code.INVALID_ARGUMENT,
-            `Invalid third parameter to Query.where(). When querying a collection by ` +
-              `FieldPath.documentId(), the value provided must be a plain document ID, but ` +
-              `'${value}' contains a slash.`
-          );
-        }
-        const path = this._query.path.child(ResourcePath.fromString(value));
-        if (!DocumentKey.isDocumentKey(path)) {
-          throw new FirestoreError(
-            Code.INVALID_ARGUMENT,
-            `Invalid third parameter to Query.where(). 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 = new RefValue(
-          this.firestore._databaseId,
-          new DocumentKey(path)
-        );
-      } else if (value instanceof DocumentReference) {
-        const ref = value as DocumentReference;
-        fieldValue = new RefValue(this.firestore._databaseId, ref._key);
+        fieldValue = new ArrayValue(referenceList);
       } else {
-        throw new FirestoreError(
-          Code.INVALID_ARGUMENT,
-          `Function Query.where() requires its third parameter to be a ` +
-            `string or a DocumentReference if the first parameter is ` +
-            `FieldPath.documentId(), but it was: ` +
-            `${valueDescription(value)}.`
-        );
+        fieldValue = this.parseDocumentIdValue(value);
       }
     } else {
       if (
         operator === Operator.IN ||
         operator === Operator.ARRAY_CONTAINS_ANY
       ) {
-        if (!Array.isArray(value) || value.length === 0) {
-          throw new FirestoreError(
-            Code.INVALID_ARGUMENT,
-            'Invalid Query. A non-empty array is required for ' +
-              `'${operator.toString()}' filters.`
-          );
-        }
-        if (value.length > 10) {
-          throw new FirestoreError(
-            Code.INVALID_ARGUMENT,
-            `Invalid Query. '${operator.toString()}' filters support a ` +
-              'maximum of 10 elements in the value array.'
-          );
-        }
+        this.validateDisjunctiveFilterElements(value, operator);
       }
       fieldValue = this.firestore._dataConverter.parseQueryValue(
         'Query.where',
@@ -1946,6 +1894,98 @@ export class Query implements firestore.Query {
     );
   }
 
+  /**
+   * 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 parseDocumentIdValue(documentIdValue: unknown): RefValue {
+    if (typeof documentIdValue === 'string') {
+      if (documentIdValue === '') {
+        throw new FirestoreError(
+          Code.INVALID_ARGUMENT,
+          'Function Query.where() requires its third parameter to be a ' +
+            'valid document ID if the first parameter is ' +
+            'FieldPath.documentId(), but it was an empty string.'
+        );
+      }
+      if (
+        !this._query.isCollectionGroupQuery() &&
+        documentIdValue.indexOf('/') !== -1
+      ) {
+        throw new FirestoreError(
+          Code.INVALID_ARGUMENT,
+          `Invalid third parameter to Query.where(). When querying a collection by ` +
+            `FieldPath.documentId(), the value provided must be a plain document ID, but ` +
+            `'${documentIdValue}' contains a slash.`
+        );
+      }
+      const path = this._query.path.child(
+        ResourcePath.fromString(documentIdValue)
+      );
+      if (!DocumentKey.isDocumentKey(path)) {
+        throw new FirestoreError(
+          Code.INVALID_ARGUMENT,
+          `Invalid third parameter to Query.where(). 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 new RefValue(this.firestore._databaseId, new DocumentKey(path));
+    } else if (documentIdValue instanceof DocumentReference) {
+      const ref = documentIdValue as DocumentReference;
+      return new RefValue(this.firestore._databaseId, ref._key);
+    } else {
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
+        `Function Query.where() requires its third parameter to be a ` +
+          `string or a DocumentReference if the first parameter is ` +
+          `FieldPath.documentId(), but it was: ` +
+          `${valueDescription(documentIdValue)}.`
+      );
+    }
+  }
+
+  /**
+   * Validates that the value passed into a disjunctrive filter satisfies all
+   * array requirements.
+   */
+  private validateDisjunctiveFilterElements(
+    value: unknown,
+    operator: Operator
+  ): void {
+    if (!Array.isArray(value) || value.length === 0) {
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
+        'Invalid Query. A non-empty array is required for ' +
+          `'${operator.toString()}' filters.`
+      );
+    }
+    if (value.length > 10) {
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
+        `Invalid Query. '${operator.toString()}' filters support a ` +
+          'maximum of 10 elements in the value array.'
+      );
+    }
+    if (value.indexOf(null) >= 0) {
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
+        `Invalid Query. '${operator.toString()}' filters cannot contain 'null' ` +
+          'in the value array.'
+      );
+    }
+    if (value.filter(element => Number.isNaN(element)).length > 0) {
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
+        `Invalid Query. '${operator.toString()}' filters cannot contain 'NaN' ` +
+          'in the value array.'
+      );
+    }
+  }
+
   private validateNewFilter(filter: Filter): void {
     if (filter instanceof FieldFilter) {
       const arrayOps = [Operator.ARRAY_CONTAINS, Operator.ARRAY_CONTAINS_ANY];
diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts
@@ -507,17 +507,29 @@ export class FieldFilter extends Filter {
     value: FieldValue
   ): FieldFilter {
     if (field.isKeyField()) {
-      assert(
-        value instanceof RefValue,
-        'Comparing on key, but filter value not a RefValue'
-      );
-      assert(
-        op !== Operator.ARRAY_CONTAINS &&
-          op !== Operator.ARRAY_CONTAINS_ANY &&
-          op !== Operator.IN,
-        `'${op.toString()}' queries don't make sense on document keys.`
-      );
-      return new KeyFieldFilter(field, op, value as RefValue);
+      if (op === Operator.IN) {
+        assert(
+          value instanceof ArrayValue,
+          'Comparing on key with IN, but filter value not an ArrayValue'
+        );
+        assert(
+          (value as ArrayValue).internalValue.every(elem => {
+            return elem instanceof RefValue;
+          }),
+          'Comparing on key with IN, but an array value was not a RefValue'
+        );
+        return new KeyFieldInFilter(field, value as ArrayValue);
+      } else {
+        assert(
+          value instanceof RefValue,
+          'Comparing on key, but filter value not a RefValue'
+        );
+        assert(
+          op !== Operator.ARRAY_CONTAINS && op !== Operator.ARRAY_CONTAINS_ANY,
+          `'${op.toString()}' queries don't make sense on document keys.`
+        );
+        return new KeyFieldFilter(field, op, value as RefValue);
+      }
     } else if (value.isEqual(NullValue.INSTANCE)) {
       if (op !== Operator.EQUAL) {
         throw new FirestoreError(
@@ -631,6 +643,20 @@ export class KeyFieldFilter extends FieldFilter {
   }
 }
 
+/** Filter that matches on key fields within an array. */
+export class KeyFieldInFilter extends FieldFilter {
+  constructor(field: FieldPath, public value: ArrayValue) {
+    super(field, Operator.IN, value);
+  }
+
+  matches(doc: Document): boolean {
+    const arrayValue = this.value;
+    return arrayValue.internalValue.some(refValue => {
+      return doc.key.isEqual((refValue as RefValue).key);
+    });
+  }
+}
+
 /** A Filter that implements the array-contains operator. */
 export class ArrayContainsFilter extends FieldFilter {
   constructor(field: FieldPath, value: FieldValue) {
@@ -645,30 +671,29 @@ export class ArrayContainsFilter extends FieldFilter {
 
 /** A Filter that implements the IN operator. */
 export class InFilter extends FieldFilter {
-  constructor(field: FieldPath, value: ArrayValue) {
+  constructor(field: FieldPath, public value: ArrayValue) {
     super(field, Operator.IN, value);
   }
 
   matches(doc: Document): boolean {
-    const arrayValue = this.value as ArrayValue;
+    const arrayValue = this.value;
     const other = doc.field(this.field);
     return other !== undefined && arrayValue.contains(other);
   }
 }
 
 /** A Filter that implements the array-contains-any operator. */
 export class ArrayContainsAnyFilter extends FieldFilter {
-  constructor(field: FieldPath, value: ArrayValue) {
+  constructor(field: FieldPath, public value: ArrayValue) {
     super(field, Operator.ARRAY_CONTAINS_ANY, value);
   }
 
   matches(doc: Document): boolean {
-    const arrayValue = this.value as ArrayValue;
     const other = doc.field(this.field);
     return (
       other instanceof ArrayValue &&
       other.internalValue.some(lhsElem => {
-        return arrayValue.contains(lhsElem);
+        return this.value.contains(lhsElem);
       })
     );
   }
diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts
@@ -585,6 +585,28 @@ apiDescribe('Queries', (persistence: boolean) => {
     }
   );
 
+  (isRunningAgainstEmulator() ? it : it.skip)(
+    'can use IN filters by document ID',
+    async () => {
+      const testDocs = {
+        aa: { key: 'aa' },
+        ab: { key: 'ab' },
+        ba: { key: 'ba' },
+        bb: { key: 'bb' }
+      };
+      await withTestCollection(persistence, testDocs, async coll => {
+        const snapshot = await coll
+          .where(FieldPath.documentId(), inOp, ['aa', 'ab'])
+          .get();
+
+        expect(toDataArray(snapshot)).to.deep.equal([
+          { key: 'aa' },
+          { key: 'ab' }
+        ]);
+      });
+    }
+  );
+
   // TODO(in-queries): Enable browser tests once backend support is ready.
   (isRunningAgainstEmulator() ? it : it.skip)(
     'can use array-contains-any filters',
diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts
