diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md index 649adc916ca..2c8b907908c 100644 --- a/packages/firestore/CHANGELOG.md +++ b/packages/firestore/CHANGELOG.md @@ -1,4 +1,3 @@ - # Unreleased (1.5.0) - [feature] Added a `Firestore.waitForPendingWrites()` method that allows users to wait until all pending writes are acknowledged by the @@ -7,6 +6,8 @@ the instance, releasing any held resources. Once it completes, you can optionally call `Firestore.clearPersistence()` to wipe persisted Firestore data from disk. +- [changed] Improved performance for queries with filters that only return a + small subset of the documents in a collection. # 1.4.10 - [changed] Transactions now perform exponential backoff before retrying. @@ -25,6 +26,7 @@ match the query (https://github.com/firebase/firebase-android-sdk/issues/155). # 1.4.4 +>>>>>>> master - [fixed] Fixed an internal assertion that was triggered when an update with a `FieldValue.serverTimestamp()` and an update with a `FieldValue.increment()` were pending for the same document. diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 22d88c051ca..3f2fc233117 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -1276,7 +1276,7 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot { return !this._document ? undefined : this.convertObject( - this._document.data, + this._document.data(), FieldValueOptions.fromSnapshotOptions( options, this._firestore._areTimestampsInSnapshotsEnabled() @@ -1291,9 +1291,9 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot { validateBetweenNumberOfArgs('DocumentSnapshot.get', arguments, 1, 2); options = validateSnapshotOptions('DocumentSnapshot.get', options); if (this._document) { - const value = this._document.data.field( - fieldPathFromArgument('DocumentSnapshot.get', fieldPath) - ); + const value = this._document + .data() + .field(fieldPathFromArgument('DocumentSnapshot.get', fieldPath)); if (value !== null) { return this.convertValue( value, diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index 295551aa0b7..37187efcaf0 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -759,7 +759,7 @@ export class Bound { } else { const docValue = doc.field(orderByComponent.field); assert( - docValue !== undefined, + docValue !== null, 'Field should exist since document matched the orderBy already.' ); comparison = component.compareTo(docValue!); diff --git a/packages/firestore/src/core/view.ts b/packages/firestore/src/core/view.ts index c57b2231341..1a3df0457d3 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -169,7 +169,7 @@ export class View { // Calculate change if (oldDoc && newDoc) { - const docsEqual = oldDoc.data.isEqual(newDoc.data); + const docsEqual = oldDoc.data().isEqual(newDoc.data()); if (!docsEqual) { if (!this.shouldWaitForSyncedDocument(oldDoc, newDoc)) { changeSet.track({ diff --git a/packages/firestore/src/model/document.ts b/packages/firestore/src/model/document.ts index 822002b246a..f04759ddb6e 100644 --- a/packages/firestore/src/model/document.ts +++ b/packages/firestore/src/model/document.ts @@ -16,13 +16,14 @@ */ import { SnapshotVersion } from '../core/snapshot_version'; -import { fail } from '../util/assert'; +import { assert, fail } from '../util/assert'; import { DocumentKey } from './document_key'; import { FieldValue, JsonObject, ObjectValue } from './field_value'; import { FieldPath } from './path'; import * as api from '../protos/firestore_proto_api'; +import * as obj from '../util/obj'; export interface DocumentOptions { hasLocalMutations?: boolean; @@ -59,33 +60,81 @@ export class Document extends MaybeDocument { readonly hasLocalMutations: boolean; readonly hasCommittedMutations: boolean; + /** + * A cache of canonicalized FieldPaths to FieldValues that have already been + * deserialized in `getField()`. + */ + private fieldValueCache?: Map; + constructor( key: DocumentKey, version: SnapshotVersion, - readonly data: ObjectValue, options: DocumentOptions, - /** - * Memoized serialized form of the document for optimization purposes (avoids repeated - * serialization). Might be undefined. - */ - readonly proto?: api.Document + private objectValue?: ObjectValue, + readonly proto?: api.Document, + private readonly converter?: (value: api.Value) => FieldValue ) { super(key, version); + assert( + this.objectValue !== undefined || + (this.proto !== undefined && this.converter !== undefined), + 'If objectValue is not defined, proto and converter need to be set.' + ); + this.hasLocalMutations = !!options.hasLocalMutations; this.hasCommittedMutations = !!options.hasCommittedMutations; } field(path: FieldPath): FieldValue | null { - return this.data.field(path); + if (this.objectValue) { + return this.objectValue.field(path); + } else { + if (!this.fieldValueCache) { + // TODO(b/136090445): Remove the cache when `getField` is no longer + // called during Query ordering. + this.fieldValueCache = new Map(); + } + + const canonicalPath = path.canonicalString(); + + let fieldValue = this.fieldValueCache.get(canonicalPath); + + if (fieldValue === undefined) { + // Instead of deserializing the full Document proto, we only + // deserialize the value at the requested field path. This speeds up + // Query execution as query filters can discard documents based on a + // single field. + const protoValue = this.getProtoField(path); + if (protoValue === undefined) { + fieldValue = null; + } else { + fieldValue = this.converter!(protoValue); + } + this.fieldValueCache.set(canonicalPath, fieldValue); + } + + return fieldValue!; + } } - fieldValue(path: FieldPath): unknown { - const field = this.field(path); - return field ? field.value() : undefined; + data(): ObjectValue { + if (!this.objectValue) { + let result = ObjectValue.EMPTY; + obj.forEach(this.proto!.fields, (key: string, value: api.Value) => { + result = result.set(new FieldPath([key]), this.converter!(value)); + }); + this.objectValue = result; + + // Once objectValue is computed, values inside the fieldValueCache are no + // longer accessed. + this.fieldValueCache = undefined; + } + + return this.objectValue; } value(): JsonObject { - return this.data.value(); + return this.data().value(); } isEqual(other: MaybeDocument | null | undefined): boolean { @@ -93,9 +142,9 @@ export class Document extends MaybeDocument { other instanceof Document && this.key.isEqual(other.key) && this.version.isEqual(other.version) && - this.data.isEqual(other.data) && this.hasLocalMutations === other.hasLocalMutations && - this.hasCommittedMutations === other.hasCommittedMutations + this.hasCommittedMutations === other.hasCommittedMutations && + this.data().isEqual(other.data()) ); } @@ -111,6 +160,29 @@ export class Document extends MaybeDocument { return this.hasLocalMutations || this.hasCommittedMutations; } + /** + * Returns the nested Protobuf value for 'path`. Can only be called if + * `proto` was provided at construction time. + */ + private getProtoField(path: FieldPath): api.Value | undefined { + assert( + this.proto !== undefined, + 'Can only call getProtoField() when proto is defined' + ); + + let protoValue: api.Value | undefined = this.proto!.fields[ + path.firstSegment() + ]; + for (let i = 1; i < path.length; ++i) { + if (!protoValue || !protoValue.mapValue) { + return undefined; + } + protoValue = protoValue.mapValue.fields[path.get(i)]; + } + + return protoValue; + } + static compareByField(field: FieldPath, d1: Document, d2: Document): number { const v1 = d1.field(field); const v2 = d2.field(field); diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts index e0aedb3b334..92a752cf45c 100644 --- a/packages/firestore/src/model/mutation.ts +++ b/packages/firestore/src/model/mutation.ts @@ -353,9 +353,14 @@ export class SetMutation extends Mutation { // have held. const version = mutationResult.version; - return new Document(this.key, version, this.value, { - hasCommittedMutations: true - }); + return new Document( + this.key, + version, + { + hasCommittedMutations: true + }, + this.value + ); } applyToLocalView( @@ -370,9 +375,14 @@ export class SetMutation extends Mutation { } const version = Mutation.getPostMutationVersion(maybeDoc); - return new Document(this.key, version, this.value, { - hasLocalMutations: true - }); + return new Document( + this.key, + version, + { + hasLocalMutations: true + }, + this.value + ); } extractBaseValue(maybeDoc: MaybeDocument | null): null { @@ -434,9 +444,14 @@ export class PatchMutation extends Mutation { } const newData = this.patchDocument(maybeDoc); - return new Document(this.key, mutationResult.version, newData, { - hasCommittedMutations: true - }); + return new Document( + this.key, + mutationResult.version, + { + hasCommittedMutations: true + }, + newData + ); } applyToLocalView( @@ -452,9 +467,14 @@ export class PatchMutation extends Mutation { const version = Mutation.getPostMutationVersion(maybeDoc); const newData = this.patchDocument(maybeDoc); - return new Document(this.key, version, newData, { - hasLocalMutations: true - }); + return new Document( + this.key, + version, + { + hasLocalMutations: true + }, + newData + ); } extractBaseValue(maybeDoc: MaybeDocument | null): null { @@ -478,7 +498,7 @@ export class PatchMutation extends Mutation { private patchDocument(maybeDoc: MaybeDocument | null): ObjectValue { let data: ObjectValue; if (maybeDoc instanceof Document) { - data = maybeDoc.data; + data = maybeDoc.data(); } else { data = ObjectValue.EMPTY; } @@ -550,10 +570,15 @@ export class TransformMutation extends Mutation { ); const version = mutationResult.version; - const newData = this.transformObject(doc.data, transformResults); - return new Document(this.key, version, newData, { - hasCommittedMutations: true - }); + const newData = this.transformObject(doc.data(), transformResults); + return new Document( + this.key, + version, + { + hasCommittedMutations: true + }, + newData + ); } applyToLocalView( @@ -573,10 +598,15 @@ export class TransformMutation extends Mutation { maybeDoc, baseDoc ); - const newData = this.transformObject(doc.data, transformResults); - return new Document(this.key, doc.version, newData, { - hasLocalMutations: true - }); + const newData = this.transformObject(doc.data(), transformResults); + return new Document( + this.key, + doc.version, + { + hasLocalMutations: true + }, + newData + ); } extractBaseValue(maybeDoc: MaybeDocument | null): ObjectValue | null { diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 0d5aa0e89b7..0018c65cd24 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -519,7 +519,7 @@ export class JsonProtoSerializer { ); return { name: this.toName(document.key), - fields: this.toFields(document.data), + fields: this.toFields(document.data()), updateTime: this.toTimestamp(document.version.toTimestamp()) }; } @@ -528,11 +528,15 @@ export class JsonProtoSerializer { document: api.Document, hasCommittedMutations?: boolean ): Document { + const key = this.fromName(document.name!); + const version = this.fromVersion(document.updateTime!); return new Document( - this.fromName(document.name!), - this.fromVersion(document.updateTime!), - this.fromFields(document.fields || {}), - { hasCommittedMutations: !!hasCommittedMutations } + key, + version, + { hasCommittedMutations: !!hasCommittedMutations }, + undefined, + document, + v => this.fromValue(v) ); } @@ -577,8 +581,9 @@ export class JsonProtoSerializer { assertPresent(doc.found!.updateTime, 'doc.found.updateTime'); const key = this.fromName(doc.found!.name!); const version = this.fromVersion(doc.found!.updateTime!); - const fields = this.fromFields(doc.found!.fields || {}); - return new Document(key, version, fields, {}, doc.found!); + return new Document(key, version, {}, undefined, doc.found!, v => + this.fromValue(v) + ); } private fromMissing(result: api.BatchGetDocumentsResponse): NoDocument { @@ -639,7 +644,7 @@ export class JsonProtoSerializer { documentChange: { document: { name: this.toName(doc.key), - fields: this.toFields(doc.data), + fields: this.toFields(doc.data()), updateTime: this.toVersion(doc.version) }, targetIds: watchChange.updatedTargetIds, @@ -718,15 +723,13 @@ export class JsonProtoSerializer { const entityChange = change.documentChange!; const key = this.fromName(entityChange.document!.name!); const version = this.fromVersion(entityChange.document!.updateTime!); - const fields = this.fromFields(entityChange.document!.fields || {}); - // The document may soon be re-serialized back to protos in order to store it in local - // persistence. Memoize the encoded form to avoid encoding it again. const doc = new Document( key, version, - fields, {}, - entityChange.document! + undefined, + entityChange.document!, + v => this.fromValue(v) ); const updatedTargetIds = entityChange.targetIds || []; const removedTargetIds = entityChange.removedTargetIds || []; diff --git a/packages/firestore/test/integration/remote/remote.test.ts b/packages/firestore/test/integration/remote/remote.test.ts index f4581bec536..a305f8cb4bd 100644 --- a/packages/firestore/test/integration/remote/remote.test.ts +++ b/packages/firestore/test/integration/remote/remote.test.ts @@ -59,7 +59,7 @@ describe('Remote Storage', () => { const doc = docs[0]; expect(doc).to.be.an.instanceof(Document); if (doc instanceof Document) { - expect(doc.data).to.deep.equal(mutation.value); + expect(doc.data()).to.deep.equal(mutation.value); expect(doc.key).to.deep.equal(k); expect(SnapshotVersion.MIN.compareTo(doc.version)).to.be.lessThan( 0 diff --git a/packages/firestore/test/unit/local/lru_garbage_collector.test.ts b/packages/firestore/test/unit/local/lru_garbage_collector.test.ts index 693956f5444..51305abfc87 100644 --- a/packages/firestore/test/unit/local/lru_garbage_collector.test.ts +++ b/packages/firestore/test/unit/local/lru_garbage_collector.test.ts @@ -243,8 +243,8 @@ function genericLruGarbageCollectorTests( return new Document( key, SnapshotVersion.fromMicroseconds(1000), - wrapObject({ foo: 3, bar: false }), - {} + {}, + wrapObject({ foo: 3, bar: false }) ); } @@ -774,8 +774,8 @@ function genericLruGarbageCollectorTests( const doc = new Document( middleDocToUpdate, SnapshotVersion.fromMicroseconds(2000), - wrapObject({ foo: 4, bar: true }), - {} + {}, + wrapObject({ foo: 4, bar: true }) ); return saveDocument(txn, doc).next(() => { return updateTargetInTransaction(txn, middleTarget); diff --git a/packages/firestore/test/unit/model/document.test.ts b/packages/firestore/test/unit/model/document.test.ts index 967925d6202..f42b74a33fb 100644 --- a/packages/firestore/test/unit/model/document.test.ts +++ b/packages/firestore/test/unit/model/document.test.ts @@ -42,10 +42,10 @@ describe('Document', () => { }; const document = doc('rooms/Eros', 1, data, { hasLocalMutations: true }); - expect(document.fieldValue(field('desc'))).to.deep.equal( + expect(document.field(field('desc'))!.value()).to.deep.equal( 'Discuss all the project related stuff' ); - expect(document.fieldValue(field('owner.title'))).to.deep.equal( + expect(document.field(field('owner.title'))!.value()).to.deep.equal( 'scallywag' ); expect(document.hasLocalMutations).to.equal(true); diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts index 657d7dd2ba6..e601e226212 100644 --- a/packages/firestore/test/unit/model/mutation.test.ts +++ b/packages/firestore/test/unit/model/mutation.test.ts @@ -199,9 +199,14 @@ describe('Mutation', () => { foo: { bar: '' }, baz: 'baz-value' }).set(field('foo.bar'), new ServerTimestampValue(timestamp, null)); - const expectedDoc = new Document(key('collection/key'), version(0), data, { - hasLocalMutations: true - }); + const expectedDoc = new Document( + key('collection/key'), + version(0), + { + hasLocalMutations: true + }, + data + ); expect(transformedDoc).to.deep.equal(expectedDoc); }); diff --git a/packages/firestore/test/unit/remote/node/serializer.test.ts b/packages/firestore/test/unit/remote/node/serializer.test.ts index 75cbc6cec52..d8b13dc4dd2 100644 --- a/packages/firestore/test/unit/remote/node/serializer.test.ts +++ b/packages/firestore/test/unit/remote/node/serializer.test.ts @@ -675,7 +675,7 @@ describe('Serializer', () => { const d = doc('foo/bar', 42, { a: 5, b: 'b' }); const proto = { name: s.toName(d.key), - fields: s.toFields(d.data), + fields: s.toFields(d.data()), updateTime: s.toVersion(d.version) }; const serialized = s.toDocument(d); diff --git a/packages/firestore/test/unit/specs/query_spec.test.ts b/packages/firestore/test/unit/specs/query_spec.test.ts index 53f05c65425..5722a916a33 100644 --- a/packages/firestore/test/unit/specs/query_spec.test.ts +++ b/packages/firestore/test/unit/specs/query_spec.test.ts @@ -82,9 +82,9 @@ describeSpec('Queries:', [], () => { ); return specWithCachedDocs(...cachedDocs) - .userSets(toWrite1.key.toString(), toWrite1.data.value()) - .userSets(toWrite2.key.toString(), toWrite2.data.value()) - .userSets(toWrite3.key.toString(), toWrite3.data.value()) + .userSets(toWrite1.key.toString(), toWrite1.data().value()) + .userSets(toWrite2.key.toString(), toWrite2.data().value()) + .userSets(toWrite3.key.toString(), toWrite3.data().value()) .userListens(cgQuery) .expectEvents(cgQuery, { added: [cachedDocs[0], toWrite1, toWrite2], diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 8365ada6ab2..fd3056564cf 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -882,7 +882,7 @@ export class SpecBuilder { return { key: SpecBuilder.keyToSpec(doc.key), version: doc.version.toMicroseconds(), - value: doc.data.value(), + value: doc.data().value(), options: { hasLocalMutations: doc.hasLocalMutations, hasCommittedMutations: doc.hasCommittedMutations diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 7b25c3e9215..1813ff3267b 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -122,7 +122,7 @@ export function doc( json: JsonObject, options: DocumentOptions = {} ): Document { - return new Document(key(keyStr), version(ver), wrapObject(json), options); + return new Document(key(keyStr), version(ver), options, wrapObject(json)); } export function deletedDoc(