From 7080047a4b80ec125a7f29ccbe46d18bba052a1d Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Thu, 10 Nov 2022 15:28:01 -0800 Subject: [PATCH 1/9] Add createTime to Document. --- packages/firestore/src/model/document.ts | 30 ++++++++++++++----- packages/firestore/src/remote/serializer.ts | 28 ++++++++++++++--- .../test/unit/local/local_store.test.ts | 27 +++++++++++++++++ .../test/unit/remote/serializer.helper.ts | 5 ++-- packages/firestore/test/util/helpers.ts | 4 ++- 5 files changed, 79 insertions(+), 15 deletions(-) diff --git a/packages/firestore/src/model/document.ts b/packages/firestore/src/model/document.ts index 279f5031371..421b74ad2b9 100644 --- a/packages/firestore/src/model/document.ts +++ b/packages/firestore/src/model/document.ts @@ -101,6 +101,13 @@ export interface Document { */ readonly readTime: SnapshotVersion; + /** + * The timestamp at which the document was created. This value increases + * monotonically when a document is deleted then recreated. It can also be + * compared to `createTime` of other documents and the `readTime` of a query. + */ + readonly createTime: SnapshotVersion; + /** The underlying data of this document or an empty value if no data exists. */ readonly data: ObjectValue; @@ -163,6 +170,7 @@ export class MutableDocument implements Document { private documentType: DocumentType, public version: SnapshotVersion, public readTime: SnapshotVersion, + public createTime: SnapshotVersion, public data: ObjectValue, private documentState: DocumentState ) {} @@ -175,8 +183,9 @@ export class MutableDocument implements Document { return new MutableDocument( documentKey, DocumentType.INVALID, - SnapshotVersion.min(), - SnapshotVersion.min(), + /* version */ SnapshotVersion.min(), + /* readTime */ SnapshotVersion.min(), + /* createTime */ SnapshotVersion.min(), ObjectValue.empty(), DocumentState.SYNCED ); @@ -189,13 +198,15 @@ export class MutableDocument implements Document { static newFoundDocument( documentKey: DocumentKey, version: SnapshotVersion, + createTime: SnapshotVersion, value: ObjectValue ): MutableDocument { return new MutableDocument( documentKey, DocumentType.FOUND_DOCUMENT, - version, - SnapshotVersion.min(), + /* version */ version, + /* readTime */ SnapshotVersion.min(), + /* createTime */ createTime, value, DocumentState.SYNCED ); @@ -209,8 +220,9 @@ export class MutableDocument implements Document { return new MutableDocument( documentKey, DocumentType.NO_DOCUMENT, - version, - SnapshotVersion.min(), + /* version */ version, + /* readTime */ SnapshotVersion.min(), + /* createTime */ SnapshotVersion.min(), ObjectValue.empty(), DocumentState.SYNCED ); @@ -228,8 +240,9 @@ export class MutableDocument implements Document { return new MutableDocument( documentKey, DocumentType.UNKNOWN_DOCUMENT, - version, - SnapshotVersion.min(), + /* version */ version, + /* readTime */ SnapshotVersion.min(), + /* createTime */ SnapshotVersion.min(), ObjectValue.empty(), DocumentState.HAS_COMMITTED_MUTATIONS ); @@ -340,6 +353,7 @@ export class MutableDocument implements Document { this.documentType, this.version, this.readTime, + this.createTime, this.data.clone(), this.documentState ); diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 21091d55a36..001f3bb77f1 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -395,7 +395,8 @@ export function toDocument( return { name: toName(serializer, document.key), fields: document.data.value.mapValue.fields, - updateTime: toTimestamp(serializer, document.version.toTimestamp()) + updateTime: toTimestamp(serializer, document.version.toTimestamp()), + createTime: toTimestamp(serializer, document.createTime.toTimestamp()) }; } @@ -406,8 +407,16 @@ export function fromDocument( ): MutableDocument { const key = fromName(serializer, document.name!); const version = fromVersion(document.updateTime!); + const createTime = document.createTime + ? fromVersion(document.createTime) + : SnapshotVersion.min(); const data = new ObjectValue({ mapValue: { fields: document.fields } }); - const result = MutableDocument.newFoundDocument(key, version, data); + const result = MutableDocument.newFoundDocument( + key, + version, + createTime, + data + ); if (hasCommittedMutations) { result.setHasCommittedMutations(); } @@ -426,8 +435,11 @@ function fromFound( assertPresent(doc.found.updateTime, 'doc.found.updateTime'); const key = fromName(serializer, doc.found.name); const version = fromVersion(doc.found.updateTime); + const createTime = doc.found.createTime + ? fromVersion(doc.found.createTime) + : SnapshotVersion.min(); const data = new ObjectValue({ mapValue: { fields: doc.found.fields } }); - return MutableDocument.newFoundDocument(key, version, data); + return MutableDocument.newFoundDocument(key, version, createTime, data); } function fromMissing( @@ -493,10 +505,18 @@ export function fromWatchChange( ); const key = fromName(serializer, entityChange.document.name); const version = fromVersion(entityChange.document.updateTime); + const createTime = entityChange.document.createTime + ? fromVersion(entityChange.document.createTime) + : SnapshotVersion.min(); const data = new ObjectValue({ mapValue: { fields: entityChange.document.fields } }); - const doc = MutableDocument.newFoundDocument(key, version, data); + const doc = MutableDocument.newFoundDocument( + key, + version, + createTime, + data + ); const updatedTargetIds = entityChange.targetIds || []; const removedTargetIds = entityChange.removedTargetIds || []; watchChange = new DocumentWatchChange( diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 9e14e553edf..26ea248e856 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -1958,6 +1958,33 @@ function genericLocalStoreTests( .finish(); }); + it('handles document creation time', () => { + return ( + expectLocalStore() + .afterAllocatingQuery(query('col')) + .toReturnTargetId(2) + .after(docAddedRemoteEvent(doc('col/doc1', 12, { foo: 'bar' }, 5), [2])) + .toReturnChanged(doc('col/doc1', 12, { foo: 'bar' }, 5)) + .toContain(doc('col/doc1', 12, { foo: 'bar' }, 5)) + .after(setMutation('col/doc1', { foo: 'newBar' })) + .toReturnChanged( + doc('col/doc1', 12, { foo: 'newBar' }, 5).setHasLocalMutations() + ) + .toContain( + doc('col/doc1', 12, { foo: 'newBar' }, 5).setHasLocalMutations() + ) + .afterAcknowledgingMutation({ documentVersion: 13 }) + // We haven't seen the remote event yet + .toReturnChanged( + doc('col/doc1', 13, { foo: 'newBar' }, 5).setHasCommittedMutations() + ) + .toContain( + doc('col/doc1', 13, { foo: 'newBar' }, 5).setHasCommittedMutations() + ) + .finish() + ); + }); + it('uses target mapping to execute queries', () => { if (gcIsEager) { return; diff --git a/packages/firestore/test/unit/remote/serializer.helper.ts b/packages/firestore/test/unit/remote/serializer.helper.ts index 7f9bb3a78b9..d097ae627b9 100644 --- a/packages/firestore/test/unit/remote/serializer.helper.ts +++ b/packages/firestore/test/unit/remote/serializer.helper.ts @@ -799,11 +799,12 @@ export function serializerTest( }); it('toDocument() / fromDocument', () => { - const d = doc('foo/bar', 42, { a: 5, b: 'b' }); + const d = doc('foo/bar', 42, { a: 5, b: 'b' }, /* createTime */ 12); const proto = { name: toName(s, d.key), fields: d.data.value.mapValue.fields, - updateTime: toVersion(s, d.version) + updateTime: toVersion(s, d.version), + createTime: toVersion(s, d.createTime) }; const serialized = toDocument(s, d); expect(serialized).to.deep.equal(proto); diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index de06453f685..5dc01b1a241 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -155,11 +155,13 @@ export function ref(key: string, offset?: number): DocumentReference { export function doc( keyStr: string, ver: TestSnapshotVersion, - jsonOrObjectValue: JsonObject | ObjectValue + jsonOrObjectValue: JsonObject | ObjectValue, + createTime?: TestSnapshotVersion ): MutableDocument { return MutableDocument.newFoundDocument( key(keyStr), version(ver), + createTime ? version(createTime) : SnapshotVersion.min(), jsonOrObjectValue instanceof ObjectValue ? jsonOrObjectValue : wrapObject(jsonOrObjectValue) From 97b2957d08f8420308cc7147bdd09a9426ae2d02 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Fri, 11 Nov 2022 13:12:21 -0500 Subject: [PATCH 2/9] More createTime tests --- packages/firestore/src/model/document.ts | 9 +++ .../test/unit/local/local_store.test.ts | 56 ++++++++++++++++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/model/document.ts b/packages/firestore/src/model/document.ts index 421b74ad2b9..3255e2c169a 100644 --- a/packages/firestore/src/model/document.ts +++ b/packages/firestore/src/model/document.ts @@ -256,6 +256,13 @@ export class MutableDocument implements Document { version: SnapshotVersion, value: ObjectValue ): MutableDocument { + if ( + SnapshotVersion.min().isEqual(this.createTime) && + (this.documentType === DocumentType.NO_DOCUMENT || + this.documentType === DocumentType.INVALID) + ) { + this.createTime = version; + } this.version = version; this.documentType = DocumentType.FOUND_DOCUMENT; this.data = value; @@ -340,6 +347,7 @@ export class MutableDocument implements Document { return ( other instanceof MutableDocument && this.key.isEqual(other.key) && + this.createTime.isEqual(other.createTime) && this.version.isEqual(other.version) && this.documentType === other.documentType && this.documentState === other.documentState && @@ -364,6 +372,7 @@ export class MutableDocument implements Document { `Document(${this.key}, ${this.version}, ${JSON.stringify( this.data.value )}, ` + + `{createTime: ${this.createTime}}), ` + `{documentType: ${this.documentType}}), ` + `{documentState: ${this.documentState}})` ); diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 26ea248e856..9237a9f7e55 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -616,10 +616,10 @@ function genericLocalStoreTests( .toContain(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 1 }) .toReturnChanged( - doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations() + doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations() ) .toNotContainIfEager( - doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations() + doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations() ) .finish(); }); @@ -1985,6 +1985,58 @@ function genericLocalStoreTests( ); }); + it('saves updateTime as createTime when creating new doc', () => { + if (gcIsEager) { + return; + } + + return ( + expectLocalStore() + .afterAllocatingQuery(query('col')) + .toReturnTargetId(2) + .after(docAddedRemoteEvent(deletedDoc('col/doc1', 12), [2])) + .toReturnChanged(deletedDoc('col/doc1', 12)) + .toContain(deletedDoc('col/doc1', 12)) + .after(setMutation('col/doc1', { foo: 'newBar' })) + .toReturnChanged( + doc('col/doc1', 12, { foo: 'newBar' }, 12).setHasLocalMutations() + ) + // TODO(COUNT): Below has createTime=0 due to an optimization that uses invalid doc as base doc for + // set mutations. This is OK because it has no impact on aggregation's heuristic logic. But it feels + // "wrong" to have createTime 0 here. We should revisit this. + .toContain( + doc('col/doc1', 12, { foo: 'newBar' }, 0).setHasLocalMutations() + ) + .afterAcknowledgingMutation({ documentVersion: 13 }) + // We haven't seen the remote event yet + .toReturnChanged( + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() + ) + .toContain( + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() + ) + .finish() + ); + }); + + it('saves updateTime as createTime when creating new doc', () => { + if (gcIsEager) { + return; + } + + return expectLocalStore() + .after(setMutation('col/doc1', { foo: 'newBar' })) + .afterAcknowledgingMutation({ documentVersion: 13 }) + .afterExecutingQuery(query('col')) + .toReturnChanged( + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() + ) + .toContain( + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() + ) + .finish(); + }); + it('uses target mapping to execute queries', () => { if (gcIsEager) { return; From 467ed0ef0f75ba3997cf32ccacb42060296e6c01 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Mon, 21 Nov 2022 15:53:51 -0800 Subject: [PATCH 3/9] All tests pass except for Bundles tests. --- .../local/indexeddb_remote_document_cache.ts | 17 +++ .../src/local/local_documents_view.ts | 8 +- packages/firestore/src/model/document.ts | 17 ++- packages/firestore/src/remote/serializer.ts | 3 + .../test/unit/local/local_store.test.ts | 130 ++++++++++-------- .../test/unit/model/mutation.test.ts | 12 +- .../test/unit/specs/bundle_spec.test.ts | 2 +- .../test/unit/specs/limbo_spec.test.ts | 8 +- .../test/unit/specs/listen_spec.test.ts | 4 +- .../test/unit/specs/persistence_spec.test.ts | 4 +- .../firestore/test/unit/specs/spec_builder.ts | 2 + .../test/unit/specs/spec_test_runner.ts | 6 +- .../test/unit/specs/write_spec.test.ts | 8 +- .../firestore/test/util/spec_test_helpers.ts | 3 +- 14 files changed, 138 insertions(+), 86 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index 19b7c566322..7895732525e 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -517,6 +517,14 @@ class IndexedDbRemoteDocumentChangeBuffer extends RemoteDocumentChangeBuffer { size: getResult.size, readTime: getResult.document.readTime }); + + // This is because older SDK versions did not store createTime. + // This can be removed in the long term. + if (!getResult.document.createTime) { + throw('this will never get executed'); + //getResult.document.createTime = SnapshotVersion.min(); + } + return getResult.document; }); } @@ -539,6 +547,15 @@ class IndexedDbRemoteDocumentChangeBuffer extends RemoteDocumentChangeBuffer { readTime: documents.get(documentKey)!.readTime }); }); + + // This is because older SDK versions did not store createTime. + // This can be removed in the long term. + documents.forEach((key, doc) => { + if(!doc.createTime) { + throw('this will never get executed'); + //doc.createTime = SnapshotVersion.min(); + } + }); return documents; }); } diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 2765ee42946..1a1670ecca6 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -557,8 +557,10 @@ export class LocalDocumentsView { key: DocumentKey, overlay: Overlay | null ): PersistencePromise { - return overlay === null || overlay.mutation.type === MutationType.Patch - ? this.remoteDocumentCache.getEntry(transaction, key) - : PersistencePromise.resolve(MutableDocument.newInvalidDocument(key)); + return this.remoteDocumentCache.getEntry(transaction, key); + // TODO(COUNT): ROI of this is pretty low and it could be quite confusing. + // return overlay === null || overlay.mutation.type === MutationType.Patch + // ? this.remoteDocumentCache.getEntry(transaction, key) + // : PersistencePromise.resolve(MutableDocument.newInvalidDocument(key)); } } diff --git a/packages/firestore/src/model/document.ts b/packages/firestore/src/model/document.ts index 3255e2c169a..9843115e271 100644 --- a/packages/firestore/src/model/document.ts +++ b/packages/firestore/src/model/document.ts @@ -256,11 +256,13 @@ export class MutableDocument implements Document { version: SnapshotVersion, value: ObjectValue ): MutableDocument { - if ( - SnapshotVersion.min().isEqual(this.createTime) && - (this.documentType === DocumentType.NO_DOCUMENT || - this.documentType === DocumentType.INVALID) - ) { + // TODO(COUNT): Add comment about why we're updating createTime here. + if (this.createTime.isEqual(SnapshotVersion.min())&& + (this.documentType === DocumentType.NO_DOCUMENT || this.documentType === DocumentType.INVALID)) { + this.createTime = version; + } + if (!this.createTime) { + throw("this will never get executed"); this.createTime = version; } this.version = version; @@ -315,6 +317,11 @@ export class MutableDocument implements Document { return this; } + setCreateTime(createTime: SnapshotVersion): MutableDocument { + this.createTime = createTime; + return this; + } + get hasLocalMutations(): boolean { return this.documentState === DocumentState.HAS_LOCAL_MUTATIONS; } diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 001f3bb77f1..e0259925a90 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -407,6 +407,9 @@ export function fromDocument( ): MutableDocument { const key = fromName(serializer, document.name!); const version = fromVersion(document.updateTime!); + // If we read a document from persistence that is missing createTime, it's due + // to older SDK versions not storing this information. In such cases, we'll + // set the createTime to zero. This can be removed in the long term. const createTime = document.createTime ? fromVersion(document.createTime) : SnapshotVersion.min(); diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 9237a9f7e55..77cadf8ac2b 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -556,29 +556,29 @@ describe('LocalStore w/ Memory Persistence', () => { genericLocalStoreTests(initialize, /* gcIsEager= */ true); }); -describe('LocalStore w/ IndexedDB Persistence', () => { - if (!IndexedDbPersistence.isAvailable()) { - console.warn( - 'No IndexedDB. Skipping LocalStore w/ IndexedDB persistence tests.' - ); - return; - } - - async function initialize(): Promise { - const queryEngine = new CountingQueryEngine(); - const persistence = await persistenceHelpers.testIndexedDbPersistence(); - const localStore = newLocalStore( - persistence, - queryEngine, - User.UNAUTHENTICATED, - JSON_SERIALIZER - ); - return { queryEngine, persistence, localStore }; - } - - addEqualityMatcher(); - genericLocalStoreTests(initialize, /* gcIsEager= */ false); -}); +// describe('LocalStore w/ IndexedDB Persistence', () => { +// if (!IndexedDbPersistence.isAvailable()) { +// console.warn( +// 'No IndexedDB. Skipping LocalStore w/ IndexedDB persistence tests.' +// ); +// return; +// } +// +// async function initialize(): Promise { +// const queryEngine = new CountingQueryEngine(); +// const persistence = await persistenceHelpers.testIndexedDbPersistence(); +// const localStore = newLocalStore( +// persistence, +// queryEngine, +// User.UNAUTHENTICATED, +// JSON_SERIALIZER +// ); +// return { queryEngine, persistence, localStore }; +// } +// +// addEqualityMatcher(); +// genericLocalStoreTests(initialize, /* gcIsEager= */ false); +// }); function genericLocalStoreTests( getComponents: () => Promise, @@ -653,10 +653,10 @@ function genericLocalStoreTests( // Last seen version is zero, so this ack must be held. .afterAcknowledgingMutation({ documentVersion: 1 }) .toReturnChanged( - doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations() + doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations() ) .toNotContainIfEager( - doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations() + doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations() ) .after(setMutation('bar/baz', { bar: 'baz' })) .toReturnChanged( @@ -667,10 +667,10 @@ function genericLocalStoreTests( .toReturnRemoved('bar/baz') .toNotContain('bar/baz') .afterRemoteEvent( - docAddedRemoteEvent(doc('foo/bar', 2, { it: 'changed' }), [2]) + docAddedRemoteEvent(doc('foo/bar', 2, { it: 'changed' }, 1), [2]) ) - .toReturnChanged(doc('foo/bar', 2, { it: 'changed' })) - .toContain(doc('foo/bar', 2, { it: 'changed' })) + .toReturnChanged(doc('foo/bar', 2, { it: 'changed' }, 1)) + .toContain(doc('foo/bar', 2, { it: 'changed' }, 1)) .toNotContain('bar/baz') .finish() ); @@ -686,15 +686,31 @@ function genericLocalStoreTests( .toReturnRemoved('foo/bar') .toNotContainIfEager(deletedDoc('foo/bar', 2)) .after(setMutation('foo/bar', { foo: 'bar' })) - .toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) - .toContain(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) + .toReturnChanged( + // For Memory Persistence, after the `deletedDoc` event, eager GC + // removes the document from the cache, so the setMutation is applied on + // a non-existent document. The result is therefore a document without a + // createTime. + // + // For IndexedDB Persistence, a "NO_DOCUMENT" with version '2' remains + // in the cache, and the setMutation is applied on top of the + // NO_DOCUMENT, and uses its version (2000) as the new createTime. + gcIsEager ? + doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations() : + doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations() + ) + .toContain( + gcIsEager ? + doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations() : + doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations() + ) .afterReleasingTarget(2) .afterAcknowledgingMutation({ documentVersion: 3 }) .toReturnChanged( - doc('foo/bar', 3, { foo: 'bar' }).setHasCommittedMutations() + doc('foo/bar', 3, { foo: 'bar' }, 3).setHasCommittedMutations() ) .toNotContainIfEager( - doc('foo/bar', 3, { foo: 'bar' }).setHasCommittedMutations() + doc('foo/bar', 3, { foo: 'bar' }, 3).setHasCommittedMutations() ) .finish(); }); @@ -706,8 +722,8 @@ function genericLocalStoreTests( .after(setMutation('foo/bar', { foo: 'bar' })) .toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) .after(docUpdateRemoteEvent(deletedDoc('foo/bar', 2), [2])) - .toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) - .toContain(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) + .toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations()) + .toContain(doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations()) .finish(); }); @@ -1329,23 +1345,23 @@ function genericLocalStoreTests( .toContain(doc('foo/bar', 0, { sum: 0 }).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 1 }) .toReturnChanged( - doc('foo/bar', 1, { sum: 0 }).setHasCommittedMutations() + doc('foo/bar', 1, { sum: 0 }, 1).setHasCommittedMutations() ) - .toContain(doc('foo/bar', 1, { sum: 0 }).setHasCommittedMutations()) + .toContain(doc('foo/bar', 1, { sum: 0 }, 1).setHasCommittedMutations()) .after(patchMutation('foo/bar', { sum: increment(1) })) - .toReturnChanged(doc('foo/bar', 1, { sum: 1 }).setHasLocalMutations()) - .toContain(doc('foo/bar', 1, { sum: 1 }).setHasLocalMutations()) + .toReturnChanged(doc('foo/bar', 1, { sum: 1 }, 1).setHasLocalMutations()) + .toContain(doc('foo/bar', 1, { sum: 1 }, 1).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 2, transformResults: [{ integerValue: 1 }] }) .toReturnChanged( - doc('foo/bar', 2, { sum: 1 }).setHasCommittedMutations() + doc('foo/bar', 2, { sum: 1 }, 1).setHasCommittedMutations() ) - .toContain(doc('foo/bar', 2, { sum: 1 }).setHasCommittedMutations()) + .toContain(doc('foo/bar', 2, { sum: 1 }, 1).setHasCommittedMutations()) .after(patchMutation('foo/bar', { sum: increment(2) })) - .toReturnChanged(doc('foo/bar', 2, { sum: 3 }).setHasLocalMutations()) - .toContain(doc('foo/bar', 2, { sum: 3 }).setHasLocalMutations()) + .toReturnChanged(doc('foo/bar', 2, { sum: 3 }, 1).setHasLocalMutations()) + .toContain(doc('foo/bar', 2, { sum: 3 }, 1).setHasLocalMutations()) .finish(); } ); @@ -1413,37 +1429,37 @@ function genericLocalStoreTests( doc('foo/bar', 1, { sum: 0, arrayUnion: [] - }).setHasCommittedMutations() + }, 1).setHasCommittedMutations() ) .afterRemoteEvent( docAddedRemoteEvent( doc('foo/bar', 1, { sum: 0, arrayUnion: [] - }), + }, 1), [2] ) ) - .toReturnChanged(doc('foo/bar', 1, { sum: 0, arrayUnion: [] })) + .toReturnChanged(doc('foo/bar', 1, { sum: 0, arrayUnion: [] }, 1)) .after(patchMutation('foo/bar', { sum: increment(1) })) .toReturnChanged( doc('foo/bar', 1, { sum: 1, arrayUnion: [] - }).setHasLocalMutations() + }, 1).setHasLocalMutations() ) .after(patchMutation('foo/bar', { arrayUnion: arrayUnion('foo') })) .toReturnChanged( doc('foo/bar', 1, { sum: 1, arrayUnion: ['foo'] - }).setHasLocalMutations() + }, 1).setHasLocalMutations() ) // The sum transform and array union transform make the SDK ignore the // backend's updated value. .afterRemoteEvent( docUpdateRemoteEvent( - doc('foo/bar', 2, { sum: 1337, arrayUnion: ['bar'] }), + doc('foo/bar', 2, { sum: 1337, arrayUnion: ['bar'] }, 1), [2] ) ) @@ -1451,7 +1467,7 @@ function genericLocalStoreTests( doc('foo/bar', 2, { sum: 1, arrayUnion: ['foo'] - }).setHasLocalMutations() + }, 1).setHasLocalMutations() ) // With a field transform acknowledgement, the overlay is recalculated // with the remaining local mutations. @@ -1463,7 +1479,7 @@ function genericLocalStoreTests( doc('foo/bar', 3, { sum: 1338, arrayUnion: ['bar', 'foo'] - }) + }, 1) .setReadTime(SnapshotVersion.fromTimestamp(new Timestamp(0, 3000))) .setHasLocalMutations() ) @@ -1481,7 +1497,7 @@ function genericLocalStoreTests( doc('foo/bar', 4, { sum: 1338, arrayUnion: ['bar', 'foo'] - }) + }, 1) .setReadTime(SnapshotVersion.fromTimestamp(new Timestamp(0, 4000))) .setHasCommittedMutations() ) @@ -2005,7 +2021,7 @@ function genericLocalStoreTests( // set mutations. This is OK because it has no impact on aggregation's heuristic logic. But it feels // "wrong" to have createTime 0 here. We should revisit this. .toContain( - doc('col/doc1', 12, { foo: 'newBar' }, 0).setHasLocalMutations() + doc('col/doc1', 12, { foo: 'newBar' }, 12).setHasLocalMutations() ) .afterAcknowledgingMutation({ documentVersion: 13 }) // We haven't seen the remote event yet @@ -2209,19 +2225,19 @@ function genericLocalStoreTests( .afterAllocatingQuery(query1) .toReturnTargetId(2) .after( - docAddedRemoteEvent([doc('foo/a', 10, { matches: true })], [2], []) + docAddedRemoteEvent([doc('foo/a', 10, { matches: true }, 5)], [2], []) ) .after(localViewChanges(2, /* fromCache= */ false, {})) // Execute the query based on the RemoteEvent. .afterExecutingQuery(query1) - .toReturnChanged(doc('foo/a', 10, { matches: true })) + .toReturnChanged(doc('foo/a', 10, { matches: true }, 5)) // Write a document. .after(setMutation('foo/b', { matches: true })) // Execute the query and make sure that the pending mutation is // included in the result. .afterExecutingQuery(query1) .toReturnChanged( - doc('foo/a', 10, { matches: true }), + doc('foo/a', 10, { matches: true }, 5), doc('foo/b', 0, { matches: true }).setHasLocalMutations() ) .afterAcknowledgingMutation({ documentVersion: 11 }) @@ -2229,8 +2245,8 @@ function genericLocalStoreTests( // included in the result. .afterExecutingQuery(query1) .toReturnChanged( - doc('foo/a', 10, { matches: true }), - doc('foo/b', 11, { matches: true }).setHasCommittedMutations() + doc('foo/a', 10, { matches: true }, 5), + doc('foo/b', 11, { matches: true }, 11).setHasCommittedMutations() ) .finish() ); diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts index 10a85a031a0..fef1b6d0e3b 100644 --- a/packages/firestore/test/unit/model/mutation.test.ts +++ b/packages/firestore/test/unit/model/mutation.test.ts @@ -137,8 +137,8 @@ describe('Mutation', () => { ); } - expect(docForOverlay).to.deep.equal( - docForMutations, + expect(docForOverlay.setCreateTime(docForOverlay.version)).to.deep.equal( + docForMutations.setCreateTime(docForMutations.version), getDescription(doc, mutations, overlay) ); } @@ -674,8 +674,10 @@ describe('Mutation', () => { ).setHasCommittedMutations(); assertVersionTransitions(set, docV3, mutationResult, docV7Committed); - assertVersionTransitions(set, deletedV3, mutationResult, docV7Committed); - assertVersionTransitions(set, invalidV3, mutationResult, docV7Committed); + // For the following two cases, the mutation flips a deleted/unknown document to a known document, and in such + // situations, we assume a createTime at the given version. + assertVersionTransitions(set, deletedV3, mutationResult, docV7Committed.mutableCopy().setCreateTime(version(7))); + assertVersionTransitions(set, invalidV3, mutationResult, docV7Committed.mutableCopy().setCreateTime(version(7))); assertVersionTransitions(patch, docV3, mutationResult, docV7Committed); assertVersionTransitions(patch, deletedV3, mutationResult, docV7Unknown); @@ -977,7 +979,7 @@ describe('Mutation', () => { it('overlay by combinations and permutations', () => { const docs: MutableDocument[] = [ - doc('collection/key', 1, { 'foo': 'foo-value', 'bar': 1 }), + doc('collection/key', 1, { 'foo': 'foo-value', 'bar': 1 }, 1), deletedDoc('collection/key', 1), unknownDoc('collection/key', 1) ]; diff --git a/packages/firestore/test/unit/specs/bundle_spec.test.ts b/packages/firestore/test/unit/specs/bundle_spec.test.ts index 3efa1be4d63..e5633a63a23 100644 --- a/packages/firestore/test/unit/specs/bundle_spec.test.ts +++ b/packages/firestore/test/unit/specs/bundle_spec.test.ts @@ -102,7 +102,7 @@ describeSpec('Bundles:', ['no-ios'], () => { return ( spec() .userListens(query1) - .watchAcksFull(query1, 1000, docA) + .watchAcksFull(query1, 2000, docA) .expectEvents(query1, { added: [docA] }) // TODO(b/160876443): This currently raises snapshots with // `fromCache=false` if users already listen to some queries and bundles diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index b7db2049ec9..3d904279400 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -22,7 +22,7 @@ import { } from '../../../src/core/query'; import { TimerId } from '../../../src/util/async_queue'; import { Code } from '../../../src/util/error'; -import { deletedDoc, doc, filter, orderBy, query } from '../../util/helpers'; +import {deletedDoc, doc, filter, orderBy, query, version} from '../../util/helpers'; import { describeSpec, specTest } from './describe_spec'; import { client, spec } from './spec_builder'; @@ -502,13 +502,13 @@ describeSpec('Limbo Documents:', [], () => { const originalQuery = query('collection'); const filteredQuery = query('collection', filter('matches', '==', true)); - const docA = doc('collection/a', 1000, { matches: true }); + const docA = doc('collection/a', 1000, { matches: true }, 1000); const docADirty = doc('collection/a', 1000, { matches: true - }).setHasCommittedMutations(); + }, 1000).setHasCommittedMutations(); const docBDirty = doc('collection/b', 1001, { matches: true - }).setHasCommittedMutations(); + }, 1001).setHasCommittedMutations(); return ( spec() diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index 8bc62391838..7d0883e0493 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -959,11 +959,11 @@ describeSpec('Listens:', [], () => { ['multi-client'], () => { const query1 = query('collection'); - const docA = doc('collection/a', 1000, { key: '1' }); + const docA = doc('collection/a', 1000, { key: '1' }, 1000); const docADeleted = deletedDoc('collection/a', 2000); const docARecreated = doc('collection/a', 2000, { key: '2' - }).setHasLocalMutations(); + }, 2000).setHasLocalMutations(); return ( client(0) diff --git a/packages/firestore/test/unit/specs/persistence_spec.test.ts b/packages/firestore/test/unit/specs/persistence_spec.test.ts index 700b0ca4424..502ab4e90c8 100644 --- a/packages/firestore/test/unit/specs/persistence_spec.test.ts +++ b/packages/firestore/test/unit/specs/persistence_spec.test.ts @@ -75,7 +75,7 @@ describeSpec('Persistence:', [], () => { specTest("Remote documents from watch are not GC'd", [], () => { const query1 = query('collection'); - const doc1 = doc('collection/key', 1000, { foo: 'bar' }); + const doc1 = doc('collection/key', 1000, { foo: 'bar' }, 1000); return ( spec() .withGCEnabled(false) @@ -103,7 +103,7 @@ describeSpec('Persistence:', [], () => { added: [ doc('collection/key', 1000, { foo: 'bar' - }).setHasCommittedMutations() + }, 1000).setHasCommittedMutations() ], fromCache: true }) diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 6f17689a882..22c1c1f3e34 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -1061,6 +1061,7 @@ export class SpecBuilder { return { key: SpecBuilder.keyToSpec(doc.key), version: doc.version.toMicroseconds(), + createTime: doc.createTime.toMicroseconds(), value: userDataWriter.convertValue( doc.data.value ) as JsonObject, @@ -1073,6 +1074,7 @@ export class SpecBuilder { return { key: SpecBuilder.keyToSpec(doc.key), version: doc.version.toMicroseconds(), + createTime: doc.createTime.toMicroseconds(), value: null }; } diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 6a649bb213f..16f76a75796 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -663,7 +663,8 @@ abstract class TestRunner { ? doc( watchEntity.doc.key, watchEntity.doc.version, - watchEntity.doc.value + watchEntity.doc.value, + watchEntity.doc.createTime ) : deletedDoc(watchEntity.doc.key, watchEntity.doc.version); if (watchEntity.doc.options?.hasCommittedMutations) { @@ -1193,7 +1194,7 @@ abstract class TestRunner { type: ChangeType, change: SpecDocument ): DocumentViewChange { - const document = doc(change.key, change.version, change.value || {}); + const document = doc(change.key, change.version, change.value || {}, change.createTime); if (change.options?.hasCommittedMutations) { document.setHasCommittedMutations(); } else if (change.options?.hasLocalMutations) { @@ -1622,6 +1623,7 @@ export interface SpecQuery { export interface SpecDocument { key: string; version: TestSnapshotVersion; + createTime: TestSnapshotVersion; value: JsonObject | null; options?: DocumentOptions; } diff --git a/packages/firestore/test/unit/specs/write_spec.test.ts b/packages/firestore/test/unit/specs/write_spec.test.ts index a4f4396fb74..d33cbbcd3e9 100644 --- a/packages/firestore/test/unit/specs/write_spec.test.ts +++ b/packages/firestore/test/unit/specs/write_spec.test.ts @@ -127,7 +127,7 @@ describeSpec('Writes:', [], () => { const query1 = query('collection'); const modifiedDoc = doc('collection/doc', 1000, { v: 1 - }).setHasCommittedMutations(); + }, 1000).setHasCommittedMutations(); return spec() .withGCEnabled(false) .userSets('collection/doc', { v: 1 }) @@ -932,7 +932,7 @@ describeSpec('Writes:', [], () => { const docV1 = doc('collection/doc', 0, { v: 1 }).setHasLocalMutations(); const docV1Committed = doc('collection/doc', 2000, { v: 1 - }).setHasCommittedMutations(); + }, 2000).setHasCommittedMutations(); const docV1Acknowledged = doc('collection/doc', 2000, { v: 1 }); return ( client(0) @@ -1314,10 +1314,10 @@ describeSpec('Writes:', [], () => { const query1 = query('collection'); const docA = doc('collection/a', 1000, { k: 'a' - }).setHasCommittedMutations(); + }, 1000).setHasCommittedMutations(); const docB = doc('collection/b', 2000, { k: 'b' - }).setHasCommittedMutations(); + }, 2000).setHasCommittedMutations(); return client(0) .expectPrimaryState(true) diff --git a/packages/firestore/test/util/spec_test_helpers.ts b/packages/firestore/test/util/spec_test_helpers.ts index c721d0e0675..f8b8715d4c7 100644 --- a/packages/firestore/test/util/spec_test_helpers.ts +++ b/packages/firestore/test/util/spec_test_helpers.ts @@ -57,7 +57,8 @@ export function encodeWatchChange( document: { name: toName(serializer, doc.key), fields: doc?.data.value.mapValue.fields, - updateTime: toVersion(serializer, doc.version) + updateTime: toVersion(serializer, doc.version), + createTime: toVersion(serializer, doc?.createTime) }, targetIds: watchChange.updatedTargetIds, removedTargetIds: watchChange.removedTargetIds From fcac75d4200b280af16a0e552a23c32d9484523a Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Mon, 21 Nov 2022 16:18:49 -0800 Subject: [PATCH 4/9] Don't use createTime in isEqual. --- .../local/indexeddb_remote_document_cache.ts | 17 --- .../src/local/local_documents_view.ts | 1 - packages/firestore/src/model/document.ts | 23 ++- .../test/unit/local/local_store.test.ts | 134 ++++++++---------- .../test/unit/model/mutation.test.ts | 12 +- .../test/unit/specs/bundle_spec.test.ts | 2 +- .../test/unit/specs/limbo_spec.test.ts | 8 +- .../test/unit/specs/listen_spec.test.ts | 4 +- .../test/unit/specs/persistence_spec.test.ts | 4 +- .../test/unit/specs/spec_test_runner.ts | 7 +- .../test/unit/specs/write_spec.test.ts | 8 +- 11 files changed, 93 insertions(+), 127 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index 7895732525e..19b7c566322 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -517,14 +517,6 @@ class IndexedDbRemoteDocumentChangeBuffer extends RemoteDocumentChangeBuffer { size: getResult.size, readTime: getResult.document.readTime }); - - // This is because older SDK versions did not store createTime. - // This can be removed in the long term. - if (!getResult.document.createTime) { - throw('this will never get executed'); - //getResult.document.createTime = SnapshotVersion.min(); - } - return getResult.document; }); } @@ -547,15 +539,6 @@ class IndexedDbRemoteDocumentChangeBuffer extends RemoteDocumentChangeBuffer { readTime: documents.get(documentKey)!.readTime }); }); - - // This is because older SDK versions did not store createTime. - // This can be removed in the long term. - documents.forEach((key, doc) => { - if(!doc.createTime) { - throw('this will never get executed'); - //doc.createTime = SnapshotVersion.min(); - } - }); return documents; }); } diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 1a1670ecca6..2421d920e95 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -46,7 +46,6 @@ import { FieldMask } from '../model/field_mask'; import { calculateOverlayMutation, mutationApplyToLocalView, - MutationType, PatchMutation } from '../model/mutation'; import { Overlay } from '../model/overlay'; diff --git a/packages/firestore/src/model/document.ts b/packages/firestore/src/model/document.ts index 9843115e271..59cd82fcf3a 100644 --- a/packages/firestore/src/model/document.ts +++ b/packages/firestore/src/model/document.ts @@ -256,13 +256,16 @@ export class MutableDocument implements Document { version: SnapshotVersion, value: ObjectValue ): MutableDocument { - // TODO(COUNT): Add comment about why we're updating createTime here. - if (this.createTime.isEqual(SnapshotVersion.min())&& - (this.documentType === DocumentType.NO_DOCUMENT || this.documentType === DocumentType.INVALID)) { - this.createTime = version; - } - if (!this.createTime) { - throw("this will never get executed"); + // If a document is switching state from being an invalid or deleted + // document to a valid (FOUND_DOCUMENT) document, either due to receiving an + // update from Watch or due to applying a local set mutation on top + // of a deleted document, our best guess about its createTime would be the + // version at which the document transitioned to a FOUND_DOCUMENT. + if ( + this.createTime.isEqual(SnapshotVersion.min()) && + (this.documentType === DocumentType.NO_DOCUMENT || + this.documentType === DocumentType.INVALID) + ) { this.createTime = version; } this.version = version; @@ -317,11 +320,6 @@ export class MutableDocument implements Document { return this; } - setCreateTime(createTime: SnapshotVersion): MutableDocument { - this.createTime = createTime; - return this; - } - get hasLocalMutations(): boolean { return this.documentState === DocumentState.HAS_LOCAL_MUTATIONS; } @@ -354,7 +352,6 @@ export class MutableDocument implements Document { return ( other instanceof MutableDocument && this.key.isEqual(other.key) && - this.createTime.isEqual(other.createTime) && this.version.isEqual(other.version) && this.documentType === other.documentType && this.documentState === other.documentState && diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 77cadf8ac2b..c3e665945ac 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -556,29 +556,29 @@ describe('LocalStore w/ Memory Persistence', () => { genericLocalStoreTests(initialize, /* gcIsEager= */ true); }); -// describe('LocalStore w/ IndexedDB Persistence', () => { -// if (!IndexedDbPersistence.isAvailable()) { -// console.warn( -// 'No IndexedDB. Skipping LocalStore w/ IndexedDB persistence tests.' -// ); -// return; -// } -// -// async function initialize(): Promise { -// const queryEngine = new CountingQueryEngine(); -// const persistence = await persistenceHelpers.testIndexedDbPersistence(); -// const localStore = newLocalStore( -// persistence, -// queryEngine, -// User.UNAUTHENTICATED, -// JSON_SERIALIZER -// ); -// return { queryEngine, persistence, localStore }; -// } -// -// addEqualityMatcher(); -// genericLocalStoreTests(initialize, /* gcIsEager= */ false); -// }); +describe('LocalStore w/ IndexedDB Persistence', () => { + if (!IndexedDbPersistence.isAvailable()) { + console.warn( + 'No IndexedDB. Skipping LocalStore w/ IndexedDB persistence tests.' + ); + return; + } + + async function initialize(): Promise { + const queryEngine = new CountingQueryEngine(); + const persistence = await persistenceHelpers.testIndexedDbPersistence(); + const localStore = newLocalStore( + persistence, + queryEngine, + User.UNAUTHENTICATED, + JSON_SERIALIZER + ); + return { queryEngine, persistence, localStore }; + } + + addEqualityMatcher(); + genericLocalStoreTests(initialize, /* gcIsEager= */ false); +}); function genericLocalStoreTests( getComponents: () => Promise, @@ -616,10 +616,10 @@ function genericLocalStoreTests( .toContain(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 1 }) .toReturnChanged( - doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations() + doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations() ) .toNotContainIfEager( - doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations() + doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations() ) .finish(); }); @@ -653,10 +653,10 @@ function genericLocalStoreTests( // Last seen version is zero, so this ack must be held. .afterAcknowledgingMutation({ documentVersion: 1 }) .toReturnChanged( - doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations() + doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations() ) .toNotContainIfEager( - doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations() + doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations() ) .after(setMutation('bar/baz', { bar: 'baz' })) .toReturnChanged( @@ -667,10 +667,10 @@ function genericLocalStoreTests( .toReturnRemoved('bar/baz') .toNotContain('bar/baz') .afterRemoteEvent( - docAddedRemoteEvent(doc('foo/bar', 2, { it: 'changed' }, 1), [2]) + docAddedRemoteEvent(doc('foo/bar', 2, { it: 'changed' }), [2]) ) - .toReturnChanged(doc('foo/bar', 2, { it: 'changed' }, 1)) - .toContain(doc('foo/bar', 2, { it: 'changed' }, 1)) + .toReturnChanged(doc('foo/bar', 2, { it: 'changed' })) + .toContain(doc('foo/bar', 2, { it: 'changed' })) .toNotContain('bar/baz') .finish() ); @@ -686,31 +686,15 @@ function genericLocalStoreTests( .toReturnRemoved('foo/bar') .toNotContainIfEager(deletedDoc('foo/bar', 2)) .after(setMutation('foo/bar', { foo: 'bar' })) - .toReturnChanged( - // For Memory Persistence, after the `deletedDoc` event, eager GC - // removes the document from the cache, so the setMutation is applied on - // a non-existent document. The result is therefore a document without a - // createTime. - // - // For IndexedDB Persistence, a "NO_DOCUMENT" with version '2' remains - // in the cache, and the setMutation is applied on top of the - // NO_DOCUMENT, and uses its version (2000) as the new createTime. - gcIsEager ? - doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations() : - doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations() - ) - .toContain( - gcIsEager ? - doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations() : - doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations() - ) + .toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) + .toContain(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) .afterReleasingTarget(2) .afterAcknowledgingMutation({ documentVersion: 3 }) .toReturnChanged( - doc('foo/bar', 3, { foo: 'bar' }, 3).setHasCommittedMutations() + doc('foo/bar', 3, { foo: 'bar' }).setHasCommittedMutations() ) .toNotContainIfEager( - doc('foo/bar', 3, { foo: 'bar' }, 3).setHasCommittedMutations() + doc('foo/bar', 3, { foo: 'bar' }).setHasCommittedMutations() ) .finish(); }); @@ -722,8 +706,8 @@ function genericLocalStoreTests( .after(setMutation('foo/bar', { foo: 'bar' })) .toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) .after(docUpdateRemoteEvent(deletedDoc('foo/bar', 2), [2])) - .toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations()) - .toContain(doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations()) + .toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) + .toContain(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations()) .finish(); }); @@ -1345,23 +1329,23 @@ function genericLocalStoreTests( .toContain(doc('foo/bar', 0, { sum: 0 }).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 1 }) .toReturnChanged( - doc('foo/bar', 1, { sum: 0 }, 1).setHasCommittedMutations() + doc('foo/bar', 1, { sum: 0 }).setHasCommittedMutations() ) - .toContain(doc('foo/bar', 1, { sum: 0 }, 1).setHasCommittedMutations()) + .toContain(doc('foo/bar', 1, { sum: 0 }).setHasCommittedMutations()) .after(patchMutation('foo/bar', { sum: increment(1) })) - .toReturnChanged(doc('foo/bar', 1, { sum: 1 }, 1).setHasLocalMutations()) - .toContain(doc('foo/bar', 1, { sum: 1 }, 1).setHasLocalMutations()) + .toReturnChanged(doc('foo/bar', 1, { sum: 1 }).setHasLocalMutations()) + .toContain(doc('foo/bar', 1, { sum: 1 }).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 2, transformResults: [{ integerValue: 1 }] }) .toReturnChanged( - doc('foo/bar', 2, { sum: 1 }, 1).setHasCommittedMutations() + doc('foo/bar', 2, { sum: 1 }).setHasCommittedMutations() ) - .toContain(doc('foo/bar', 2, { sum: 1 }, 1).setHasCommittedMutations()) + .toContain(doc('foo/bar', 2, { sum: 1 }).setHasCommittedMutations()) .after(patchMutation('foo/bar', { sum: increment(2) })) - .toReturnChanged(doc('foo/bar', 2, { sum: 3 }, 1).setHasLocalMutations()) - .toContain(doc('foo/bar', 2, { sum: 3 }, 1).setHasLocalMutations()) + .toReturnChanged(doc('foo/bar', 2, { sum: 3 }).setHasLocalMutations()) + .toContain(doc('foo/bar', 2, { sum: 3 }).setHasLocalMutations()) .finish(); } ); @@ -1429,37 +1413,37 @@ function genericLocalStoreTests( doc('foo/bar', 1, { sum: 0, arrayUnion: [] - }, 1).setHasCommittedMutations() + }).setHasCommittedMutations() ) .afterRemoteEvent( docAddedRemoteEvent( doc('foo/bar', 1, { sum: 0, arrayUnion: [] - }, 1), + }), [2] ) ) - .toReturnChanged(doc('foo/bar', 1, { sum: 0, arrayUnion: [] }, 1)) + .toReturnChanged(doc('foo/bar', 1, { sum: 0, arrayUnion: [] })) .after(patchMutation('foo/bar', { sum: increment(1) })) .toReturnChanged( doc('foo/bar', 1, { sum: 1, arrayUnion: [] - }, 1).setHasLocalMutations() + }).setHasLocalMutations() ) .after(patchMutation('foo/bar', { arrayUnion: arrayUnion('foo') })) .toReturnChanged( doc('foo/bar', 1, { sum: 1, arrayUnion: ['foo'] - }, 1).setHasLocalMutations() + }).setHasLocalMutations() ) // The sum transform and array union transform make the SDK ignore the // backend's updated value. .afterRemoteEvent( docUpdateRemoteEvent( - doc('foo/bar', 2, { sum: 1337, arrayUnion: ['bar'] }, 1), + doc('foo/bar', 2, { sum: 1337, arrayUnion: ['bar'] }), [2] ) ) @@ -1467,7 +1451,7 @@ function genericLocalStoreTests( doc('foo/bar', 2, { sum: 1, arrayUnion: ['foo'] - }, 1).setHasLocalMutations() + }).setHasLocalMutations() ) // With a field transform acknowledgement, the overlay is recalculated // with the remaining local mutations. @@ -1479,7 +1463,7 @@ function genericLocalStoreTests( doc('foo/bar', 3, { sum: 1338, arrayUnion: ['bar', 'foo'] - }, 1) + }) .setReadTime(SnapshotVersion.fromTimestamp(new Timestamp(0, 3000))) .setHasLocalMutations() ) @@ -1497,7 +1481,7 @@ function genericLocalStoreTests( doc('foo/bar', 4, { sum: 1338, arrayUnion: ['bar', 'foo'] - }, 1) + }) .setReadTime(SnapshotVersion.fromTimestamp(new Timestamp(0, 4000))) .setHasCommittedMutations() ) @@ -2021,7 +2005,7 @@ function genericLocalStoreTests( // set mutations. This is OK because it has no impact on aggregation's heuristic logic. But it feels // "wrong" to have createTime 0 here. We should revisit this. .toContain( - doc('col/doc1', 12, { foo: 'newBar' }, 12).setHasLocalMutations() + doc('col/doc1', 12, { foo: 'newBar' }, 0).setHasLocalMutations() ) .afterAcknowledgingMutation({ documentVersion: 13 }) // We haven't seen the remote event yet @@ -2225,19 +2209,19 @@ function genericLocalStoreTests( .afterAllocatingQuery(query1) .toReturnTargetId(2) .after( - docAddedRemoteEvent([doc('foo/a', 10, { matches: true }, 5)], [2], []) + docAddedRemoteEvent([doc('foo/a', 10, { matches: true })], [2], []) ) .after(localViewChanges(2, /* fromCache= */ false, {})) // Execute the query based on the RemoteEvent. .afterExecutingQuery(query1) - .toReturnChanged(doc('foo/a', 10, { matches: true }, 5)) + .toReturnChanged(doc('foo/a', 10, { matches: true })) // Write a document. .after(setMutation('foo/b', { matches: true })) // Execute the query and make sure that the pending mutation is // included in the result. .afterExecutingQuery(query1) .toReturnChanged( - doc('foo/a', 10, { matches: true }, 5), + doc('foo/a', 10, { matches: true }), doc('foo/b', 0, { matches: true }).setHasLocalMutations() ) .afterAcknowledgingMutation({ documentVersion: 11 }) @@ -2245,8 +2229,8 @@ function genericLocalStoreTests( // included in the result. .afterExecutingQuery(query1) .toReturnChanged( - doc('foo/a', 10, { matches: true }, 5), - doc('foo/b', 11, { matches: true }, 11).setHasCommittedMutations() + doc('foo/a', 10, { matches: true }), + doc('foo/b', 11, { matches: true }).setHasCommittedMutations() ) .finish() ); diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts index fef1b6d0e3b..10a85a031a0 100644 --- a/packages/firestore/test/unit/model/mutation.test.ts +++ b/packages/firestore/test/unit/model/mutation.test.ts @@ -137,8 +137,8 @@ describe('Mutation', () => { ); } - expect(docForOverlay.setCreateTime(docForOverlay.version)).to.deep.equal( - docForMutations.setCreateTime(docForMutations.version), + expect(docForOverlay).to.deep.equal( + docForMutations, getDescription(doc, mutations, overlay) ); } @@ -674,10 +674,8 @@ describe('Mutation', () => { ).setHasCommittedMutations(); assertVersionTransitions(set, docV3, mutationResult, docV7Committed); - // For the following two cases, the mutation flips a deleted/unknown document to a known document, and in such - // situations, we assume a createTime at the given version. - assertVersionTransitions(set, deletedV3, mutationResult, docV7Committed.mutableCopy().setCreateTime(version(7))); - assertVersionTransitions(set, invalidV3, mutationResult, docV7Committed.mutableCopy().setCreateTime(version(7))); + assertVersionTransitions(set, deletedV3, mutationResult, docV7Committed); + assertVersionTransitions(set, invalidV3, mutationResult, docV7Committed); assertVersionTransitions(patch, docV3, mutationResult, docV7Committed); assertVersionTransitions(patch, deletedV3, mutationResult, docV7Unknown); @@ -979,7 +977,7 @@ describe('Mutation', () => { it('overlay by combinations and permutations', () => { const docs: MutableDocument[] = [ - doc('collection/key', 1, { 'foo': 'foo-value', 'bar': 1 }, 1), + doc('collection/key', 1, { 'foo': 'foo-value', 'bar': 1 }), deletedDoc('collection/key', 1), unknownDoc('collection/key', 1) ]; diff --git a/packages/firestore/test/unit/specs/bundle_spec.test.ts b/packages/firestore/test/unit/specs/bundle_spec.test.ts index e5633a63a23..3efa1be4d63 100644 --- a/packages/firestore/test/unit/specs/bundle_spec.test.ts +++ b/packages/firestore/test/unit/specs/bundle_spec.test.ts @@ -102,7 +102,7 @@ describeSpec('Bundles:', ['no-ios'], () => { return ( spec() .userListens(query1) - .watchAcksFull(query1, 2000, docA) + .watchAcksFull(query1, 1000, docA) .expectEvents(query1, { added: [docA] }) // TODO(b/160876443): This currently raises snapshots with // `fromCache=false` if users already listen to some queries and bundles diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index 3d904279400..b7db2049ec9 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -22,7 +22,7 @@ import { } from '../../../src/core/query'; import { TimerId } from '../../../src/util/async_queue'; import { Code } from '../../../src/util/error'; -import {deletedDoc, doc, filter, orderBy, query, version} from '../../util/helpers'; +import { deletedDoc, doc, filter, orderBy, query } from '../../util/helpers'; import { describeSpec, specTest } from './describe_spec'; import { client, spec } from './spec_builder'; @@ -502,13 +502,13 @@ describeSpec('Limbo Documents:', [], () => { const originalQuery = query('collection'); const filteredQuery = query('collection', filter('matches', '==', true)); - const docA = doc('collection/a', 1000, { matches: true }, 1000); + const docA = doc('collection/a', 1000, { matches: true }); const docADirty = doc('collection/a', 1000, { matches: true - }, 1000).setHasCommittedMutations(); + }).setHasCommittedMutations(); const docBDirty = doc('collection/b', 1001, { matches: true - }, 1001).setHasCommittedMutations(); + }).setHasCommittedMutations(); return ( spec() diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index 7d0883e0493..8bc62391838 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -959,11 +959,11 @@ describeSpec('Listens:', [], () => { ['multi-client'], () => { const query1 = query('collection'); - const docA = doc('collection/a', 1000, { key: '1' }, 1000); + const docA = doc('collection/a', 1000, { key: '1' }); const docADeleted = deletedDoc('collection/a', 2000); const docARecreated = doc('collection/a', 2000, { key: '2' - }, 2000).setHasLocalMutations(); + }).setHasLocalMutations(); return ( client(0) diff --git a/packages/firestore/test/unit/specs/persistence_spec.test.ts b/packages/firestore/test/unit/specs/persistence_spec.test.ts index 502ab4e90c8..700b0ca4424 100644 --- a/packages/firestore/test/unit/specs/persistence_spec.test.ts +++ b/packages/firestore/test/unit/specs/persistence_spec.test.ts @@ -75,7 +75,7 @@ describeSpec('Persistence:', [], () => { specTest("Remote documents from watch are not GC'd", [], () => { const query1 = query('collection'); - const doc1 = doc('collection/key', 1000, { foo: 'bar' }, 1000); + const doc1 = doc('collection/key', 1000, { foo: 'bar' }); return ( spec() .withGCEnabled(false) @@ -103,7 +103,7 @@ describeSpec('Persistence:', [], () => { added: [ doc('collection/key', 1000, { foo: 'bar' - }, 1000).setHasCommittedMutations() + }).setHasCommittedMutations() ], fromCache: true }) diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 16f76a75796..a86b4943757 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -1194,7 +1194,12 @@ abstract class TestRunner { type: ChangeType, change: SpecDocument ): DocumentViewChange { - const document = doc(change.key, change.version, change.value || {}, change.createTime); + const document = doc( + change.key, + change.version, + change.value || {}, + change.createTime + ); if (change.options?.hasCommittedMutations) { document.setHasCommittedMutations(); } else if (change.options?.hasLocalMutations) { diff --git a/packages/firestore/test/unit/specs/write_spec.test.ts b/packages/firestore/test/unit/specs/write_spec.test.ts index d33cbbcd3e9..a4f4396fb74 100644 --- a/packages/firestore/test/unit/specs/write_spec.test.ts +++ b/packages/firestore/test/unit/specs/write_spec.test.ts @@ -127,7 +127,7 @@ describeSpec('Writes:', [], () => { const query1 = query('collection'); const modifiedDoc = doc('collection/doc', 1000, { v: 1 - }, 1000).setHasCommittedMutations(); + }).setHasCommittedMutations(); return spec() .withGCEnabled(false) .userSets('collection/doc', { v: 1 }) @@ -932,7 +932,7 @@ describeSpec('Writes:', [], () => { const docV1 = doc('collection/doc', 0, { v: 1 }).setHasLocalMutations(); const docV1Committed = doc('collection/doc', 2000, { v: 1 - }, 2000).setHasCommittedMutations(); + }).setHasCommittedMutations(); const docV1Acknowledged = doc('collection/doc', 2000, { v: 1 }); return ( client(0) @@ -1314,10 +1314,10 @@ describeSpec('Writes:', [], () => { const query1 = query('collection'); const docA = doc('collection/a', 1000, { k: 'a' - }, 1000).setHasCommittedMutations(); + }).setHasCommittedMutations(); const docB = doc('collection/b', 2000, { k: 'b' - }, 2000).setHasCommittedMutations(); + }).setHasCommittedMutations(); return client(0) .expectPrimaryState(true) From a5b06da14c1d15bf4ef6e83d8847097fcff42f4c Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 23 Nov 2022 12:26:34 -0800 Subject: [PATCH 5/9] getBaseDocument should always get results from remoteDocumentCache. --- .../src/local/local_documents_view.ts | 23 +- .../test/unit/local/local_store.test.ts | 241 +++++++++++++++--- 2 files changed, 204 insertions(+), 60 deletions(-) diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 2421d920e95..e5d50c18e25 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -91,7 +91,7 @@ export class LocalDocumentsView { .getOverlay(transaction, key) .next(value => { overlay = value; - return this.getBaseDocument(transaction, key, overlay); + return this.remoteDocumentCache.getEntry(transaction, key); }) .next(document => { if (overlay !== null) { @@ -423,11 +423,11 @@ export class LocalDocumentsView { if (originalDocs.get(key)) { return PersistencePromise.resolve(); } - return this.getBaseDocument(transaction, key, overlay).next( - doc => { + return this.remoteDocumentCache + .getEntry(transaction, key) + .next(doc => { modifiedDocs = modifiedDocs.insert(key, doc); - } - ); + }); } ) .next(() => @@ -549,17 +549,4 @@ export class LocalDocumentsView { return results; }); } - - /** Returns a base document that can be used to apply `overlay`. */ - private getBaseDocument( - transaction: PersistenceTransaction, - key: DocumentKey, - overlay: Overlay | null - ): PersistencePromise { - return this.remoteDocumentCache.getEntry(transaction, key); - // TODO(COUNT): ROI of this is pretty low and it could be quite confusing. - // return overlay === null || overlay.mutation.type === MutationType.Patch - // ? this.remoteDocumentCache.getEntry(transaction, key) - // : PersistencePromise.resolve(MutableDocument.newInvalidDocument(key)); - } } diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index c3e665945ac..30e5113f1f9 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -398,7 +398,10 @@ class LocalStoreTester { return this; } - toReturnChanged(...docs: Document[]): LocalStoreTester { + toReturnChangedInternal( + docs: Document[], + isEqual?: (lhs: Document | null, rhs: Document | null) => boolean + ): LocalStoreTester { this.promiseChain = this.promiseChain.then(() => { debugAssert( this.lastChanges !== null, @@ -407,13 +410,29 @@ class LocalStoreTester { expect(this.lastChanges.size).to.equal(docs.length, 'number of changes'); for (const doc of docs) { const returned = this.lastChanges.get(doc.key); - expectEqual(doc, returned, `Expected '${returned}' to equal '${doc}'.`); + const message = `Expected '${returned}' to equal '${doc}'.`; + if (isEqual) { + expect(isEqual(doc, returned)).to.equal(true, message); + } else { + expectEqual(doc, returned, message); + } } this.lastChanges = null; }); return this; } + toReturnChanged(...docs: Document[]): LocalStoreTester { + return this.toReturnChangedInternal(docs); + } + + toReturnChangedWithDocComparator( + isEqual: (lhs: Document | null, rhs: Document | null) => boolean, + ...docs: Document[] + ): LocalStoreTester { + return this.toReturnChangedInternal(docs, isEqual); + } + toReturnRemoved(...keyStrings: string[]): LocalStoreTester { this.promiseChain = this.promiseChain.then(() => { debugAssert( @@ -433,16 +452,20 @@ class LocalStoreTester { return this; } - toContain(doc: Document): LocalStoreTester { + toContain( + doc: Document, + isEqual?: (lhs: Document, rhs: Document) => boolean + ): LocalStoreTester { this.promiseChain = this.promiseChain.then(() => { return localStoreReadDocument(this.localStore, doc.key).then(result => { - expectEqual( - result, - doc, - `Expected ${ - result ? result.toString() : null - } to match ${doc.toString()}.` - ); + const message = `Expected ${ + result ? result.toString() : null + } to match ${doc.toString()}.`; + if (isEqual) { + expect(isEqual(result, doc)).to.equal(true, message); + } else { + expectEqual(result, doc, message); + } }); }); return this; @@ -539,22 +562,38 @@ class LocalStoreTester { } } -describe('LocalStore w/ Memory Persistence', () => { - async function initialize(): Promise { - const queryEngine = new CountingQueryEngine(); - const persistence = await persistenceHelpers.testMemoryEagerPersistence(); - const localStore = newLocalStore( - persistence, - queryEngine, - User.UNAUTHENTICATED, - JSON_SERIALIZER - ); - return { queryEngine, persistence, localStore }; - } +// The `isEqual` method for the Document class does not compare createTime and +// readTime. For some tests, we'd like to verify that a certain createTime has +// been calculated for documents. In such cases we can use this comparator. +function compareDocsWithCreateTime( + lhs: Document | null, + rhs: Document | null +): boolean { + return ( + (lhs === null && rhs === null) || + (lhs !== null && + rhs !== null && + lhs.isEqual(rhs) && + lhs.createTime.isEqual(rhs.createTime)) + ); +} - addEqualityMatcher(); - genericLocalStoreTests(initialize, /* gcIsEager= */ true); -}); +// describe('LocalStore w/ Memory Persistence', () => { +// async function initialize(): Promise { +// const queryEngine = new CountingQueryEngine(); +// const persistence = await persistenceHelpers.testMemoryEagerPersistence(); +// const localStore = newLocalStore( +// persistence, +// queryEngine, +// User.UNAUTHENTICATED, +// JSON_SERIALIZER +// ); +// return { queryEngine, persistence, localStore }; +// } +// +// addEqualityMatcher(); +// genericLocalStoreTests(initialize, /* gcIsEager= */ true); +// }); describe('LocalStore w/ IndexedDB Persistence', () => { if (!IndexedDbPersistence.isAvailable()) { @@ -1964,28 +2003,58 @@ function genericLocalStoreTests( .afterAllocatingQuery(query('col')) .toReturnTargetId(2) .after(docAddedRemoteEvent(doc('col/doc1', 12, { foo: 'bar' }, 5), [2])) - .toReturnChanged(doc('col/doc1', 12, { foo: 'bar' }, 5)) - .toContain(doc('col/doc1', 12, { foo: 'bar' }, 5)) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc('col/doc1', 12, { foo: 'bar' }, 5) + ) + .toContain( + doc('col/doc1', 12, { foo: 'bar' }, 5), + compareDocsWithCreateTime + ) .after(setMutation('col/doc1', { foo: 'newBar' })) - .toReturnChanged( + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, doc('col/doc1', 12, { foo: 'newBar' }, 5).setHasLocalMutations() ) .toContain( - doc('col/doc1', 12, { foo: 'newBar' }, 5).setHasLocalMutations() + doc('col/doc1', 12, { foo: 'newBar' }, 5).setHasLocalMutations(), + compareDocsWithCreateTime ) .afterAcknowledgingMutation({ documentVersion: 13 }) // We haven't seen the remote event yet - .toReturnChanged( + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, doc('col/doc1', 13, { foo: 'newBar' }, 5).setHasCommittedMutations() ) .toContain( - doc('col/doc1', 13, { foo: 'newBar' }, 5).setHasCommittedMutations() + doc('col/doc1', 13, { foo: 'newBar' }, 5).setHasCommittedMutations(), + compareDocsWithCreateTime ) .finish() ); }); - it('saves updateTime as createTime when creating new doc', () => { + it('saves updateTime as createTime when receives ack for creating a new doc', () => { + if (gcIsEager) { + return; + } + + return expectLocalStore() + .after(setMutation('col/doc1', { foo: 'newBar' })) + .afterAcknowledgingMutation({ documentVersion: 13 }) + .afterExecutingQuery(query('col')) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() + ) + .toContain( + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations(), + compareDocsWithCreateTime + ) + .finish(); + }); + + it('saves updateTime as createTime when recreating a deleted doc', async () => { if (gcIsEager) { return; } @@ -1998,28 +2067,29 @@ function genericLocalStoreTests( .toReturnChanged(deletedDoc('col/doc1', 12)) .toContain(deletedDoc('col/doc1', 12)) .after(setMutation('col/doc1', { foo: 'newBar' })) - .toReturnChanged( + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, doc('col/doc1', 12, { foo: 'newBar' }, 12).setHasLocalMutations() ) - // TODO(COUNT): Below has createTime=0 due to an optimization that uses invalid doc as base doc for - // set mutations. This is OK because it has no impact on aggregation's heuristic logic. But it feels - // "wrong" to have createTime 0 here. We should revisit this. .toContain( - doc('col/doc1', 12, { foo: 'newBar' }, 0).setHasLocalMutations() + doc('col/doc1', 12, { foo: 'newBar' }, 12).setHasLocalMutations(), + compareDocsWithCreateTime ) .afterAcknowledgingMutation({ documentVersion: 13 }) // We haven't seen the remote event yet - .toReturnChanged( + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() ) .toContain( - doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations(), + compareDocsWithCreateTime ) .finish() ); }); - it('saves updateTime as createTime when creating new doc', () => { + it('document createTime is preserved through Set -> Ack -> Patch -> Ack', () => { if (gcIsEager) { return; } @@ -2028,11 +2098,98 @@ function genericLocalStoreTests( .after(setMutation('col/doc1', { foo: 'newBar' })) .afterAcknowledgingMutation({ documentVersion: 13 }) .afterExecutingQuery(query('col')) - .toReturnChanged( + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() ) .toContain( - doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations(), + compareDocsWithCreateTime + ) + .afterMutations([patchMutation('col/doc1', { 'likes': 1 })]) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc( + 'col/doc1', + 13, + { foo: 'newBar', likes: 1 }, + 13 + ).setHasLocalMutations() + ) + .toContain( + doc( + 'col/doc1', + 13, + { foo: 'newBar', likes: 1 }, + 13 + ).setHasLocalMutations(), + compareDocsWithCreateTime + ) + .afterAcknowledgingMutation({ documentVersion: 14 }) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc( + 'col/doc1', + 14, + { foo: 'newBar', likes: 1 }, + 13 + ).setHasCommittedMutations() + ) + .toContain( + doc( + 'col/doc1', + 14, + { foo: 'newBar', likes: 1 }, + 13 + ).setHasCommittedMutations(), + compareDocsWithCreateTime + ) + .finish(); + }); + + it('document createTime is preserved through Doc Added -> Patch -> Ack', () => { + if (gcIsEager) { + return; + } + return expectLocalStore() + .afterAllocatingQuery(query('col')) + .toReturnTargetId(2) + .after(docAddedRemoteEvent(doc('col/doc1', 12, { foo: 'bar' }, 5), [2])) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc('col/doc1', 12, { foo: 'bar' }, 5) + ) + .toContain( + doc('col/doc1', 12, { foo: 'bar' }, 5), + compareDocsWithCreateTime + ) + .afterMutations([patchMutation('col/doc1', { 'likes': 1 })]) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc('col/doc1', 13, { foo: 'bar', likes: 1 }, 5).setHasLocalMutations() + ) + .toContain( + doc('col/doc1', 13, { foo: 'bar', likes: 1 }, 5).setHasLocalMutations(), + compareDocsWithCreateTime + ) + .afterAcknowledgingMutation({ documentVersion: 14 }) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc( + 'col/doc1', + 14, + { foo: 'bar', likes: 1 }, + 5 + ).setHasCommittedMutations() + ) + .toContain( + doc( + 'col/doc1', + 14, + { foo: 'bar', likes: 1 }, + 5 + ).setHasCommittedMutations(), + compareDocsWithCreateTime ) .finish(); }); From cd61c7dacf4b72d93bc802e5f4aa5111fda56a89 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 23 Nov 2022 12:38:32 -0800 Subject: [PATCH 6/9] Bring back tests. --- .../test/unit/local/local_store.test.ts | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 30e5113f1f9..f87f6ef1c02 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -578,22 +578,22 @@ function compareDocsWithCreateTime( ); } -// describe('LocalStore w/ Memory Persistence', () => { -// async function initialize(): Promise { -// const queryEngine = new CountingQueryEngine(); -// const persistence = await persistenceHelpers.testMemoryEagerPersistence(); -// const localStore = newLocalStore( -// persistence, -// queryEngine, -// User.UNAUTHENTICATED, -// JSON_SERIALIZER -// ); -// return { queryEngine, persistence, localStore }; -// } -// -// addEqualityMatcher(); -// genericLocalStoreTests(initialize, /* gcIsEager= */ true); -// }); +describe('LocalStore w/ Memory Persistence', () => { + async function initialize(): Promise { + const queryEngine = new CountingQueryEngine(); + const persistence = await persistenceHelpers.testMemoryEagerPersistence(); + const localStore = newLocalStore( + persistence, + queryEngine, + User.UNAUTHENTICATED, + JSON_SERIALIZER + ); + return { queryEngine, persistence, localStore }; + } + + addEqualityMatcher(); + genericLocalStoreTests(initialize, /* gcIsEager= */ true); +}); describe('LocalStore w/ IndexedDB Persistence', () => { if (!IndexedDbPersistence.isAvailable()) { From eea9d453973cb5cf4f2de99435626d3bf74d13b2 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 29 Nov 2022 15:16:18 -0800 Subject: [PATCH 7/9] Add test that checks createTime gets updated after remote event. --- .../test/unit/local/local_store.test.ts | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index f87f6ef1c02..0f9d9198b8e 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -2054,6 +2054,35 @@ function genericLocalStoreTests( .finish(); }); + it('updates createTime upon receiving a remote event with a new createTime', () => { + if (gcIsEager) { + return; + } + + return expectLocalStore() + .after(setMutation('col/doc1', { foo: 'newBar' })) + .afterAcknowledgingMutation({ documentVersion: 13 }) + .afterExecutingQuery(query('col')) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations() + ) + .toContain( + doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations(), + compareDocsWithCreateTime + ) + .after(docAddedRemoteEvent(doc('col/doc1', 14, { foo: 'baz' }, 5), [2])) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc('col/doc1', 14, { foo: 'baz' }, 5) + ) + .toContain( + doc('col/doc1', 14, { foo: 'baz' }, 5), + compareDocsWithCreateTime + ) + .finish(); + }); + it('saves updateTime as createTime when recreating a deleted doc', async () => { if (gcIsEager) { return; From a763e61edf5cb2b9b1559fcf4a9e56b9df633dce Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 29 Nov 2022 15:47:34 -0800 Subject: [PATCH 8/9] Add test for Set->RemoteEvent->Ack. --- .../test/unit/local/local_store.test.ts | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 0f9d9198b8e..a7b4e519fd7 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -2054,7 +2054,7 @@ function genericLocalStoreTests( .finish(); }); - it('updates createTime upon receiving a remote event with a new createTime', () => { + it('handles createTime for Set -> Ack -> RemoteEvent', () => { if (gcIsEager) { return; } @@ -2083,6 +2083,27 @@ function genericLocalStoreTests( .finish(); }); + it('handles createTime for Set -> RemoteEvent -> Ack', () => { + if (gcIsEager) { + return; + } + + return expectLocalStore() + .after(setMutation('col/doc1', { foo: 'newBar' })) + .after(docAddedRemoteEvent(doc('col/doc1', 13, { foo: 'baz' }, 5), [2])) + .afterAcknowledgingMutation({ documentVersion: 14 }) + .afterExecutingQuery(query('col')) + .toReturnChangedWithDocComparator( + compareDocsWithCreateTime, + doc('col/doc1', 14, { foo: 'newBar' }, 5).setHasCommittedMutations() + ) + .toContain( + doc('col/doc1', 14, { foo: 'newBar' }, 5).setHasCommittedMutations(), + compareDocsWithCreateTime + ) + .finish(); + }); + it('saves updateTime as createTime when recreating a deleted doc', async () => { if (gcIsEager) { return; From c80911ddb16a6c998e429b4d0a1ccf939d79b39d Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 30 Nov 2022 21:37:48 -0800 Subject: [PATCH 9/9] try higher timeout. --- packages/firestore/test/unit/model/mutation.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts index 10a85a031a0..f6a707a72e2 100644 --- a/packages/firestore/test/unit/model/mutation.test.ts +++ b/packages/firestore/test/unit/model/mutation.test.ts @@ -1012,7 +1012,7 @@ describe('Mutation', () => { // There are (0! + 7*1! + 21*2! + 35*3! + 35*4! + 21*5! + 7*6! + 7!) * 3 = 41100 cases. expect(testCases).to.equal(41100); - }); + }).timeout(10000); it('overlay by combinations and permutations for array transforms', () => { const docs: MutableDocument[] = [