Skip to content

Commit fd66c1a

Browse files
schmidt-sebastianschmidt-sebastian
authored
Mark mostly readonly calls as idempotent (#2252)
1 parent 1383598 commit fd66c1a

12 files changed

+171
-115
lines changed

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

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ import { MutationQueue } from './mutation_queue';
6262
import {
6363
Persistence,
6464
PersistenceTransaction,
65+
PersistenceTransactionMode,
6566
PrimaryStateListener,
6667
ReferenceDelegate
6768
} from './persistence';
@@ -121,15 +122,6 @@ export class IndexedDbTransaction extends PersistenceTransaction {
121122
}
122123
}
123124

124-
// The different modes supported by `IndexedDbPersistence.runTransaction()`
125-
type IndexedDbTransactionMode =
126-
| 'readonly'
127-
| 'readwrite'
128-
| 'readwrite-primary'
129-
| 'readonly-idempotent'
130-
| 'readwrite-idempotent'
131-
| 'readwrite-primary-idempotent';
132-
133125
/**
134126
* An IndexedDB-backed instance of Persistence. Data is stored persistently
135127
* across sessions.
@@ -748,7 +740,7 @@ export class IndexedDbPersistence implements Persistence {
748740

749741
runTransaction<T>(
750742
action: string,
751-
mode: IndexedDbTransactionMode,
743+
mode: PersistenceTransactionMode,
752744
transactionOperation: (
753745
transaction: PersistenceTransaction
754746
) => PersistencePromise<T>

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

Lines changed: 45 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,7 @@ export class LocalStore {
321321
lookupMutationDocuments(batchId: BatchId): Promise<MaybeDocumentMap | null> {
322322
return this.persistence.runTransaction(
323323
'Lookup mutation documents',
324-
'readonly',
324+
'readonly-idempotent',
325325
txn => {
326326
return this.mutationQueue
327327
.lookupMutationKeys(txn, batchId)
@@ -412,7 +412,7 @@ export class LocalStore {
412412
getHighestUnacknowledgedBatchId(): Promise<BatchId> {
413413
return this.persistence.runTransaction(
414414
'Get highest unacknowledged batch id',
415-
'readonly',
415+
'readonly-idempotent',
416416
txn => {
417417
return this.mutationQueue.getHighestUnacknowledgedBatchId(txn);
418418
}
@@ -423,7 +423,7 @@ export class LocalStore {
423423
getLastStreamToken(): Promise<ProtoByteString> {
424424
return this.persistence.runTransaction(
425425
'Get last stream token',
426-
'readonly',
426+
'readonly-idempotent',
427427
txn => {
428428
return this.mutationQueue.getLastStreamToken(txn);
429429
}
@@ -438,7 +438,7 @@ export class LocalStore {
438438
setLastStreamToken(streamToken: ProtoByteString): Promise<void> {
439439
return this.persistence.runTransaction(
440440
'Set last stream token',
441-
'readwrite-primary',
441+
'readwrite-primary-idempotent',
442442
txn => {
443443
return this.mutationQueue.setLastStreamToken(txn, streamToken);
444444
}
@@ -452,7 +452,7 @@ export class LocalStore {
452452
getLastRemoteSnapshotVersion(): Promise<SnapshotVersion> {
453453
return this.persistence.runTransaction(
454454
'Get last remote snapshot version',
455-
'readonly',
455+
'readonly-idempotent',
456456
txn => this.queryCache.getLastRemoteSnapshotVersion(txn)
457457
);
458458
}
@@ -731,7 +731,7 @@ export class LocalStore {
731731
nextMutationBatch(afterBatchId?: BatchId): Promise<MutationBatch | null> {
732732
return this.persistence.runTransaction(
733733
'Get next mutation batch',
734-
'readonly',
734+
'readonly-idempotent',
735735
txn => {
736736
if (afterBatchId === undefined) {
737737
afterBatchId = BATCHID_UNKNOWN;
@@ -749,9 +749,13 @@ export class LocalStore {
749749
* found - used for testing.
750750
*/
751751
readDocument(key: DocumentKey): Promise<MaybeDocument | null> {
752-
return this.persistence.runTransaction('read document', 'readonly', txn => {
753-
return this.localDocuments.getDocument(txn, key);
754-
});
752+
return this.persistence.runTransaction(
753+
'read document',
754+
'readonly-idempotent',
755+
txn => {
756+
return this.localDocuments.getDocument(txn, key);
757+
}
758+
);
755759
}
756760

757761
/**
@@ -870,33 +874,37 @@ export class LocalStore {
870874
let lastLimboFreeSnapshotVersion = SnapshotVersion.MIN;
871875
let remoteKeys = documentKeySet();
872876

873-
return this.persistence.runTransaction('Execute query', 'readonly', txn => {
874-
return this.getQueryData(txn, query)
875-
.next(queryData => {
876-
if (queryData) {
877-
lastLimboFreeSnapshotVersion =
878-
queryData.lastLimboFreeSnapshotVersion;
879-
return this.queryCache
880-
.getMatchingKeysForTargetId(txn, queryData.targetId)
881-
.next(result => {
882-
remoteKeys = result;
883-
});
884-
}
885-
})
886-
.next(() =>
887-
this.queryEngine.getDocumentsMatchingQuery(
888-
txn,
889-
query,
890-
usePreviousResults
891-
? lastLimboFreeSnapshotVersion
892-
: SnapshotVersion.MIN,
893-
usePreviousResults ? remoteKeys : documentKeySet()
877+
return this.persistence.runTransaction(
878+
'Execute query',
879+
'readonly-idempotent',
880+
txn => {
881+
return this.getQueryData(txn, query)
882+
.next(queryData => {
883+
if (queryData) {
884+
lastLimboFreeSnapshotVersion =
885+
queryData.lastLimboFreeSnapshotVersion;
886+
return this.queryCache
887+
.getMatchingKeysForTargetId(txn, queryData.targetId)
888+
.next(result => {
889+
remoteKeys = result;
890+
});
891+
}
892+
})
893+
.next(() =>
894+
this.queryEngine.getDocumentsMatchingQuery(
895+
txn,
896+
query,
897+
usePreviousResults
898+
? lastLimboFreeSnapshotVersion
899+
: SnapshotVersion.MIN,
900+
usePreviousResults ? remoteKeys : documentKeySet()
901+
)
894902
)
895-
)
896-
.next(documents => {
897-
return { documents, remoteKeys };
898-
});
899-
});
903+
.next(documents => {
904+
return { documents, remoteKeys };
905+
});
906+
}
907+
);
900908
}
901909

902910
/**
@@ -906,7 +914,7 @@ export class LocalStore {
906914
remoteDocumentKeys(targetId: TargetId): Promise<DocumentKeySet> {
907915
return this.persistence.runTransaction(
908916
'Remote document keys',
909-
'readonly',
917+
'readonly-idempotent',
910918
txn => {
911919
return this.queryCache.getMatchingKeysForTargetId(txn, targetId);
912920
}
@@ -988,7 +996,7 @@ export class LocalStore {
988996
} else {
989997
return this.persistence.runTransaction(
990998
'Get query data',
991-
'readonly',
999+
'readonly-idempotent',
9921000
txn => {
9931001
return this.queryCache
9941002
.getQueryDataForTarget(txn, targetId)

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import { MutationQueue } from './mutation_queue';
4242
import {
4343
Persistence,
4444
PersistenceTransaction,
45+
PersistenceTransactionMode,
4546
PrimaryStateListener,
4647
ReferenceDelegate
4748
} from './persistence';
@@ -169,7 +170,7 @@ export class MemoryPersistence implements Persistence {
169170

170171
runTransaction<T>(
171172
action: string,
172-
mode: 'readonly' | 'readwrite' | 'readwrite-primary',
173+
mode: PersistenceTransactionMode,
173174
transactionOperation: (
174175
transaction: PersistenceTransaction
175176
) => PersistencePromise<T>

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,15 @@ export abstract class PersistenceTransaction {
3939
abstract readonly currentSequenceNumber: ListenSequenceNumber;
4040
}
4141

42+
/** The different modes supported by `IndexedDbPersistence.runTransaction()`. */
43+
export type PersistenceTransactionMode =
44+
| 'readonly'
45+
| 'readwrite'
46+
| 'readwrite-primary'
47+
| 'readonly-idempotent'
48+
| 'readwrite-idempotent'
49+
| 'readwrite-primary-idempotent';
50+
4251
/**
4352
* Callback type for primary state notifications. This callback can be
4453
* registered with the persistence layer to get notified when we transition from
@@ -255,7 +264,7 @@ export interface Persistence {
255264
*/
256265
runTransaction<T>(
257266
action: string,
258-
mode: 'readonly' | 'readwrite' | 'readwrite-primary',
267+
mode: PersistenceTransactionMode,
259268
transactionOperation: (
260269
transaction: PersistenceTransaction
261270
) => PersistencePromise<T>

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,14 @@ export class SimpleDb {
273273
objectStores
274274
);
275275
try {
276+
// TODO(schmidt-sebastian): Remove this code/comment or find a way to
277+
// make this a test-only setting.
278+
// // Horrible hack to verify that idempotent functions can be run more
279+
// // than once.
280+
// const transactionFnResult = (idempotent && attemptNumber === 1
281+
// ? transactionFn(transaction)
282+
// : PersistencePromise.resolve({} as T)
283+
// ).next(() => transactionFn(transaction))
276284
const transactionFnResult = transactionFn(transaction)
277285
.catch(error => {
278286
// Abort the transaction if there was an error.

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

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -144,27 +144,31 @@ describe('IndexFreeQueryEngine', () => {
144144
'Encountered runQuery() call not wrapped in expectIndexFreeQuery()/expectFullCollectionQuery()'
145145
);
146146

147-
return persistence.runTransaction('runQuery', 'readonly', txn => {
148-
return queryCache
149-
.getMatchingKeysForTargetId(txn, TEST_TARGET_ID)
150-
.next(remoteKeys => {
151-
return queryEngine
152-
.getDocumentsMatchingQuery(
153-
txn,
154-
query,
155-
lastLimboFreeSnapshot,
156-
remoteKeys
157-
)
158-
.next(docs => {
159-
const view = new View(query, remoteKeys);
160-
const viewDocChanges = view.computeDocChanges(docs);
161-
return view.applyChanges(
162-
viewDocChanges,
163-
/*updateLimboDocuments=*/ true
164-
).snapshot!.docs;
165-
});
166-
});
167-
});
147+
return persistence.runTransaction(
148+
'runQuery',
149+
'readonly-idempotent',
150+
txn => {
151+
return queryCache
152+
.getMatchingKeysForTargetId(txn, TEST_TARGET_ID)
153+
.next(remoteKeys => {
154+
return queryEngine
155+
.getDocumentsMatchingQuery(
156+
txn,
157+
query,
158+
lastLimboFreeSnapshot,
159+
remoteKeys
160+
)
161+
.next(docs => {
162+
const view = new View(query, remoteKeys);
163+
const viewDocChanges = view.computeDocChanges(docs);
164+
return view.applyChanges(
165+
viewDocChanges,
166+
/*updateLimboDocuments=*/ true
167+
).snapshot!.docs;
168+
});
169+
});
170+
}
171+
);
168172
}
169173

170174
beforeEach(async () => {

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1546,7 +1546,7 @@ function genericLocalStoreTests(
15461546
// At this point, we have not yet confirmed that the query is limbo free.
15471547
let cachedQueryData = await persistence.runTransaction(
15481548
'getQueryData',
1549-
'readonly',
1549+
'readonly-idempotent',
15501550
txn => localStore.getQueryData(txn, query)
15511551
);
15521552
expect(
@@ -1559,7 +1559,7 @@ function genericLocalStoreTests(
15591559
]);
15601560
cachedQueryData = await persistence.runTransaction(
15611561
'getQueryData',
1562-
'readonly',
1562+
'readonly-idempotent',
15631563
txn => localStore.getQueryData(txn, query)
15641564
);
15651565
expect(cachedQueryData!.lastLimboFreeSnapshotVersion.isEqual(version(10)))
@@ -1572,7 +1572,7 @@ function genericLocalStoreTests(
15721572
if (!gcIsEager) {
15731573
cachedQueryData = await persistence.runTransaction(
15741574
'getQueryData',
1575-
'readonly',
1575+
'readonly-idempotent',
15761576
txn => localStore.getQueryData(txn, query)
15771577
);
15781578
expect(cachedQueryData!.lastLimboFreeSnapshotVersion.isEqual(version(10)))

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export class TestIndexManager {
3232
addToCollectionParentIndex(collectionPath: ResourcePath): Promise<void> {
3333
return this.persistence.runTransaction(
3434
'addToCollectionParentIndex',
35-
'readwrite',
35+
'readwrite-idempotent',
3636
txn => {
3737
return this.indexManager.addToCollectionParentIndex(
3838
txn,
@@ -45,7 +45,7 @@ export class TestIndexManager {
4545
getCollectionParents(collectionId: string): Promise<ResourcePath[]> {
4646
return this.persistence.runTransaction(
4747
'getCollectionParents',
48-
'readwrite',
48+
'readonly-idempotent',
4949
txn => {
5050
return this.indexManager.getCollectionParents(txn, collectionId);
5151
}

0 commit comments

Comments
 (0)