From 0caf2c49f6a98933168caed911b15929583dc10b Mon Sep 17 00:00:00 2001 From: Martynas Kazlauskas Date: Fri, 25 Oct 2024 13:10:39 +0300 Subject: [PATCH] fix(wallet): preserve pouchdb collection documents when bulkDocs fails PouchDbCollectionStore.setAll used to 1. delete all documents 2. insert the new documents This approach has a problem that when step 2. fails, all documents from db are gone and not re-created. For some collections such as transactions or utxo this is not a problem, because it can recover, but for it is a big problem for WalletRepository, because there is no way to recover the wallets if they are lost. This fix updates setAll to perform the following steps: 1. delete *only* the documents that are intended to be deleted 2. upsert all new documents --- packages/wallet/package.json | 3 +- .../pouchDbStores/PouchDbCollectionStore.ts | 36 +++++++++++++++---- .../persistence/pouchDbStores/PouchDbStore.ts | 2 +- .../pouchDbStores/pouchDbWalletStores.ts | 6 +++- .../test/persistence/pouchDbStores.test.ts | 14 ++++++++ yarn.lock | 1 + 6 files changed, 52 insertions(+), 10 deletions(-) diff --git a/packages/wallet/package.json b/packages/wallet/package.json index 9f8c041de46..0aef37af0c0 100644 --- a/packages/wallet/package.json +++ b/packages/wallet/package.json @@ -81,7 +81,8 @@ "pouchdb": "^7.3.0", "rxjs": "^7.4.0", "ts-custom-error": "^3.2.0", - "ts-log": "^2.2.3" + "ts-log": "^2.2.3", + "uuid": "^8.3.2" }, "files": [ "dist/*", diff --git a/packages/wallet/src/persistence/pouchDbStores/PouchDbCollectionStore.ts b/packages/wallet/src/persistence/pouchDbStores/PouchDbCollectionStore.ts index 713ae5edebb..3a9bacc184c 100644 --- a/packages/wallet/src/persistence/pouchDbStores/PouchDbCollectionStore.ts +++ b/packages/wallet/src/persistence/pouchDbStores/PouchDbCollectionStore.ts @@ -4,6 +4,7 @@ import { Logger } from 'ts-log'; import { PouchDbStore } from './PouchDbStore'; import { observeAll } from '../util'; import { sanitizePouchDbDoc } from './util'; +import { v4 } from 'uuid'; export type ComputePouchDbDocId = (doc: T) => string; @@ -14,7 +15,7 @@ export interface PouchDbCollectionStoreProps { /** PouchDB database that implements CollectionStore. Supports sorting by custom document _id */ export class PouchDbCollectionStore extends PouchDbStore implements CollectionStore { - readonly #computeDocId: ComputePouchDbDocId | undefined; + readonly #computeDocId: ComputePouchDbDocId; readonly #updates$ = new Subject(); observeAll: CollectionStore['observeAll']; @@ -29,7 +30,7 @@ export class PouchDbCollectionStore extends PouchDbStore implem // Using a db per collection super(dbName, logger); this.observeAll = observeAll(this, this.#updates$); - this.#computeDocId = computeDocId; + this.#computeDocId = computeDocId ?? (() => v4()); } getAll(): Observable { @@ -50,13 +51,34 @@ export class PouchDbCollectionStore extends PouchDbStore implem return from( (this.idle = this.idle.then(async (): Promise => { try { - await this.clearDB(); + const newDocsWithId = docs.map((doc) => ({ + ...this.toPouchDbDoc(doc), + _id: this.#computeDocId(doc) + })); + const existingDocs = await this.fetchAllDocs(); + const newDocsWithRev = newDocsWithId.map((newDoc): T & { _id: string; _rev?: string } => { + const existingDoc = existingDocs.find((doc) => doc.id === newDoc._id); + if (!existingDoc) return newDoc; + return { + ...newDoc, + _rev: existingDoc.value.rev + }; + }); + const docsToDelete = existingDocs.filter( + (existingDoc) => !newDocsWithId.some((newDoc) => newDoc._id === existingDoc.id) + ); await this.db.bulkDocs( - docs.map((doc) => ({ - ...this.toPouchDbDoc(doc), - _id: this.#computeDocId?.(doc) - })) + docsToDelete.map( + ({ id, value: { rev } }) => + ({ + _deleted: true, + _id: id, + _rev: rev + } as unknown as T) + ) ); + await this.db.bulkDocs(newDocsWithRev); + this.#updates$.next(docs); } catch (error) { this.logger.error(`PouchDbCollectionStore(${this.dbName}): failed to setAll`, docs, error); diff --git a/packages/wallet/src/persistence/pouchDbStores/PouchDbStore.ts b/packages/wallet/src/persistence/pouchDbStores/PouchDbStore.ts index 43af670d4c5..ab767bdd93d 100644 --- a/packages/wallet/src/persistence/pouchDbStores/PouchDbStore.ts +++ b/packages/wallet/src/persistence/pouchDbStores/PouchDbStore.ts @@ -9,7 +9,7 @@ export abstract class PouchDbStore { destroyed = false; protected idle: Promise = Promise.resolve(); protected readonly logger: Logger; - protected readonly db: PouchDB.Database; + public readonly db: PouchDB.Database; constructor(public dbName: string, logger: Logger) { this.logger = logger; diff --git a/packages/wallet/src/persistence/pouchDbStores/pouchDbWalletStores.ts b/packages/wallet/src/persistence/pouchDbStores/pouchDbWalletStores.ts index 60e8ed121d8..3a19c5f420b 100644 --- a/packages/wallet/src/persistence/pouchDbStores/pouchDbWalletStores.ts +++ b/packages/wallet/src/persistence/pouchDbStores/pouchDbWalletStores.ts @@ -85,7 +85,11 @@ export const createPouchDbWalletStores = ( // However it is extremely unlikely that it would have inline datums, // and renaming this store is not an option as it's being used // for storing collateral settings - unspendableUtxo: new PouchDbUtxoStore({ dbName: `${baseDbName}UnspendableUtxo` }, logger), + unspendableUtxo: new PouchDbUtxoStore( + // random doc id; setAll will always delete and re-insert all docs + { dbName: `${baseDbName}UnspendableUtxo` }, + logger + ), utxo: new PouchDbUtxoStore({ dbName: `${baseDbName}Utxo_v2` }, logger), volatileTransactions: new PouchDbVolatileTransactionsStore(docsDbName, 'volatileTransactions_v3', logger) }; diff --git a/packages/wallet/test/persistence/pouchDbStores.test.ts b/packages/wallet/test/persistence/pouchDbStores.test.ts index 6ad588d7ce5..580019fff7e 100644 --- a/packages/wallet/test/persistence/pouchDbStores.test.ts +++ b/packages/wallet/test/persistence/pouchDbStores.test.ts @@ -82,6 +82,20 @@ describe('pouchDbStores', () => { expect(await firstValueFrom(store1.getAll())).toEqual([doc2]); }); + it('setAll preserves existing documents if bulkDocs fails', async () => { + await firstValueFrom(store1.setAll([doc1])); + const originalBulkDocs = store1.db.bulkDocs.bind(store1.db); + // 1st call used to clearAll, 2nd call was used to insert new docs. + // after the fix, 1st only deletes the documents that are intended to be deleted + store1.db.bulkDocs = jest.fn().mockImplementationOnce(originalBulkDocs).mockRejectedValueOnce(new Error('fail')); + + // attempting to insert doc2 + await firstValueFrom(store1.setAll([doc1, doc2])); + + const docs = await firstValueFrom(store1.getAll()); + expect(docs.length).toBeGreaterThan(0); + }); + it('simultaneous setAll() calls are resolved in series - last value is always persisted', async () => { await firstValueFrom( combineLatest([store1.setAll([doc1]), timer(1).pipe(mergeMap(() => store1.setAll([doc2])))]) diff --git a/yarn.lock b/yarn.lock index 544468670f6..e3ba38b8aea 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4085,6 +4085,7 @@ __metadata: ts-log: ^2.2.3 tsc-alias: ^1.8.10 typescript: ^4.7.4 + uuid: ^8.3.2 wait-on: ^6.0.1 webextension-polyfill: ^0.9.0 languageName: unknown