Skip to content

Commit 9bb7e70

Browse files
var-constvar-const
authored
Fix array-contains queries (#1913)
#1894 introduced a regression that made persisted queries deserialize incorrectly. Fix the regression and add tests. The root cause is that the refactoring introduced a type hierarchy of classes derived from `FieldFilter`, but serializer always deserializes persisted filters to the base class `FieldFilter`, never to the derived types. This would have affected array-contains queries, in queries, and key field queries. Fixes #1904.
1 parent 95cbd6f commit 9bb7e70

File tree

6 files changed

+138
-67
lines changed

6 files changed

+138
-67
lines changed

‎packages/firestore/src/api/database.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1515,7 +1515,7 @@ export class Query implements firestore.Query {
15151515
value
15161516
);
15171517
}
1518-
const filter = Filter.create(fieldPath, operator, fieldValue);
1518+
const filter = FieldFilter.create(fieldPath, operator, fieldValue);
15191519
this.validateNewFilter(filter);
15201520
return new Query(this._query.addFilter(filter), this.firestore);
15211521
}

‎packages/firestore/src/core/query.ts

Lines changed: 56 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -443,57 +443,6 @@ export abstract class Filter {
443443
abstract matches(doc: Document): boolean;
444444
abstract canonicalId(): string;
445445
abstract isEqual(filter: Filter): boolean;
446-
447-
/**
448-
* Creates a filter based on the provided arguments.
449-
*/
450-
static create(field: FieldPath, op: Operator, value: FieldValue): Filter {
451-
if (field.isKeyField()) {
452-
assert(
453-
value instanceof RefValue,
454-
'Comparing on key, but filter value not a RefValue'
455-
);
456-
assert(
457-
op !== Operator.ARRAY_CONTAINS &&
458-
op !== Operator.ARRAY_CONTAINS_ANY &&
459-
op !== Operator.IN,
460-
`'${op.toString()}' queries don't make sense on document keys.`
461-
);
462-
return new KeyFieldFilter(field, op, value as RefValue);
463-
} else if (value.isEqual(NullValue.INSTANCE)) {
464-
if (op !== Operator.EQUAL) {
465-
throw new FirestoreError(
466-
Code.INVALID_ARGUMENT,
467-
'Invalid query. You can only perform equals comparisons on null.'
468-
);
469-
}
470-
return new FieldFilter(field, op, value);
471-
} else if (value.isEqual(DoubleValue.NAN)) {
472-
if (op !== Operator.EQUAL) {
473-
throw new FirestoreError(
474-
Code.INVALID_ARGUMENT,
475-
'Invalid query. You can only perform equals comparisons on NaN.'
476-
);
477-
}
478-
return new FieldFilter(field, op, value);
479-
} else if (op === Operator.ARRAY_CONTAINS) {
480-
return new ArrayContainsFilter(field, value);
481-
} else if (op === Operator.IN) {
482-
assert(
483-
value instanceof ArrayValue,
484-
'IN filter has invalid value: ' + value.toString()
485-
);
486-
return new InFilter(field, value as ArrayValue);
487-
} else if (op === Operator.ARRAY_CONTAINS_ANY) {
488-
assert(
489-
value instanceof ArrayValue,
490-
'ARRAY_CONTAINS_ANY filter has invalid value: ' + value.toString()
491-
);
492-
return new ArrayContainsAnyFilter(field, value as ArrayValue);
493-
} else {
494-
return new FieldFilter(field, op, value);
495-
}
496-
}
497446
}
498447

499448
export class Operator {
@@ -541,14 +490,69 @@ export class Operator {
541490
}
542491

543492
export class FieldFilter extends Filter {
544-
constructor(
493+
protected constructor(
545494
public field: FieldPath,
546495
public op: Operator,
547496
public value: FieldValue
548497
) {
549498
super();
550499
}
551500

501+
/**
502+
* Creates a filter based on the provided arguments.
503+
*/
504+
static create(
505+
field: FieldPath,
506+
op: Operator,
507+
value: FieldValue
508+
): FieldFilter {
509+
if (field.isKeyField()) {
510+
assert(
511+
value instanceof RefValue,
512+
'Comparing on key, but filter value not a RefValue'
513+
);
514+
assert(
515+
op !== Operator.ARRAY_CONTAINS &&
516+
op !== Operator.ARRAY_CONTAINS_ANY &&
517+
op !== Operator.IN,
518+
`'${op.toString()}' queries don't make sense on document keys.`
519+
);
520+
return new KeyFieldFilter(field, op, value as RefValue);
521+
} else if (value.isEqual(NullValue.INSTANCE)) {
522+
if (op !== Operator.EQUAL) {
523+
throw new FirestoreError(
524+
Code.INVALID_ARGUMENT,
525+
'Invalid query. You can only perform equals comparisons on null.'
526+
);
527+
}
528+
return new FieldFilter(field, op, value);
529+
} else if (value.isEqual(DoubleValue.NAN)) {
530+
if (op !== Operator.EQUAL) {
531+
throw new FirestoreError(
532+
Code.INVALID_ARGUMENT,
533+
'Invalid query. You can only perform equals comparisons on NaN.'
534+
);
535+
}
536+
return new FieldFilter(field, op, value);
537+
} else if (op === Operator.ARRAY_CONTAINS) {
538+
return new ArrayContainsFilter(field, value);
539+
} else if (op === Operator.IN) {
540+
assert(
541+
value instanceof ArrayValue,
542+
'IN filter has invalid value: ' + value.toString()
543+
);
544+
return new InFilter(field, value as ArrayValue);
545+
} else if (op === Operator.ARRAY_CONTAINS_ANY) {
546+
assert(
547+
value instanceof ArrayValue,
548+
'ARRAY_CONTAINS_ANY filter has invalid value: ' + value.toString()
549+
);
550+
return new ArrayContainsAnyFilter(field, value as ArrayValue);
551+
} else {
552+
return new FieldFilter(field, op, value);
553+
}
554+
}
555+
552556
matches(doc: Document): boolean {
553557
const other = doc.field(this.field);
554558

‎packages/firestore/src/remote/serializer.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1282,7 +1282,7 @@ export class JsonProtoSerializer {
12821282
}
12831283

12841284
fromFieldFilter(filter: api.Filter): Filter {
1285-
return new FieldFilter(
1285+
return FieldFilter.create(
12861286
this.fromFieldPathReference(filter.fieldFilter!.field!),
12871287
this.fromOperatorName(filter.fieldFilter!.op!),
12881288
this.fromValue(filter.fieldFilter!.value!)
@@ -1323,12 +1323,16 @@ export class JsonProtoSerializer {
13231323
const nanField = this.fromFieldPathReference(
13241324
filter.unaryFilter!.field!
13251325
);
1326-
return new FieldFilter(nanField, Operator.EQUAL, DoubleValue.NAN);
1326+
return FieldFilter.create(nanField, Operator.EQUAL, DoubleValue.NAN);
13271327
case 'IS_NULL':
13281328
const nullField = this.fromFieldPathReference(
13291329
filter.unaryFilter!.field!
13301330
);
1331-
return new FieldFilter(nullField, Operator.EQUAL, NullValue.INSTANCE);
1331+
return FieldFilter.create(
1332+
nullField,
1333+
Operator.EQUAL,
1334+
NullValue.INSTANCE
1335+
);
13321336
case 'OPERATOR_UNSPECIFIED':
13331337
return fail('Unspecified filter');
13341338
default:

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

Lines changed: 51 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,12 @@ import { GeoPoint } from '../../../../src/api/geo_point';
2525
import { Timestamp } from '../../../../src/api/timestamp';
2626
import { DatabaseId } from '../../../../src/core/database_info';
2727
import {
28+
ArrayContainsAnyFilter,
29+
ArrayContainsFilter,
2830
Direction,
2931
FieldFilter,
32+
InFilter,
33+
KeyFieldFilter,
3034
Operator,
3135
OrderBy,
3236
Query
@@ -684,7 +688,7 @@ describe('Serializer', () => {
684688

685689
it('makes dotted-property names', () => {
686690
const path = new FieldPath(['item', 'part', 'top']);
687-
const input = new FieldFilter(path, Operator.EQUAL, wrap('food'));
691+
const input = FieldFilter.create(path, Operator.EQUAL, wrap('food'));
688692
const actual = s.toUnaryOrFieldFilter(input);
689693
expect(actual).to.deep.equal({
690694
fieldFilter: {
@@ -693,7 +697,9 @@ describe('Serializer', () => {
693697
value: { stringValue: 'food' }
694698
}
695699
});
696-
expect(s.fromFieldFilter(actual)).to.deep.equal(input);
700+
const roundtripped = s.fromFieldFilter(actual);
701+
expect(roundtripped).to.deep.equal(input);
702+
expect(roundtripped).to.be.instanceof(FieldFilter);
697703
});
698704

699705
it('converts LessThan', () => {
@@ -706,7 +712,9 @@ describe('Serializer', () => {
706712
value: { integerValue: '42' }
707713
}
708714
});
709-
expect(s.fromFieldFilter(actual)).to.deep.equal(input);
715+
const roundtripped = s.fromFieldFilter(actual);
716+
expect(roundtripped).to.deep.equal(input);
717+
expect(roundtripped).to.be.instanceof(FieldFilter);
710718
});
711719

712720
it('converts LessThanOrEqual', () => {
@@ -719,7 +727,9 @@ describe('Serializer', () => {
719727
value: { stringValue: 'food' }
720728
}
721729
});
722-
expect(s.fromFieldFilter(actual)).to.deep.equal(input);
730+
const roundtripped = s.fromFieldFilter(actual);
731+
expect(roundtripped).to.deep.equal(input);
732+
expect(roundtripped).to.be.instanceof(FieldFilter);
723733
});
724734

725735
it('converts GreaterThan', () => {
@@ -732,7 +742,9 @@ describe('Serializer', () => {
732742
value: { booleanValue: false }
733743
}
734744
});
735-
expect(s.fromFieldFilter(actual)).to.deep.equal(input);
745+
const roundtripped = s.fromFieldFilter(actual);
746+
expect(roundtripped).to.deep.equal(input);
747+
expect(roundtripped).to.be.instanceof(FieldFilter);
736748
});
737749

738750
it('converts GreaterThanOrEqual', () => {
@@ -745,7 +757,31 @@ describe('Serializer', () => {
745757
value: { doubleValue: 1e100 }
746758
}
747759
});
748-
expect(s.fromFieldFilter(actual)).to.deep.equal(input);
760+
const roundtripped = s.fromFieldFilter(actual);
761+
expect(roundtripped).to.deep.equal(input);
762+
expect(roundtripped).to.be.instanceof(FieldFilter);
763+
});
764+
765+
it('converts key field', () => {
766+
const input = filter(
767+
DOCUMENT_KEY_NAME,
768+
'==',
769+
ref('project/database', 'coll/doc')
770+
);
771+
const actual = s.toUnaryOrFieldFilter(input);
772+
expect(actual).to.deep.equal({
773+
fieldFilter: {
774+
field: { fieldPath: '__name__' },
775+
op: 'EQUAL',
776+
value: {
777+
referenceValue:
778+
'projects/project/databases/database/documents/coll/doc'
779+
}
780+
}
781+
});
782+
const roundtripped = s.fromFieldFilter(actual);
783+
expect(roundtripped).to.deep.equal(input);
784+
expect(roundtripped).to.be.instanceof(KeyFieldFilter);
749785
});
750786

751787
it('converts array-contains', () => {
@@ -758,7 +794,9 @@ describe('Serializer', () => {
758794
value: { integerValue: '42' }
759795
}
760796
});
761-
expect(s.fromFieldFilter(actual)).to.deep.equal(input);
797+
const roundtripped = s.fromFieldFilter(actual);
798+
expect(roundtripped).to.deep.equal(input);
799+
expect(roundtripped).to.be.instanceof(ArrayContainsFilter);
762800
});
763801

764802
it('converts IN', () => {
@@ -779,7 +817,9 @@ describe('Serializer', () => {
779817
}
780818
}
781819
});
782-
expect(s.fromFieldFilter(actual)).to.deep.equal(input);
820+
const roundtripped = s.fromFieldFilter(actual);
821+
expect(roundtripped).to.deep.equal(input);
822+
expect(roundtripped).to.be.instanceof(InFilter);
783823
});
784824

785825
it('converts array-contains-any', () => {
@@ -800,7 +840,9 @@ describe('Serializer', () => {
800840
}
801841
}
802842
});
803-
expect(s.fromFieldFilter(actual)).to.deep.equal(input);
843+
const roundtripped = s.fromFieldFilter(actual);
844+
expect(roundtripped).to.deep.equal(input);
845+
expect(roundtripped).to.be.instanceof(ArrayContainsAnyFilter);
804846
});
805847
});
806848

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,28 @@ describeSpec('Listens:', [], () => {
607607
.expectEvents(query, {});
608608
});
609609

610+
specTest('Array-contains queries support resuming', [], () => {
611+
const query = Query.atPath(path('collection')).addFilter(
612+
filter('array', 'array-contains', 42)
613+
);
614+
const docA = doc('collection/a', 2000, { foo: 'bar', array: [1, 42, 3] });
615+
return spec()
616+
.withGCEnabled(false)
617+
.userListens(query)
618+
.watchAcksFull(query, 1000)
619+
.expectEvents(query, {})
620+
.watchSends({ affects: [query] }, docA)
621+
.watchSnapshots(2000, [query], 'resume-token-2000')
622+
.watchSnapshots(2000)
623+
.expectEvents(query, { added: [docA] })
624+
.userUnlistens(query)
625+
.watchRemoves(query)
626+
.userListens(query, 'resume-token-2000')
627+
.expectEvents(query, { added: [docA], fromCache: true })
628+
.watchAcksFull(query, 3000)
629+
.expectEvents(query, {});
630+
});
631+
610632
specTest('Persists global resume tokens on unlisten', [], () => {
611633
const query = Query.atPath(path('collection'));
612634
const docA = doc('collection/a', 1000, { key: 'a' });

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import {
3030
Bound,
3131
Direction,
3232
FieldFilter,
33-
Filter,
3433
Operator,
3534
OrderBy
3635
} from '../../src/core/query';
@@ -190,7 +189,7 @@ export function blob(...bytes: number[]): Blob {
190189
export function filter(path: string, op: string, value: unknown): FieldFilter {
191190
const dataValue = wrap(value);
192191
const operator = Operator.fromString(op);
193-
const filter = Filter.create(field(path), operator, dataValue);
192+
const filter = FieldFilter.create(field(path), operator, dataValue);
194193

195194
if (filter instanceof FieldFilter) {
196195
return filter;

0 commit comments

Comments
 (0)