Skip to content

Commit fcac75d

Browse files
ehsannasehsannas
committed
Don't use createTime in isEqual.
1 parent 467ed0e commit fcac75d

File tree

11 files changed

+93
-127
lines changed

11 files changed

+93
-127
lines changed

‎packages/firestore/src/local/indexeddb_remote_document_cache.ts

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -517,14 +517,6 @@ class IndexedDbRemoteDocumentChangeBuffer extends RemoteDocumentChangeBuffer {
517517
size: getResult.size,
518518
readTime: getResult.document.readTime
519519
});
520-
521-
// This is because older SDK versions did not store createTime.
522-
// This can be removed in the long term.
523-
if (!getResult.document.createTime) {
524-
throw('this will never get executed');
525-
//getResult.document.createTime = SnapshotVersion.min();
526-
}
527-
528520
return getResult.document;
529521
});
530522
}
@@ -547,15 +539,6 @@ class IndexedDbRemoteDocumentChangeBuffer extends RemoteDocumentChangeBuffer {
547539
readTime: documents.get(documentKey)!.readTime
548540
});
549541
});
550-
551-
// This is because older SDK versions did not store createTime.
552-
// This can be removed in the long term.
553-
documents.forEach((key, doc) => {
554-
if(!doc.createTime) {
555-
throw('this will never get executed');
556-
//doc.createTime = SnapshotVersion.min();
557-
}
558-
});
559542
return documents;
560543
});
561544
}

‎packages/firestore/src/local/local_documents_view.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@ import { FieldMask } from '../model/field_mask';
4646
import {
4747
calculateOverlayMutation,
4848
mutationApplyToLocalView,
49-
MutationType,
5049
PatchMutation
5150
} from '../model/mutation';
5251
import { Overlay } from '../model/overlay';

‎packages/firestore/src/model/document.ts

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -256,13 +256,16 @@ export class MutableDocument implements Document {
256256
version: SnapshotVersion,
257257
value: ObjectValue
258258
): MutableDocument {
259-
// TODO(COUNT): Add comment about why we're updating createTime here.
260-
if (this.createTime.isEqual(SnapshotVersion.min())&&
261-
(this.documentType === DocumentType.NO_DOCUMENT || this.documentType === DocumentType.INVALID)) {
262-
this.createTime = version;
263-
}
264-
if (!this.createTime) {
265-
throw("this will never get executed");
259+
// If a document is switching state from being an invalid or deleted
260+
// document to a valid (FOUND_DOCUMENT) document, either due to receiving an
261+
// update from Watch or due to applying a local set mutation on top
262+
// of a deleted document, our best guess about its createTime would be the
263+
// version at which the document transitioned to a FOUND_DOCUMENT.
264+
if (
265+
this.createTime.isEqual(SnapshotVersion.min()) &&
266+
(this.documentType === DocumentType.NO_DOCUMENT ||
267+
this.documentType === DocumentType.INVALID)
268+
) {
266269
this.createTime = version;
267270
}
268271
this.version = version;
@@ -317,11 +320,6 @@ export class MutableDocument implements Document {
317320
return this;
318321
}
319322

320-
setCreateTime(createTime: SnapshotVersion): MutableDocument {
321-
this.createTime = createTime;
322-
return this;
323-
}
324-
325323
get hasLocalMutations(): boolean {
326324
return this.documentState === DocumentState.HAS_LOCAL_MUTATIONS;
327325
}
@@ -354,7 +352,6 @@ export class MutableDocument implements Document {
354352
return (
355353
other instanceof MutableDocument &&
356354
this.key.isEqual(other.key) &&
357-
this.createTime.isEqual(other.createTime) &&
358355
this.version.isEqual(other.version) &&
359356
this.documentType === other.documentType &&
360357
this.documentState === other.documentState &&

‎packages/firestore/test/unit/local/local_store.test.ts

Lines changed: 59 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -556,29 +556,29 @@ describe('LocalStore w/ Memory Persistence', () => {
556556
genericLocalStoreTests(initialize, /* gcIsEager= */ true);
557557
});
558558

559-
// describe('LocalStore w/ IndexedDB Persistence', () => {
560-
// if (!IndexedDbPersistence.isAvailable()) {
561-
// console.warn(
562-
// 'No IndexedDB. Skipping LocalStore w/ IndexedDB persistence tests.'
563-
// );
564-
// return;
565-
// }
566-
//
567-
// async function initialize(): Promise<LocalStoreComponents> {
568-
// const queryEngine = new CountingQueryEngine();
569-
// const persistence = await persistenceHelpers.testIndexedDbPersistence();
570-
// const localStore = newLocalStore(
571-
// persistence,
572-
// queryEngine,
573-
// User.UNAUTHENTICATED,
574-
// JSON_SERIALIZER
575-
// );
576-
// return { queryEngine, persistence, localStore };
577-
// }
578-
//
579-
// addEqualityMatcher();
580-
// genericLocalStoreTests(initialize, /* gcIsEager= */ false);
581-
// });
559+
describe('LocalStore w/ IndexedDB Persistence', () => {
560+
if (!IndexedDbPersistence.isAvailable()) {
561+
console.warn(
562+
'No IndexedDB. Skipping LocalStore w/ IndexedDB persistence tests.'
563+
);
564+
return;
565+
}
566+
567+
async function initialize(): Promise<LocalStoreComponents> {
568+
const queryEngine = new CountingQueryEngine();
569+
const persistence = await persistenceHelpers.testIndexedDbPersistence();
570+
const localStore = newLocalStore(
571+
persistence,
572+
queryEngine,
573+
User.UNAUTHENTICATED,
574+
JSON_SERIALIZER
575+
);
576+
return { queryEngine, persistence, localStore };
577+
}
578+
579+
addEqualityMatcher();
580+
genericLocalStoreTests(initialize, /* gcIsEager= */ false);
581+
});
582582

583583
function genericLocalStoreTests(
584584
getComponents: () => Promise<LocalStoreComponents>,
@@ -616,10 +616,10 @@ function genericLocalStoreTests(
616616
.toContain(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations())
617617
.afterAcknowledgingMutation({ documentVersion: 1 })
618618
.toReturnChanged(
619-
doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations()
619+
doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations()
620620
)
621621
.toNotContainIfEager(
622-
doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations()
622+
doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations()
623623
)
624624
.finish();
625625
});
@@ -653,10 +653,10 @@ function genericLocalStoreTests(
653653
// Last seen version is zero, so this ack must be held.
654654
.afterAcknowledgingMutation({ documentVersion: 1 })
655655
.toReturnChanged(
656-
doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations()
656+
doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations()
657657
)
658658
.toNotContainIfEager(
659-
doc('foo/bar', 1, { foo: 'bar' }, 1).setHasCommittedMutations()
659+
doc('foo/bar', 1, { foo: 'bar' }).setHasCommittedMutations()
660660
)
661661
.after(setMutation('bar/baz', { bar: 'baz' }))
662662
.toReturnChanged(
@@ -667,10 +667,10 @@ function genericLocalStoreTests(
667667
.toReturnRemoved('bar/baz')
668668
.toNotContain('bar/baz')
669669
.afterRemoteEvent(
670-
docAddedRemoteEvent(doc('foo/bar', 2, { it: 'changed' }, 1), [2])
670+
docAddedRemoteEvent(doc('foo/bar', 2, { it: 'changed' }), [2])
671671
)
672-
.toReturnChanged(doc('foo/bar', 2, { it: 'changed' }, 1))
673-
.toContain(doc('foo/bar', 2, { it: 'changed' }, 1))
672+
.toReturnChanged(doc('foo/bar', 2, { it: 'changed' }))
673+
.toContain(doc('foo/bar', 2, { it: 'changed' }))
674674
.toNotContain('bar/baz')
675675
.finish()
676676
);
@@ -686,31 +686,15 @@ function genericLocalStoreTests(
686686
.toReturnRemoved('foo/bar')
687687
.toNotContainIfEager(deletedDoc('foo/bar', 2))
688688
.after(setMutation('foo/bar', { foo: 'bar' }))
689-
.toReturnChanged(
690-
// For Memory Persistence, after the `deletedDoc` event, eager GC
691-
// removes the document from the cache, so the setMutation is applied on
692-
// a non-existent document. The result is therefore a document without a
693-
// createTime.
694-
//
695-
// For IndexedDB Persistence, a "NO_DOCUMENT" with version '2' remains
696-
// in the cache, and the setMutation is applied on top of the
697-
// NO_DOCUMENT, and uses its version (2000) as the new createTime.
698-
gcIsEager ?
699-
doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations() :
700-
doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations()
701-
)
702-
.toContain(
703-
gcIsEager ?
704-
doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations() :
705-
doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations()
706-
)
689+
.toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations())
690+
.toContain(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations())
707691
.afterReleasingTarget(2)
708692
.afterAcknowledgingMutation({ documentVersion: 3 })
709693
.toReturnChanged(
710-
doc('foo/bar', 3, { foo: 'bar' }, 3).setHasCommittedMutations()
694+
doc('foo/bar', 3, { foo: 'bar' }).setHasCommittedMutations()
711695
)
712696
.toNotContainIfEager(
713-
doc('foo/bar', 3, { foo: 'bar' }, 3).setHasCommittedMutations()
697+
doc('foo/bar', 3, { foo: 'bar' }).setHasCommittedMutations()
714698
)
715699
.finish();
716700
});
@@ -722,8 +706,8 @@ function genericLocalStoreTests(
722706
.after(setMutation('foo/bar', { foo: 'bar' }))
723707
.toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations())
724708
.after(docUpdateRemoteEvent(deletedDoc('foo/bar', 2), [2]))
725-
.toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations())
726-
.toContain(doc('foo/bar', 0, { foo: 'bar' }, 2).setHasLocalMutations())
709+
.toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations())
710+
.toContain(doc('foo/bar', 0, { foo: 'bar' }).setHasLocalMutations())
727711
.finish();
728712
});
729713

@@ -1345,23 +1329,23 @@ function genericLocalStoreTests(
13451329
.toContain(doc('foo/bar', 0, { sum: 0 }).setHasLocalMutations())
13461330
.afterAcknowledgingMutation({ documentVersion: 1 })
13471331
.toReturnChanged(
1348-
doc('foo/bar', 1, { sum: 0 }, 1).setHasCommittedMutations()
1332+
doc('foo/bar', 1, { sum: 0 }).setHasCommittedMutations()
13491333
)
1350-
.toContain(doc('foo/bar', 1, { sum: 0 }, 1).setHasCommittedMutations())
1334+
.toContain(doc('foo/bar', 1, { sum: 0 }).setHasCommittedMutations())
13511335
.after(patchMutation('foo/bar', { sum: increment(1) }))
1352-
.toReturnChanged(doc('foo/bar', 1, { sum: 1 }, 1).setHasLocalMutations())
1353-
.toContain(doc('foo/bar', 1, { sum: 1 }, 1).setHasLocalMutations())
1336+
.toReturnChanged(doc('foo/bar', 1, { sum: 1 }).setHasLocalMutations())
1337+
.toContain(doc('foo/bar', 1, { sum: 1 }).setHasLocalMutations())
13541338
.afterAcknowledgingMutation({
13551339
documentVersion: 2,
13561340
transformResults: [{ integerValue: 1 }]
13571341
})
13581342
.toReturnChanged(
1359-
doc('foo/bar', 2, { sum: 1 }, 1).setHasCommittedMutations()
1343+
doc('foo/bar', 2, { sum: 1 }).setHasCommittedMutations()
13601344
)
1361-
.toContain(doc('foo/bar', 2, { sum: 1 }, 1).setHasCommittedMutations())
1345+
.toContain(doc('foo/bar', 2, { sum: 1 }).setHasCommittedMutations())
13621346
.after(patchMutation('foo/bar', { sum: increment(2) }))
1363-
.toReturnChanged(doc('foo/bar', 2, { sum: 3 }, 1).setHasLocalMutations())
1364-
.toContain(doc('foo/bar', 2, { sum: 3 }, 1).setHasLocalMutations())
1347+
.toReturnChanged(doc('foo/bar', 2, { sum: 3 }).setHasLocalMutations())
1348+
.toContain(doc('foo/bar', 2, { sum: 3 }).setHasLocalMutations())
13651349
.finish();
13661350
}
13671351
);
@@ -1429,45 +1413,45 @@ function genericLocalStoreTests(
14291413
doc('foo/bar', 1, {
14301414
sum: 0,
14311415
arrayUnion: []
1432-
}, 1).setHasCommittedMutations()
1416+
}).setHasCommittedMutations()
14331417
)
14341418
.afterRemoteEvent(
14351419
docAddedRemoteEvent(
14361420
doc('foo/bar', 1, {
14371421
sum: 0,
14381422
arrayUnion: []
1439-
}, 1),
1423+
}),
14401424
[2]
14411425
)
14421426
)
1443-
.toReturnChanged(doc('foo/bar', 1, { sum: 0, arrayUnion: [] }, 1))
1427+
.toReturnChanged(doc('foo/bar', 1, { sum: 0, arrayUnion: [] }))
14441428
.after(patchMutation('foo/bar', { sum: increment(1) }))
14451429
.toReturnChanged(
14461430
doc('foo/bar', 1, {
14471431
sum: 1,
14481432
arrayUnion: []
1449-
}, 1).setHasLocalMutations()
1433+
}).setHasLocalMutations()
14501434
)
14511435
.after(patchMutation('foo/bar', { arrayUnion: arrayUnion('foo') }))
14521436
.toReturnChanged(
14531437
doc('foo/bar', 1, {
14541438
sum: 1,
14551439
arrayUnion: ['foo']
1456-
}, 1).setHasLocalMutations()
1440+
}).setHasLocalMutations()
14571441
)
14581442
// The sum transform and array union transform make the SDK ignore the
14591443
// backend's updated value.
14601444
.afterRemoteEvent(
14611445
docUpdateRemoteEvent(
1462-
doc('foo/bar', 2, { sum: 1337, arrayUnion: ['bar'] }, 1),
1446+
doc('foo/bar', 2, { sum: 1337, arrayUnion: ['bar'] }),
14631447
[2]
14641448
)
14651449
)
14661450
.toReturnChanged(
14671451
doc('foo/bar', 2, {
14681452
sum: 1,
14691453
arrayUnion: ['foo']
1470-
}, 1).setHasLocalMutations()
1454+
}).setHasLocalMutations()
14711455
)
14721456
// With a field transform acknowledgement, the overlay is recalculated
14731457
// with the remaining local mutations.
@@ -1479,7 +1463,7 @@ function genericLocalStoreTests(
14791463
doc('foo/bar', 3, {
14801464
sum: 1338,
14811465
arrayUnion: ['bar', 'foo']
1482-
}, 1)
1466+
})
14831467
.setReadTime(SnapshotVersion.fromTimestamp(new Timestamp(0, 3000)))
14841468
.setHasLocalMutations()
14851469
)
@@ -1497,7 +1481,7 @@ function genericLocalStoreTests(
14971481
doc('foo/bar', 4, {
14981482
sum: 1338,
14991483
arrayUnion: ['bar', 'foo']
1500-
}, 1)
1484+
})
15011485
.setReadTime(SnapshotVersion.fromTimestamp(new Timestamp(0, 4000)))
15021486
.setHasCommittedMutations()
15031487
)
@@ -2021,7 +2005,7 @@ function genericLocalStoreTests(
20212005
// set mutations. This is OK because it has no impact on aggregation's heuristic logic. But it feels
20222006
// "wrong" to have createTime 0 here. We should revisit this.
20232007
.toContain(
2024-
doc('col/doc1', 12, { foo: 'newBar' }, 12).setHasLocalMutations()
2008+
doc('col/doc1', 12, { foo: 'newBar' }, 0).setHasLocalMutations()
20252009
)
20262010
.afterAcknowledgingMutation({ documentVersion: 13 })
20272011
// We haven't seen the remote event yet
@@ -2225,28 +2209,28 @@ function genericLocalStoreTests(
22252209
.afterAllocatingQuery(query1)
22262210
.toReturnTargetId(2)
22272211
.after(
2228-
docAddedRemoteEvent([doc('foo/a', 10, { matches: true }, 5)], [2], [])
2212+
docAddedRemoteEvent([doc('foo/a', 10, { matches: true })], [2], [])
22292213
)
22302214
.after(localViewChanges(2, /* fromCache= */ false, {}))
22312215
// Execute the query based on the RemoteEvent.
22322216
.afterExecutingQuery(query1)
2233-
.toReturnChanged(doc('foo/a', 10, { matches: true }, 5))
2217+
.toReturnChanged(doc('foo/a', 10, { matches: true }))
22342218
// Write a document.
22352219
.after(setMutation('foo/b', { matches: true }))
22362220
// Execute the query and make sure that the pending mutation is
22372221
// included in the result.
22382222
.afterExecutingQuery(query1)
22392223
.toReturnChanged(
2240-
doc('foo/a', 10, { matches: true }, 5),
2224+
doc('foo/a', 10, { matches: true }),
22412225
doc('foo/b', 0, { matches: true }).setHasLocalMutations()
22422226
)
22432227
.afterAcknowledgingMutation({ documentVersion: 11 })
22442228
// Execute the query and make sure that the acknowledged mutation is
22452229
// included in the result.
22462230
.afterExecutingQuery(query1)
22472231
.toReturnChanged(
2248-
doc('foo/a', 10, { matches: true }, 5),
2249-
doc('foo/b', 11, { matches: true }, 11).setHasCommittedMutations()
2232+
doc('foo/a', 10, { matches: true }),
2233+
doc('foo/b', 11, { matches: true }).setHasCommittedMutations()
22502234
)
22512235
.finish()
22522236
);

0 commit comments

Comments
 (0)