Fix array-contains queries

packages/firestore/src/api/database.ts

@@ -1515,7 +1515,7 @@ export class Query implements firestore.Query {
         value
       );
     }
-    const filter = Filter.create(fieldPath, operator, fieldValue);
+    const filter = FieldFilter.create(fieldPath, operator, fieldValue);
     this.validateNewFilter(filter);
     return new Query(this._query.addFilter(filter), this.firestore);
   }

packages/firestore/src/core/query.ts

@@ -443,57 +443,6 @@ export abstract class Filter {
   abstract matches(doc: Document): boolean;
   abstract canonicalId(): string;
   abstract isEqual(filter: Filter): boolean;
-
-  /**
-   * Creates a filter based on the provided arguments.
-   */
-  static create(field: FieldPath, op: Operator, value: FieldValue): Filter {
-    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);
-    } else if (value.isEqual(NullValue.INSTANCE)) {
-      if (op !== Operator.EQUAL) {
-        throw new FirestoreError(
-          Code.INVALID_ARGUMENT,
-          'Invalid query. You can only perform equals comparisons on null.'
-        );
-      }
-      return new FieldFilter(field, op, value);
-    } else if (value.isEqual(DoubleValue.NAN)) {
-      if (op !== Operator.EQUAL) {
-        throw new FirestoreError(
-          Code.INVALID_ARGUMENT,
-          'Invalid query. You can only perform equals comparisons on NaN.'
-        );
-      }
-      return new FieldFilter(field, op, value);
-    } else if (op === Operator.ARRAY_CONTAINS) {
-      return new ArrayContainsFilter(field, value);
-    } else if (op === Operator.IN) {
-      assert(
-        value instanceof ArrayValue,
-        'IN filter has invalid value: ' + value.toString()
-      );
-      return new InFilter(field, value as ArrayValue);
-    } else if (op === Operator.ARRAY_CONTAINS_ANY) {
-      assert(
-        value instanceof ArrayValue,
-        'ARRAY_CONTAINS_ANY filter has invalid value: ' + value.toString()
-      );
-      return new ArrayContainsAnyFilter(field, value as ArrayValue);
-    } else {
-      return new FieldFilter(field, op, value);
-    }
-  }
 }
 
 export class Operator {
@@ -541,14 +490,69 @@ export class Operator {
 }
 
 export class FieldFilter extends Filter {
-  constructor(
+  protected constructor(
     public field: FieldPath,
     public op: Operator,
     public value: FieldValue
   ) {
     super();
   }
 
+  /**
+   * Creates a filter based on the provided arguments.
+   */
+  static create(
+    field: FieldPath,
+    op: Operator,
+    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);
+    } else if (value.isEqual(NullValue.INSTANCE)) {
+      if (op !== Operator.EQUAL) {
+        throw new FirestoreError(
+          Code.INVALID_ARGUMENT,
+          'Invalid query. You can only perform equals comparisons on null.'
+        );
+      }
+      return new FieldFilter(field, op, value);
+    } else if (value.isEqual(DoubleValue.NAN)) {
+      if (op !== Operator.EQUAL) {
+        throw new FirestoreError(
+          Code.INVALID_ARGUMENT,
+          'Invalid query. You can only perform equals comparisons on NaN.'
+        );
+      }
+      return new FieldFilter(field, op, value);
+    } else if (op === Operator.ARRAY_CONTAINS) {
+      return new ArrayContainsFilter(field, value);
+    } else if (op === Operator.IN) {
+      assert(
+        value instanceof ArrayValue,
+        'IN filter has invalid value: ' + value.toString()
+      );
+      return new InFilter(field, value as ArrayValue);
+    } else if (op === Operator.ARRAY_CONTAINS_ANY) {
+      assert(
+        value instanceof ArrayValue,
+        'ARRAY_CONTAINS_ANY filter has invalid value: ' + value.toString()
+      );
+      return new ArrayContainsAnyFilter(field, value as ArrayValue);
+    } else {
+      return new FieldFilter(field, op, value);
+    }
+  }
+
   matches(doc: Document): boolean {
     const other = doc.field(this.field);
 

packages/firestore/src/remote/serializer.ts

@@ -1282,7 +1282,7 @@ export class JsonProtoSerializer {
   }
 
   fromFieldFilter(filter: api.Filter): Filter {
-    return new FieldFilter(
+    return FieldFilter.create(
       this.fromFieldPathReference(filter.fieldFilter!.field!),
       this.fromOperatorName(filter.fieldFilter!.op!),
       this.fromValue(filter.fieldFilter!.value!)
@@ -1323,12 +1323,16 @@ export class JsonProtoSerializer {
         const nanField = this.fromFieldPathReference(
           filter.unaryFilter!.field!
         );
-        return new FieldFilter(nanField, Operator.EQUAL, DoubleValue.NAN);
+        return FieldFilter.create(nanField, Operator.EQUAL, DoubleValue.NAN);
       case 'IS_NULL':
         const nullField = this.fromFieldPathReference(
           filter.unaryFilter!.field!
         );
-        return new FieldFilter(nullField, Operator.EQUAL, NullValue.INSTANCE);
+        return FieldFilter.create(
+          nullField,
+          Operator.EQUAL,
+          NullValue.INSTANCE
+        );
       case 'OPERATOR_UNSPECIFIED':
         return fail('Unspecified filter');
       default:

packages/firestore/test/unit/remote/node/serializer.test.ts

@@ -25,8 +25,12 @@ import { GeoPoint } from '../../../../src/api/geo_point';
 import { Timestamp } from '../../../../src/api/timestamp';
 import { DatabaseId } from '../../../../src/core/database_info';
 import {
+  ArrayContainsAnyFilter,
+  ArrayContainsFilter,
   Direction,
   FieldFilter,
+  InFilter,
+  KeyFieldFilter,
   Operator,
   OrderBy,
   Query
@@ -684,7 +688,7 @@ describe('Serializer', () => {
 
     it('makes dotted-property names', () => {
       const path = new FieldPath(['item', 'part', 'top']);
-      const input = new FieldFilter(path, Operator.EQUAL, wrap('food'));
+      const input = FieldFilter.create(path, Operator.EQUAL, wrap('food'));
       const actual = s.toUnaryOrFieldFilter(input);
       expect(actual).to.deep.equal({
         fieldFilter: {
@@ -693,7 +697,9 @@ describe('Serializer', () => {
           value: { stringValue: 'food' }
         }
       });
-      expect(s.fromFieldFilter(actual)).to.deep.equal(input);
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(FieldFilter);
     });
 
     it('converts LessThan', () => {
@@ -706,7 +712,9 @@ describe('Serializer', () => {
           value: { integerValue: '42' }
         }
       });
-      expect(s.fromFieldFilter(actual)).to.deep.equal(input);
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(FieldFilter);
     });
 
     it('converts LessThanOrEqual', () => {
@@ -719,7 +727,9 @@ describe('Serializer', () => {
           value: { stringValue: 'food' }
         }
       });
-      expect(s.fromFieldFilter(actual)).to.deep.equal(input);
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(FieldFilter);
     });
 
     it('converts GreaterThan', () => {
@@ -732,7 +742,9 @@ describe('Serializer', () => {
           value: { booleanValue: false }
         }
       });
-      expect(s.fromFieldFilter(actual)).to.deep.equal(input);
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(FieldFilter);
     });
 
     it('converts GreaterThanOrEqual', () => {
@@ -745,7 +757,31 @@ describe('Serializer', () => {
           value: { doubleValue: 1e100 }
         }
       });
-      expect(s.fromFieldFilter(actual)).to.deep.equal(input);
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(FieldFilter);
+    });
+
+    it('converts key field', () => {
+      const input = filter(
+        DOCUMENT_KEY_NAME,
+        '==',
+        ref('project/database', 'coll/doc')
+      );
+      const actual = s.toUnaryOrFieldFilter(input);
+      expect(actual).to.deep.equal({
+        fieldFilter: {
+          field: { fieldPath: '__name__' },
+          op: 'EQUAL',
+          value: {
+            referenceValue:
+              'projects/project/databases/database/documents/coll/doc'
+          }
+        }
+      });
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(KeyFieldFilter);
     });
 
     it('converts array-contains', () => {
@@ -758,7 +794,9 @@ describe('Serializer', () => {
           value: { integerValue: '42' }
         }
       });
-      expect(s.fromFieldFilter(actual)).to.deep.equal(input);
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(ArrayContainsFilter);
     });
 
     it('converts IN', () => {
@@ -779,7 +817,9 @@ describe('Serializer', () => {
           }
         }
       });
-      expect(s.fromFieldFilter(actual)).to.deep.equal(input);
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(InFilter);
     });
 
     it('converts array-contains-any', () => {
@@ -800,7 +840,9 @@ describe('Serializer', () => {
           }
         }
       });
-      expect(s.fromFieldFilter(actual)).to.deep.equal(input);
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(ArrayContainsAnyFilter);
     });
   });
 

packages/firestore/test/unit/specs/listen_spec.test.ts

@@ -607,6 +607,28 @@ describeSpec('Listens:', [], () => {
       .expectEvents(query, {});
   });
 
+  specTest('Array-contains queries support resuming', [], () => {
+    const query = Query.atPath(path('collection')).addFilter(
+      filter('array', 'array-contains', 42)
+    );
+    const docA = doc('collection/a', 2000, { foo: 'bar', array: [1, 42, 3] });
+    return spec()
+      .withGCEnabled(false)
+      .userListens(query)
+      .watchAcksFull(query, 1000)
+      .expectEvents(query, {})
+      .watchSends({ affects: [query] }, docA)
+      .watchSnapshots(2000, [query], 'resume-token-2000')
+      .watchSnapshots(2000)
+      .expectEvents(query, { added: [docA] })
+      .userUnlistens(query)
+      .watchRemoves(query)
+      .userListens(query, 'resume-token-2000')
+      .expectEvents(query, { added: [docA], fromCache: true })
+      .watchAcksFull(query, 3000)
+      .expectEvents(query, {});
+  });
+
   specTest('Persists global resume tokens on unlisten', [], () => {
     const query = Query.atPath(path('collection'));
     const docA = doc('collection/a', 1000, { key: 'a' });

packages/firestore/test/util/helpers.ts

@@ -30,7 +30,6 @@ import {
   Bound,
   Direction,
   FieldFilter,
-  Filter,
   Operator,
   OrderBy
 } from '../../src/core/query';
@@ -190,7 +189,7 @@ export function blob(...bytes: number[]): Blob {
 export function filter(path: string, op: string, value: unknown): FieldFilter {
   const dataValue = wrap(value);
   const operator = Operator.fromString(op);
-  const filter = Filter.create(field(path), operator, dataValue);
+  const filter = FieldFilter.create(field(path), operator, dataValue);
 
   if (filter instanceof FieldFilter) {
     return filter;