Splits findOrAttach file into 2 functions

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<AddFileResult> {
     // 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;
   }

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<boolean> {
     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<boolean> {
+    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<boolean> {
+    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<void> {
+    await db.run('UPDATE _gristsys_Files SET data=?, storageId=? WHERE ident=?', fileData, storageId, fileIdent);
+  }
 }
 
 interface RebuildResult {

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<DocStorage, 'docName' | 'getFileInfo' | 'findOrAttachFile' | 'listAllFiles'>
+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<boolean> {
-    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<boolean> {
+    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;
   }
 }
 

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));