resolve comments

Author: Brian Chen

packages/firestore/src/api/database.ts

@@ -1667,7 +1667,7 @@ function parseDocumentIdValue(
 }
 
 /**
- * Validates that the value passed into a disjunctrive filter satisfies all
+ * Validates that the value passed into a disjunctive filter satisfies all
  * array requirements.
  */
 function validateDisjunctiveFilterElements(
@@ -1688,45 +1688,58 @@ function validateDisjunctiveFilterElements(
         'maximum of 10 elements in the value array.'
     );
   }
-  if (value.indexOf(null) >= 0 && operator !== Operator.NOT_IN) {
-    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 &&
-    operator !== Operator.NOT_IN
-  ) {
-    throw new FirestoreError(
-      Code.INVALID_ARGUMENT,
-      `Invalid Query. '${operator.toString()}' filters cannot contain 'NaN' ` +
-        'in the value array.'
-    );
+  if (operator === Operator.IN || operator === Operator.ARRAY_CONTAINS_ANY) {
+    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.'
+      );
+    }
   }
 }
 
+/**
+ * Given an operator, returns the set of operators that cannot be used with it.
+ *
+ * Array operators: ARRAY_CONTAINS, ARRAY_CONTAINS_ANY
+ * Disjunctive operators: IN, ARRAY_CONTAINS_ANY, NOT_IN
+ */
 function conflictingOps(op: Operator): Operator[] {
   switch (op) {
     case Operator.NOT_EQUAL:
-      return [Operator.NOT_IN, Operator.NOT_EQUAL];
+      // No other NOT_EQUAL or NOT_IN operator can be used with NOT_EQUAL.
+      return [Operator.NOT_EQUAL, Operator.NOT_IN];
     case Operator.ARRAY_CONTAINS:
       return [
+        // Only 1 ARRAY operator can be used.
         Operator.ARRAY_CONTAINS,
         Operator.ARRAY_CONTAINS_ANY,
+        // NOT_IN cannot be used with any array operators.
         Operator.NOT_IN
       ];
     case Operator.IN:
+      // Only one disjunctive operator can be used.
       return [Operator.ARRAY_CONTAINS_ANY, Operator.IN, Operator.NOT_IN];
     case Operator.ARRAY_CONTAINS_ANY:
       return [
+        // Only one ARRAY operator can be used.
         Operator.ARRAY_CONTAINS,
+        // Only one disjunctive operator can be used.
         Operator.ARRAY_CONTAINS_ANY,
         Operator.IN,
         Operator.NOT_IN
       ];
     case Operator.NOT_IN:
+      // NOT_IN cannot be used with another array, disjunctive, or NOT_EQUAL
+      // operator.
       return [
         Operator.ARRAY_CONTAINS,
         Operator.ARRAY_CONTAINS_ANY,
@@ -1762,7 +1775,7 @@ function validateNewFilter(query: InternalQuery, filter: Filter): void {
 
   const conflictingOp = query.findFilterOperator(conflictingOps(filter.op));
   if (conflictingOp !== null) {
-    // We special case when it's a duplicate op to give a slightly clearer error message.
+    // Special case when it's a duplicate op to give a slightly clearer error message.
     if (conflictingOp === filter.op) {
       throw new FirestoreError(
         Code.INVALID_ARGUMENT,

packages/firestore/src/core/query.ts

@@ -647,13 +647,9 @@ export class FieldFilter extends Filter {
 
   private static createKeyFieldInFilter(
     field: FieldPath,
-    op: Operator,
+    op: Operator.IN | Operator.NOT_IN,
     value: api.Value
   ): FieldFilter {
-    debugAssert(
-      op === Operator.IN || op === Operator.NOT_IN,
-      'createKeyFieldInFilter requires an IN or NOT_IN operator'
-    );
     debugAssert(
       isArray(value),
       `Comparing on key with ${op.toString()}` +
@@ -784,14 +780,7 @@ export class KeyFieldInFilter extends FieldFilter {
 
   constructor(field: FieldPath, value: api.Value) {
     super(field, Operator.IN, value);
-    debugAssert(isArray(value), 'KeyFieldInFilter expects an ArrayValue');
-    this.keys = (value.arrayValue.values || []).map(v => {
-      debugAssert(
-        isReferenceValue(v),
-        'Comparing on key with IN, but an array value was not a ReferenceValue'
-      );
-      return DocumentKey.fromName(v.referenceValue);
-    });
+    this.keys = extractDocumentKeysFromArrayValue(Operator.IN, value);
   }
 
   matches(doc: Document): boolean {
@@ -805,21 +794,32 @@ export class KeyFieldNotInFilter extends FieldFilter {
 
   constructor(field: FieldPath, value: api.Value) {
     super(field, Operator.NOT_IN, value);
-    debugAssert(isArray(value), 'KeyFieldNotInFilter expects an ArrayValue');
-    this.keys = (value.arrayValue.values || []).map(v => {
-      debugAssert(
-        isReferenceValue(v),
-        'Comparing on key with NOT_IN, but an array value was not a ReferenceValue'
-      );
-      return DocumentKey.fromName(v.referenceValue);
-    });
+    this.keys = extractDocumentKeysFromArrayValue(Operator.NOT_IN, value);
   }
 
   matches(doc: Document): boolean {
     return !this.keys.some(key => key.isEqual(doc.key));
   }
 }
 
+function extractDocumentKeysFromArrayValue(
+  op: Operator.IN | Operator.NOT_IN,
+  value: api.Value
+): DocumentKey[] {
+  debugAssert(
+    isArray(value),
+    'KeyFieldInFilter/KeyFieldNotInFilter expects an ArrayValue'
+  );
+  return (value.arrayValue?.values || []).map(v => {
+    debugAssert(
+      isReferenceValue(v),
+      `Comparing on key with ${op.toString()}, but an array value was not ` +
+        `a ReferenceValue`
+    );
+    return DocumentKey.fromName(v.referenceValue);
+  });
+}
+
 /** A Filter that implements the array-contains operator. */
 export class ArrayContainsFilter extends FieldFilter {
   constructor(field: FieldPath, value: api.Value) {

packages/firestore/test/integration/api/validation.test.ts

@@ -1216,14 +1216,14 @@ apiDescribe('Validation:', (persistence: boolean) => {
       );
     });
 
-    validationIt(persistence, 'with != and NOT_IN filters fail', db => {
+    validationIt(persistence, 'with != and not-in filters fail', db => {
       expect(() =>
         db
           .collection('test')
           .where('foo', notInOp, [2, 3])
           .where('foo', notEqualOp, 4)
       ).to.throw(
-        "Invalid query. You cannot use '!=' filters with " + "'not-in' filters."
+        "Invalid query. You cannot use '!=' filters with 'not-in' filters."
       );
 
       expect(() =>
@@ -1232,7 +1232,7 @@ apiDescribe('Validation:', (persistence: boolean) => {
           .where('foo', notEqualOp, 4)
           .where('foo', notInOp, [2, 3])
       ).to.throw(
-        "Invalid query. You cannot use 'not-in' filters with " + "'!=' filters."
+        "Invalid query. You cannot use 'not-in' filters with '!=' filters."
       );
     });
 
@@ -1309,7 +1309,7 @@ apiDescribe('Validation:', (persistence: boolean) => {
           .where('foo', notInOp, [2, 3])
           .where('foo', 'in', [2, 3])
       ).to.throw(
-        "Invalid query. You cannot use 'in' filters with " + "'not-in' filters."
+        "Invalid query. You cannot use 'in' filters with 'not-in' filters."
       );
 
       expect(() =>
@@ -1318,7 +1318,7 @@ apiDescribe('Validation:', (persistence: boolean) => {
           .where('foo', 'in', [2, 3])
           .where('foo', notInOp, [2, 3])
       ).to.throw(
-        "Invalid query. You cannot use 'not-in' filters with " + "'in' filters."
+        "Invalid query. You cannot use 'not-in' filters with 'in' filters."
       );
 
       // This is redundant with the above tests, but makes sure our validation
@@ -1330,8 +1330,7 @@ apiDescribe('Validation:', (persistence: boolean) => {
           .where('foo', 'array-contains', 1)
           .where('foo', 'array-contains-any', [2])
       ).to.throw(
-        "Invalid query. You cannot use 'array-contains-any' filters with " +
-          "'in' filters."
+        "Invalid query. You cannot use 'array-contains-any' filters with 'in' filters."
       );
 
       expect(() =>

packages/firestore/test/integration/util/helpers.ts

@@ -279,8 +279,8 @@ export function withTestCollectionSettings(
 }
 
 // TODO(ne-queries): This exists just so we don't have to do the cast
-// repeatedly. Once we expose '!=' publicly we can remove it and
-// just use 'array-contains-any' in all the tests.
+// repeatedly. Once we expose '!=' publicly we can remove it and just use '!='
+// in all the tests.
 export const notEqualOp = '!=' as firestore.WhereFilterOp;
 
 // TODO(ne-queries): This exists just so we don't have to do the cast

packages/firestore/test/unit/core/query.test.ts

@@ -218,7 +218,7 @@ describe('Query', () => {
     expect(queryMatches(query1, document)).to.be.true;
   });
 
-  it('matches NOT_IN filters', () => {
+  it('matches not-in filters', () => {
     const query1 = query('collection', filter('zip', 'not-in', [12345]));
 
     // No match.
@@ -260,7 +260,7 @@ describe('Query', () => {
     expect(queryMatches(query1, document)).to.be.false;
   });
 
-  it('matches NOT_IN filters with object values', () => {
+  it('matches not-in filters with object values', () => {
     const query1 = query('collection', filter('zip', 'not-in', [{ a: [42] }]));
 
     // Containing object in array.