From bf72ef07f167a4b32dcda28ef09b9a53ceae31ea Mon Sep 17 00:00:00 2001 From: Daniel Meyer <8926560+pubkey@users.noreply.github.com> Date: Mon, 25 Nov 2024 11:22:33 +0100 Subject: [PATCH] REFACTOR bulkRemove (#6634) * REFACTOR bulkRemove * FIX docs --- docs-src/docs/rx-collection.md | 9 ++++ src/rx-collection.ts | 30 ++++++++++--- src/rx-document.ts | 82 +++++----------------------------- src/rx-query.ts | 30 ++++++------- 4 files changed, 58 insertions(+), 93 deletions(-) diff --git a/docs-src/docs/rx-collection.md b/docs-src/docs/rx-collection.md index 5297c36089f..87a86afa67c 100644 --- a/docs-src/docs/rx-collection.md +++ b/docs-src/docs/rx-collection.md @@ -125,6 +125,15 @@ const result = await myCollection.bulkRemove([ // } ``` +Instead of providing the document ids, you can also use the [RxDocument](./rx-document.md) instances. This can have better performance if your code knows them already at the moment of removing them: +```js +const result = await myCollection.bulkRemove([ + myRxDocument1, + myRxDocument2, + /* ... */ +]); +``` + ### upsert() Inserts the document if it does not exist within the collection, otherwise it will overwrite it. Returns the new or overwritten RxDocument. ```js diff --git a/src/rx-collection.ts b/src/rx-collection.ts index 04c4c52aa0f..e21aac3b615 100644 --- a/src/rx-collection.ts +++ b/src/rx-collection.ts @@ -436,7 +436,12 @@ export class RxCollectionBase< } async bulkRemove( - ids: string[] + /** + * You can either remove the documents by their ids + * or by directly providing the RxDocument instances + * if you have them already. This improves performance a bit. + */ + idsOrDocs: string[] | RxDocument[] ): Promise<{ success: RxDocument[]; error: RxStorageWriteError[]; @@ -447,14 +452,21 @@ export class RxCollectionBase< * Optimization shortcut, * do nothing when called with an empty array */ - if (ids.length === 0) { + if (idsOrDocs.length === 0) { return { success: [], error: [] }; } - const rxDocumentMap = await this.findByIds(ids).exec(); + let rxDocumentMap: Map>; + if (typeof idsOrDocs[0] === 'string') { + rxDocumentMap = await this.findByIds(idsOrDocs as string[]).exec(); + } else { + rxDocumentMap = new Map(); + (idsOrDocs as RxDocument[]).forEach(d => rxDocumentMap.set(d.primary, d)); + } + const docsData: RxDocumentData[] = []; const docsMap: Map> = new Map(); Array.from(rxDocumentMap.values()).forEach(rxDocument => { @@ -488,7 +500,14 @@ export class RxCollectionBase< removeDocs, results ); - const successIds: string[] = success.map(d => d[primaryPath] as string); + + const deletedRxDocuments: RxDocument[] = []; + const successIds: string[] = success.map(d => { + const id = d[primaryPath] as string; + const doc = this._docCache.getCachedRxDocument(d); + deletedRxDocuments.push(doc); + return id; + }); // run hooks await Promise.all( @@ -502,10 +521,9 @@ export class RxCollectionBase< }) ); - const rxDocuments = successIds.map(id => getFromMapOrThrow(rxDocumentMap, id)); return { - success: rxDocuments, + success: deletedRxDocuments, error: results.error }; } diff --git a/src/rx-document.ts b/src/rx-document.ts index 925dd171922..730167b7b2f 100644 --- a/src/rx-document.ts +++ b/src/rx-document.ts @@ -17,7 +17,6 @@ import { RXJS_SHARE_REPLAY_DEFAULTS, getProperty, getFromMapOrCreate, - getFromMapOrThrow, ensureNotFalsy } from './plugins/utils/index.ts'; import { @@ -34,9 +33,7 @@ import type { RxDocumentWriteData, UpdateQuery, CRDTEntry, - ModifyFunction, - BulkWriteRow, - RxStorageWriteError + ModifyFunction } from './types/index.d.ts'; import { getDocumentDataOfRxChangeEvent } from './rx-change-event.ts'; import { overwritable } from './overwritable.ts'; @@ -371,7 +368,6 @@ export const basePrototype = { * instead deleted documents get flagged with _deleted=true. */ async remove(this: RxDocument): Promise { - const collection = this.collection; if (this.deleted) { return Promise.reject(newRxError('DOC13', { document: this, @@ -379,13 +375,17 @@ export const basePrototype = { })); } - const removeResult = await removeRxDocuments([this]); - if (removeResult.errors.length > 0) { - const error = removeResult.errors[0]; - const deletedData = getFromMapOrThrow(removeResult.deleteDataById, this.primary); - throwIfIsStorageWriteError(collection, this.primary, deletedData, error); + const removeResult = await this.collection.bulkRemove([this]); + if (removeResult.error.length > 0) { + const error = removeResult.error[0]; + throwIfIsStorageWriteError( + this.collection, + this.primary, + this._data, + error + ); } - return removeResult.docs[0]; + return removeResult.success[0]; }, incrementalRemove(this: RxDocument): Promise { return this.incrementalModify(async (docData) => { @@ -537,63 +537,3 @@ function getDocumentProperty(doc: RxDocument, objPath: string): any | null { } ); }; - - - -/** - * To remove documents we use a bulk operations - * to have a better performance on RxQuery.find().remove(); - */ -export async function removeRxDocuments( - docs: RxDocument[] -): Promise<{ - errors: RxStorageWriteError[]; - docs: RxDocument[]; - deleteDataById: Map>; -}> { - if (docs.length === 0) { - throw newRxError('SNH'); - } - const collection = docs[0].collection; - const primaryPath = collection.schema.primaryPath; - const writeRows: BulkWriteRow[] = []; - const docsById = new Map>(); - const deleteDataById = new Map>(); - await Promise.all( - docs.map(async (doc) => { - docsById.set(doc.primary, doc); - const deletedData = flatClone(doc._data); - await collection._runHooks('pre', 'remove', deletedData, doc); - deletedData._deleted = true; - deleteDataById.set(doc.primary, deletedData); - const writeRow = { - previous: doc._data, - document: deletedData - }; - writeRows.push(writeRow); - }) - ); - const writeResult = await collection.storageInstance.bulkWrite(writeRows, 'rx-document-remove'); - const errors: RxStorageWriteError[] = writeResult.error; - const writtenDocsData = getWrittenDocumentsFromBulkWriteResponse( - primaryPath, - writeRows, - writeResult - ); - const returnDocs: RxDocument[] = []; - await Promise.all( - writtenDocsData.map(async (writtenDocData) => { - const id = writtenDocData[primaryPath] as string; - const doc = getFromMapOrThrow(docsById, id); - const deletedData = getFromMapOrThrow(deleteDataById, id); - await collection._runHooks('post', 'remove', deletedData, doc); - returnDocs.push(collection._docCache.getCachedRxDocument(writtenDocData)); - }) - ); - - return { - docs: returnDocs, - errors, - deleteDataById - }; -} diff --git a/src/rx-query.ts b/src/rx-query.ts index 3061e5518d4..65757019933 100644 --- a/src/rx-query.ts +++ b/src/rx-query.ts @@ -23,7 +23,8 @@ import { appendToArray } from './plugins/utils/index.ts'; import { - newRxError + newRxError, + rxStorageWriteErrorToRxError } from './rx-error.ts'; import { runPluginHooks @@ -53,7 +54,6 @@ import { } from './rx-query-helper.ts'; import { RxQuerySingleResult } from './rx-query-single-result.ts'; -import { removeRxDocuments } from './rx-document.ts'; let _queryCount = 0; const newQueryID = function (): number { @@ -379,20 +379,18 @@ export class RxQueryBase< * deletes all found documents * @return promise with deleted documents */ - remove(): Promise { - return this - .exec() - .then(docs => { - if (Array.isArray(docs)) { - if (docs.length === 0) { - return []; - } else { - return removeRxDocuments(docs).then(r => r.docs); - } - } else { - return (docs as any).remove(); - } - }); + async remove(): Promise { + const docs = await this.exec(); + if (Array.isArray(docs)) { + const result = await this.collection.bulkRemove(docs); + if (result.error.length > 0) { + throw rxStorageWriteErrorToRxError(result.error[0]); + } else { + return result.success as any; + } + } else { + return (docs as any).remove(); + } } incrementalRemove(): Promise { return runQueryUpdateFunction(