From d721c793e0f557f38216f9a2887e609066311cda Mon Sep 17 00:00:00 2001 From: Spoffy Date: Tue, 14 Jan 2025 05:12:39 +0000 Subject: [PATCH] Splits findOrAttach file into 2 functions --- app/server/lib/AttachmentFileManager.ts | 4 +- app/server/lib/DocStorage.ts | 68 +++++++++++++++++------- test/server/lib/AttachmentFileManager.ts | 23 ++++++-- test/server/lib/DocStorage.js | 2 +- 4 files changed, 71 insertions(+), 26 deletions(-) diff --git a/app/server/lib/AttachmentFileManager.ts b/app/server/lib/AttachmentFileManager.ts index eeb54c9fe1..2b324220d8 100644 --- a/app/server/lib/AttachmentFileManager.ts +++ b/app/server/lib/AttachmentFileManager.ts @@ -401,7 +401,7 @@ export class AttachmentFileManager implements IAttachmentFileManager { // Uploads the file to local storage, overwriting the current DB record for the file. private async _storeFileInLocalStorage(fileIdent: string, fileData: Buffer): Promise { // Insert (or overwrite) the entry for this file in the document database. - const isNewFile = await this._docStorage.findOrAttachFile(fileIdent, fileData, undefined, true); + const isNewFile = await this._docStorage.attachOrUpdateFile(fileIdent, fileData, undefined); return { fileIdent, @@ -418,7 +418,7 @@ export class AttachmentFileManager implements IAttachmentFileManager { await store.upload(this._getDocPoolId(), fileIdent, Readable.from(fileData)); // Insert (or overwrite) the entry for this file in the document database. - await this._docStorage.findOrAttachFile(fileIdent, undefined, store.id, true); + await this._docStorage.attachOrUpdateFile(fileIdent, undefined, store.id); return fileIdent; } diff --git a/app/server/lib/DocStorage.ts b/app/server/lib/DocStorage.ts index 02f5be5678..26746decb6 100644 --- a/app/server/lib/DocStorage.ts +++ b/app/server/lib/DocStorage.ts @@ -775,37 +775,46 @@ export class DocStorage implements ISQLiteDB, OnDemandStorage { * would be (very?) inefficient until node-sqlite3 adds support for incremental reading from a * blob: https://github.com/mapbox/node-sqlite3/issues/424. * - * @param {string} fileIdent - The unique identifier of the file in the database. ActiveDoc uses the - * checksum of the file's contents with the original extension. + * @param {string} fileIdent - The unique identifier of the file in the database. * @param {Buffer | undefined} fileData - Contents of the file. * @param {string | undefined} storageId - Identifier of the store that file is stored in. - * @param {boolean} shouldUpdate - Update the file record if found. * @returns {Promise[Boolean]} True if the file got attached; false if this ident already exists. */ public findOrAttachFile( fileIdent: string, fileData: Buffer | undefined, storageId?: string, - shouldUpdate: boolean = false, ): Promise { return this.execTransaction(async (db) => { - let isNewFile = true; - - try { - // Try to insert a new record with the given ident. It'll fail UNIQUE constraint if exists. - await db.run('INSERT INTO _gristsys_Files (ident) VALUES (?)', fileIdent); - } catch(err) { - // If UNIQUE constraint failed, this ident must already exist. - if (/^(SQLITE_CONSTRAINT: )?UNIQUE constraint failed/.test(err.message)) { - isNewFile = false; - } else { - throw err; - } - } - if (isNewFile || shouldUpdate) { - await db.run('UPDATE _gristsys_Files SET data=?, storageId=? WHERE ident=?', fileData, storageId, fileIdent); + const isNewFile = await this._addBasicFileRecord(db, fileIdent); + if (isNewFile) { + await this._updateFileRecord(db, fileIdent, fileData, storageId); } + return isNewFile; + }); + } + /** + * Attaches a file to the document, updating the file record if it already exists. + * + * TODO: This currently does not make the attachment available to the sandbox code. This is likely + * to be needed in the future, and a suitable API will need to be provided. Note that large blobs + * would be (very?) inefficient until node-sqlite3 adds support for incremental reading from a + * blob: https://github.com/mapbox/node-sqlite3/issues/424. + * + * @param {string} fileIdent - The unique identifier of the file in the database. + * @param {Buffer | undefined} fileData - Contents of the file. + * @param {string | undefined} storageId - Identifier of the store that file is stored in. + * @returns {Promise[Boolean]} True if the file got attached; false if this ident already exists. + */ + public attachOrUpdateFile( + fileIdent: string, + fileData: Buffer | undefined, + storageId?: string, + ): Promise { + return this.execTransaction(async (db) => { + const isNewFile = await this._addBasicFileRecord(db, fileIdent); + await this._updateFileRecord(db, fileIdent, fileData, storageId); return isNewFile; }); } @@ -1851,6 +1860,27 @@ export class DocStorage implements ISQLiteDB, OnDemandStorage { } return null; } + + private async _addBasicFileRecord(db: SQLiteDB, fileIdent: string): Promise { + try { + // Try to insert a new record with the given ident. It'll fail UNIQUE constraint if exists. + await db.run('INSERT INTO _gristsys_Files (ident, data, storageId) VALUES (?)', fileIdent); + } catch(err) { + // If UNIQUE constraint failed, this ident must already exist. + if (/^(SQLITE_CONSTRAINT: )?UNIQUE constraint failed/.test(err.message)) { + return false; + } else { + throw err; + } + } + return true; + } + + private async _updateFileRecord( + db: SQLiteDB, fileIdent: string, fileData?: Buffer, storageId?: string + ): Promise { + await db.run('UPDATE _gristsys_Files SET data=?, storageId=? WHERE ident=?', fileData, storageId, fileIdent); + } } interface RebuildResult { diff --git a/test/server/lib/AttachmentFileManager.ts b/test/server/lib/AttachmentFileManager.ts index bf3881fce3..0603381c9e 100644 --- a/test/server/lib/AttachmentFileManager.ts +++ b/test/server/lib/AttachmentFileManager.ts @@ -13,7 +13,10 @@ import * as sinon from "sinon"; import * as stream from "node:stream"; // Minimum features of doc storage that are needed to make AttachmentFileManager work. -type IMinimalDocStorage = Pick +type IMinimalDocStorage = Pick< + DocStorage, + 'docName' | 'getFileInfo' | 'findOrAttachFile' | 'attachOrUpdateFile' | 'listAllFiles' +> // Implements the minimal functionality needed for the AttachmentFileManager to work. class DocStorageFake implements IMinimalDocStorage { @@ -37,17 +40,29 @@ class DocStorageFake implements IMinimalDocStorage { // Needs to match the semantics of DocStorage's implementation. public async findOrAttachFile( - fileIdent: string, fileData: Buffer | undefined, storageId?: string | undefined, shouldUpdate: boolean = true + fileIdent: string, fileData: Buffer | undefined, storageId?: string | undefined ): Promise { - if (fileIdent in this._files && !shouldUpdate) { + if (fileIdent in this._files) { return false; } + this._setFileRecord(fileIdent, fileData, storageId); + return true; + } + + public async attachOrUpdateFile( + fileIdent: string, fileData: Buffer | undefined, storageId?: string | undefined + ): Promise { + const exists = fileIdent in this._files; + this._setFileRecord(fileIdent, fileData, storageId); + return !exists; + } + + private _setFileRecord(fileIdent: string, fileData: Buffer | undefined, storageId?: string | undefined) { this._files[fileIdent] = { ident: fileIdent, data: fileData ?? Buffer.alloc(0), storageId: storageId ?? null, }; - return true; } } diff --git a/test/server/lib/DocStorage.js b/test/server/lib/DocStorage.js index e956c56c94..d880486bc6 100644 --- a/test/server/lib/DocStorage.js +++ b/test/server/lib/DocStorage.js @@ -404,7 +404,7 @@ describe('DocStorage', function() { .then(fileInfo => assert.equal(fileInfo.data.toString('utf8'), correctFileContents)) // The update parameter should allow the record to be overwritten - .then(() => docStorage.findOrAttachFile("hello_world.txt", Buffer.from(replacementFileContents), undefined, true)) + .then(() => docStorage.attachOrUpdateFile("hello_world.txt", Buffer.from(replacementFileContents), undefined)) .then(result => assert.isFalse(result)) .then(() => docStorage.getFileInfo("hello_world.txt")) .then(fileInfo => assert.equal(fileInfo.data.toString('utf8'), replacementFileContents));