From c3cd471b0d10149f06f2acbc7d7e4044b604d922 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 30 Jun 2020 10:59:26 -0700 Subject: [PATCH] Tree-shakeable FieldFilter (#3252) --- .changeset/nice-seas-type.md | 2 + .../firestore/lite/test/dependencies.json | 92 ++++++++----------- packages/firestore/src/api/database.ts | 2 +- packages/firestore/src/core/query.ts | 61 ++++++------ packages/firestore/src/core/target.ts | 17 ++-- .../test/unit/remote/serializer.helper.ts | 11 ++- packages/firestore/test/util/helpers.ts | 8 +- 7 files changed, 91 insertions(+), 102 deletions(-) create mode 100644 .changeset/nice-seas-type.md diff --git a/.changeset/nice-seas-type.md b/.changeset/nice-seas-type.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/nice-seas-type.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/firestore/lite/test/dependencies.json b/packages/firestore/lite/test/dependencies.json index 7f0182ed57b..df88ad5b687 100644 --- a/packages/firestore/lite/test/dependencies.json +++ b/packages/firestore/lite/test/dependencies.json @@ -73,6 +73,7 @@ "canonifyArray", "canonifyBound", "canonifyByteString", + "canonifyFilter", "canonifyGeoPoint", "canonifyMap", "canonifyOrderBy", @@ -102,6 +103,7 @@ "fieldPathFromArgument", "fieldPathFromArgument$1", "fieldPathFromDotSeparatedString", + "filterEquals", "forEach", "formatJSON", "formatPlural", @@ -155,6 +157,7 @@ "refValue", "registerFirestore", "sortsBeforeDocument", + "stringifyFilter", "stringifyOrderBy", "stringifyTarget", "targetEquals", @@ -235,7 +238,6 @@ "SetMutation", "SnapshotVersion", "StreamBridge", - "Target", "TargetImpl", "Timestamp", "TransformMutation", @@ -246,7 +248,7 @@ ], "variables": [] }, - "sizeInBytes": 99058 + "sizeInBytes": 99162 }, "DocumentReference": { "dependencies": { @@ -591,14 +593,6 @@ "assertUint8ArrayAvailable", "binaryStringFromUint8Array", "blobEquals", - "canonicalId", - "canonifyArray", - "canonifyByteString", - "canonifyGeoPoint", - "canonifyMap", - "canonifyReference", - "canonifyTimestamp", - "canonifyValue", "cast", "compareArrays", "compareBlobs", @@ -751,7 +745,7 @@ ], "variables": [] }, - "sizeInBytes": 87590 + "sizeInBytes": 85476 }, "QueryDocumentSnapshot": { "dependencies": { @@ -1071,7 +1065,7 @@ ], "variables": [] }, - "sizeInBytes": 69580 + "sizeInBytes": 69588 }, "WriteBatch": { "dependencies": { @@ -1226,7 +1220,7 @@ ], "variables": [] }, - "sizeInBytes": 72504 + "sizeInBytes": 72512 }, "addDoc": { "dependencies": { @@ -1244,6 +1238,7 @@ "canonifyArray", "canonifyBound", "canonifyByteString", + "canonifyFilter", "canonifyGeoPoint", "canonifyMap", "canonifyOrderBy", @@ -1275,6 +1270,7 @@ "fieldPathFromArgument", "fieldPathFromArgument$1", "fieldPathFromDotSeparatedString", + "filterEquals", "forEach", "formatJSON", "formatPlural", @@ -1335,6 +1331,7 @@ "registerFirestore", "serverTimestamp", "sortsBeforeDocument", + "stringifyFilter", "stringifyOrderBy", "stringifyTarget", "targetEquals", @@ -1430,7 +1427,6 @@ "SetMutation", "SnapshotVersion", "StreamBridge", - "Target", "TargetImpl", "Timestamp", "TransformMutation", @@ -1442,7 +1438,7 @@ ], "variables": [] }, - "sizeInBytes": 109409 + "sizeInBytes": 109513 }, "arrayRemove": { "dependencies": { @@ -1680,6 +1676,7 @@ "canonifyArray", "canonifyBound", "canonifyByteString", + "canonifyFilter", "canonifyGeoPoint", "canonifyMap", "canonifyOrderBy", @@ -1710,6 +1707,7 @@ "fieldPathFromArgument", "fieldPathFromArgument$1", "fieldPathFromDotSeparatedString", + "filterEquals", "forEach", "formatJSON", "formatPlural", @@ -1763,6 +1761,7 @@ "refValue", "registerFirestore", "sortsBeforeDocument", + "stringifyFilter", "stringifyOrderBy", "stringifyTarget", "targetEquals", @@ -1844,7 +1843,6 @@ "SetMutation", "SnapshotVersion", "StreamBridge", - "Target", "TargetImpl", "Timestamp", "TransformMutation", @@ -1855,7 +1853,7 @@ ], "variables": [] }, - "sizeInBytes": 99686 + "sizeInBytes": 99790 }, "collectionGroup": { "dependencies": { @@ -1871,6 +1869,7 @@ "canonifyArray", "canonifyBound", "canonifyByteString", + "canonifyFilter", "canonifyGeoPoint", "canonifyMap", "canonifyOrderBy", @@ -1901,6 +1900,7 @@ "fieldPathFromArgument", "fieldPathFromArgument$1", "fieldPathFromDotSeparatedString", + "filterEquals", "forEach", "formatJSON", "formatPlural", @@ -1954,6 +1954,7 @@ "refValue", "registerFirestore", "sortsBeforeDocument", + "stringifyFilter", "stringifyOrderBy", "stringifyTarget", "targetEquals", @@ -2033,7 +2034,6 @@ "SetMutation", "SnapshotVersion", "StreamBridge", - "Target", "TargetImpl", "Timestamp", "TransformMutation", @@ -2044,7 +2044,7 @@ ], "variables": [] }, - "sizeInBytes": 99118 + "sizeInBytes": 99222 }, "deleteDoc": { "dependencies": { @@ -2219,6 +2219,7 @@ "canonifyArray", "canonifyBound", "canonifyByteString", + "canonifyFilter", "canonifyGeoPoint", "canonifyMap", "canonifyOrderBy", @@ -2249,6 +2250,7 @@ "fieldPathFromArgument", "fieldPathFromArgument$1", "fieldPathFromDotSeparatedString", + "filterEquals", "forEach", "formatJSON", "formatPlural", @@ -2303,6 +2305,7 @@ "refValue", "registerFirestore", "sortsBeforeDocument", + "stringifyFilter", "stringifyOrderBy", "stringifyTarget", "targetEquals", @@ -2385,7 +2388,6 @@ "SetMutation", "SnapshotVersion", "StreamBridge", - "Target", "TargetImpl", "Timestamp", "TransformMutation", @@ -2396,7 +2398,7 @@ ], "variables": [] }, - "sizeInBytes": 100507 + "sizeInBytes": 100611 }, "documentId": { "dependencies": { @@ -2629,14 +2631,6 @@ "assertUint8ArrayAvailable", "binaryStringFromUint8Array", "blobEquals", - "canonicalId", - "canonifyArray", - "canonifyByteString", - "canonifyGeoPoint", - "canonifyMap", - "canonifyReference", - "canonifyTimestamp", - "canonifyValue", "cast", "compareArrays", "compareBlobs", @@ -2811,7 +2805,7 @@ ], "variables": [] }, - "sizeInBytes": 93349 + "sizeInBytes": 91235 }, "increment": { "dependencies": { @@ -2983,6 +2977,7 @@ "canonifyArray", "canonifyBound", "canonifyByteString", + "canonifyFilter", "canonifyGeoPoint", "canonifyMap", "canonifyOrderBy", @@ -3012,6 +3007,7 @@ "fieldPathFromArgument", "fieldPathFromArgument$1", "fieldPathFromDotSeparatedString", + "filterEquals", "forEach", "formatJSON", "formatPlural", @@ -3066,6 +3062,7 @@ "refValue", "registerFirestore", "sortsBeforeDocument", + "stringifyFilter", "stringifyOrderBy", "stringifyTarget", "targetEquals", @@ -3146,7 +3143,6 @@ "SetMutation", "SnapshotVersion", "StreamBridge", - "Target", "TargetImpl", "Timestamp", "TransformMutation", @@ -3157,7 +3153,7 @@ ], "variables": [] }, - "sizeInBytes": 99401 + "sizeInBytes": 99505 }, "queryEqual": { "dependencies": { @@ -3168,14 +3164,6 @@ "assertUint8ArrayAvailable", "binaryStringFromUint8Array", "blobEquals", - "canonicalId", - "canonifyArray", - "canonifyByteString", - "canonifyGeoPoint", - "canonifyMap", - "canonifyReference", - "canonifyTimestamp", - "canonifyValue", "cast", "compareArrays", "compareBlobs", @@ -3329,7 +3317,7 @@ ], "variables": [] }, - "sizeInBytes": 87794 + "sizeInBytes": 85680 }, "refEqual": { "dependencies": { @@ -3345,6 +3333,7 @@ "canonifyArray", "canonifyBound", "canonifyByteString", + "canonifyFilter", "canonifyGeoPoint", "canonifyMap", "canonifyOrderBy", @@ -3374,6 +3363,7 @@ "fieldPathFromArgument", "fieldPathFromArgument$1", "fieldPathFromDotSeparatedString", + "filterEquals", "forEach", "formatJSON", "formatPlural", @@ -3428,6 +3418,7 @@ "refValue", "registerFirestore", "sortsBeforeDocument", + "stringifyFilter", "stringifyOrderBy", "stringifyTarget", "targetEquals", @@ -3508,7 +3499,6 @@ "SetMutation", "SnapshotVersion", "StreamBridge", - "Target", "TargetImpl", "Timestamp", "TransformMutation", @@ -3519,7 +3509,7 @@ ], "variables": [] }, - "sizeInBytes": 99343 + "sizeInBytes": 99447 }, "runTransaction": { "dependencies": { @@ -3702,7 +3692,7 @@ ], "variables": [] }, - "sizeInBytes": 91955 + "sizeInBytes": 91963 }, "serverTimestamp": { "dependencies": { @@ -3907,7 +3897,7 @@ ], "variables": [] }, - "sizeInBytes": 70793 + "sizeInBytes": 70801 }, "setLogLevel": { "dependencies": { @@ -3963,14 +3953,6 @@ "assertUint8ArrayAvailable", "binaryStringFromUint8Array", "blobEquals", - "canonicalId", - "canonifyArray", - "canonifyByteString", - "canonifyGeoPoint", - "canonifyMap", - "canonifyReference", - "canonifyTimestamp", - "canonifyValue", "cast", "compareArrays", "compareBlobs", @@ -4126,7 +4108,7 @@ ], "variables": [] }, - "sizeInBytes": 88527 + "sizeInBytes": 86413 }, "terminate": { "dependencies": { @@ -4479,6 +4461,6 @@ ], "variables": [] }, - "sizeInBytes": 72584 + "sizeInBytes": 72592 } } \ No newline at end of file diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 13e7306a111..47cab0ec39e 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -1821,7 +1821,7 @@ export class BaseQuery { if (conflictingOp === null && isArrayOp) { conflictingOp = this._query.findFilterOperator(arrayOps); } - if (conflictingOp != null) { + if (conflictingOp !== null) { // We special case when it's a duplicate op to give a slightly clearer error message. if (conflictingOp === filter.op) { throw new FirestoreError( diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index d4c806ac72b..712539139a4 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -471,8 +471,6 @@ export class Query { export abstract class Filter { abstract matches(doc: Document): boolean; - abstract canonicalId(): string; - abstract isEqual(filter: Filter): boolean; } export const enum Operator { @@ -594,35 +592,42 @@ export class FieldFilter extends Filter { ].indexOf(this.op) >= 0 ); } +} - canonicalId(): string { - // TODO(b/29183165): Technically, this won't be unique if two values have - // the same description, such as the int 3 and the string "3". So we should - // add the types in here somehow, too. - return ( - this.field.canonicalString() + - this.op.toString() + - canonicalId(this.value) - ); - } +export function canonifyFilter(filter: Filter): string { + debugAssert( + filter instanceof FieldFilter, + 'canonifyFilter() only supports FieldFilters' + ); + // TODO(b/29183165): Technically, this won't be unique if two values have + // the same description, such as the int 3 and the string "3". So we should + // add the types in here somehow, too. + return ( + filter.field.canonicalString() + + filter.op.toString() + + canonicalId(filter.value) + ); +} - isEqual(other: Filter): boolean { - if (other instanceof FieldFilter) { - return ( - this.op === other.op && - this.field.isEqual(other.field) && - valueEquals(this.value, other.value) - ); - } else { - return false; - } - } +export function filterEquals(f1: Filter, f2: Filter): boolean { + return ( + f1 instanceof FieldFilter && + f2 instanceof FieldFilter && + f1.op === f2.op && + f1.field.isEqual(f2.field) && + valueEquals(f1.value, f2.value) + ); +} - toString(): string { - return `${this.field.canonicalString()} ${this.op} ${canonicalId( - this.value - )}`; - } +/** Returns a debug description for `filter`. */ +export function stringifyFilter(filter: Filter): string { + debugAssert( + filter instanceof FieldFilter, + 'stringifyFilter() only supports FieldFilters' + ); + return `${filter.field.canonicalString()} ${filter.op} ${canonicalId( + filter.value + )}`; } /** Filter that matches on key fields (i.e. '__name__'). */ diff --git a/packages/firestore/src/core/target.ts b/packages/firestore/src/core/target.ts index a24b61b8608..82908f8d58f 100644 --- a/packages/firestore/src/core/target.ts +++ b/packages/firestore/src/core/target.ts @@ -22,11 +22,14 @@ import { Bound, boundEquals, canonifyBound, - canonifyOrderBy, - Filter, + canonifyFilter, + filterEquals, + stringifyFilter, OrderBy, orderByEquals, - stringifyOrderBy + stringifyOrderBy, + canonifyOrderBy, + Filter } from './query'; import { debugCast } from '../util/assert'; @@ -98,7 +101,7 @@ export function canonifyTarget(target: Target): string { canonicalId += '|cg:' + targetImpl.collectionGroup; } canonicalId += '|f:'; - canonicalId += targetImpl.filters.map(f => f.canonicalId()).join(','); + canonicalId += targetImpl.filters.map(f => canonifyFilter(f)).join(','); canonicalId += '|ob:'; canonicalId += targetImpl.orderBy.map(o => canonifyOrderBy(o)).join(','); @@ -125,7 +128,9 @@ export function stringifyTarget(target: Target): string { str += ' collectionGroup=' + target.collectionGroup; } if (target.filters.length > 0) { - str += `, filters: [${target.filters.join(', ')}]`; + str += `, filters: [${target.filters + .map(f => stringifyFilter(f)) + .join(', ')}]`; } if (!isNullOrUndefined(target.limit)) { str += ', limit: ' + target.limit; @@ -164,7 +169,7 @@ export function targetEquals(left: Target, right: Target): boolean { } for (let i = 0; i < left.filters.length; i++) { - if (!left.filters[i].isEqual(right.filters[i])) { + if (!filterEquals(left.filters[i], right.filters[i])) { return false; } } diff --git a/packages/firestore/test/unit/remote/serializer.helper.ts b/packages/firestore/test/unit/remote/serializer.helper.ts index 55e4027c573..859e08c8819 100644 --- a/packages/firestore/test/unit/remote/serializer.helper.ts +++ b/packages/firestore/test/unit/remote/serializer.helper.ts @@ -31,11 +31,12 @@ import { InFilter, KeyFieldFilter, Operator, + filterEquals, OrderBy, Query } from '../../../src/core/query'; import { SnapshotVersion } from '../../../src/core/snapshot_version'; -import { Target } from '../../../src/core/target'; +import { Target, targetEquals, TargetImpl } from '../../../src/core/target'; import { TargetData, TargetPurpose } from '../../../src/local/target_data'; import { DeleteMutation, @@ -136,7 +137,7 @@ export function serializerTest( } describe('converts value', () => { - addEqualityMatcher(); + addEqualityMatcher({ equalsFn: filterEquals, forType: FieldFilter }); /** * Verifies full round-trip of encoding/decoding fieldValue objects: @@ -754,7 +755,7 @@ export function serializerTest( }); describe('to/from FieldFilter', () => { - addEqualityMatcher(); + addEqualityMatcher({ equalsFn: filterEquals, forType: FieldFilter }); it('makes dotted-property names', () => { const path = new FieldPath(['item', 'part', 'top']); @@ -913,7 +914,7 @@ export function serializerTest( }); describe('to/from UnaryFilter', () => { - addEqualityMatcher(); + addEqualityMatcher({ equalsFn: filterEquals, forType: FieldFilter }); it('converts null', () => { const input = filter('field', '==', null); @@ -964,7 +965,7 @@ export function serializerTest( }); describe('toTarget', () => { - addEqualityMatcher(); + addEqualityMatcher({ equalsFn: targetEquals, forType: TargetImpl }); it('converts first-level key queries', () => { const q = Query.atPath(path('docs/1')).toTarget(); diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 410ca4568cd..1aca63f36bd 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -208,13 +208,7 @@ export function blob(...bytes: number[]): Blob { export function filter(path: string, op: string, value: unknown): FieldFilter { const dataValue = wrap(value); const operator = op as Operator; - const filter = FieldFilter.create(field(path), operator, dataValue); - - if (filter instanceof FieldFilter) { - return filter; - } else { - return fail('Unrecognized filter: ' + JSON.stringify(filter)); - } + return FieldFilter.create(field(path), operator, dataValue); } export function setMutation(