Adds transfers between stores to external attachments

[Spoffy]

Currently a draft, pending final changes. Opened here for initial feedback.

Context

This PR follows on from #1320, and adds support for moving attachments between stores.

This allows a user to change the document's default store, then transfer all of the attachments from their current store(s) to the new default.

This includes transfers from internal (SQLite) storage to external storage, external to internal, and external to external (e.g MinIO to filesystem).

User-facing changes:

• Adds an API endpoint to migrate all current attachments to the default store set for that document

Internal changes:

• Adds methods to AttachmentFileManager to facilitate transfers from one storage to another.
• Exposes new methods on ActiveDoc for triggering transfers and retrieving transfer status.

All of the logic behind these changes should be documented in the source code with comments - if anything is unclear, that would be great feedback.

There a few large block comments (particularly in AttachmentFileManager) that might be a good place to start when reading this code.

Related issues

#1320
#1021

Has this been tested?

• 👍 yes, I added tests to the test suite
• 💭 no, because this PR is a draft and still needs work (some tests missing)

[Spoffy]

This isn't actually used anywhere. I've left it in for now (since it's already implemented), but we could remove it?

[fflorent]

Great, that looks promising :).

Here are my remarks so far (I need to continue reading the code and start looking at your tests).

[fflorent]

I am usually not very comfortable with boolean flag params, due to their lack of expressivity.

But am I right to say the shouldUpdate may be the default? I only see the flag is kept to false only in tests.

In any case, I would suggest for adding more expressivity the following, if that makes sense to you too:

• make this function private;
• introducing 2 new functions named updateOrAttachFile (where you call the private function with shouldUpdate = true) and attachFileIfNew (with shouldUpdate = false);
• maybe (or maybe not, that's arguable if this is now private) change shouldUpdate: boolean into { shouldUpdate = false }: {shouldUpdate: boolean } = {} or similar;

[Spoffy]

Interesting thoughts here! I thought a lot about how to split this - I don't really like the flag either, but ended up in the "tangled implementation" case of that article you linked! 🙂

I'd be happy to do it the way you suggest - make the flagged method private, and expose it as two different public methods. It absolutely feels easier to read.

[Spoffy]

I split this up fully, and separated out the implementation in a way that I think it makes it easier to read. 🙂

Let me know if that works for you!

[fflorent]

The comment that used to be line 793 gave some reason that is still useful, I think. So I would add some more context:

Suggested change

	// Try to insert a new record with the given ident. It'll fail UNIQUE constraint if exists.
	// Try to insert a new record with the given ident. It'll fail UNIQUE constraint if exists.
	// Even when attempting to attach a new file exclusively (and do nothing when it exists),
	// it's a good idea to first check the existence of the fileIdent and if not insert the file and its data

[Spoffy]

I'm not sure if this is still relevant after the other changes made to this function? :)

[fflorent]

The code is acceptable as it is for me, so please skip if you don't agree much about my idea.

I feel like, even it introduces a supplementary SQL query to maintain, we would increase the readability by separating the query to check the existence of the file and do the INSERT/UPDATE. Am I right to say the below code would be equivalent?

const exists = await db.run('SELECT 1 from _gristsys_Files where ident = ?', fileIdent); // Not very sure what is returned here
if (exists && shouldUpdate) {
  await db.run('UPDATE _gristsys_Files SET data=?, storageId=? WHERE ident=?', fileData, storageId, fileIdent);
} else {
  await db.run('INSERT INTO _gristsys_Files(ident, data, storageId) VALUES (?, ?, ?)', fileIdent, fileData, storageId);
}
return exists;

[Spoffy]

These look equivalent to me. I'm not sure why it was originally done by catching the unique constraint, but it looks like the only place in the code where that's done.

I'll make this change, and we'll see if @paulfitz or anyone has any opinion :)

Edit: Change is pushed

[fflorent]

What would you think of the following to avoid the boolean flag param:

  public getFileInfo(fileIdent: string): Promise<FileInfo | null> {
    return this._getFileInfo(fileIdent, 'ident, storageId');
  }
  public getFileInfoWithData(fileIdent: string): Promise<FileInfo | null> {
    return this._getFileInfo(fileIdent, 'ident, storageId, data');
  }
  // ...
  private _getFileInfo(fileIdent: string, columns: string): Promise<FileInfo | null> {
    return this.get(`SELECT ${columns} FROM _gristsys_Files WHERE ident=?`, fileIdent)
      .... // just like what you did
   }

[Spoffy]

I think splitting them up is a good idea - it definitely makes it more readable.

I think I'd rather duplicate the original function, than pass columns as a string.

That's because there's no risk of bad data accidentally being loaded into the code that formats the result into a typed object (e.g, a missing column).

Additionally, because it lowers the risk of someone doing something silly (such as passing a user-provided column name) in the future.

[Spoffy]

This is done, please let me know if the refactored version is clearer :)

[fflorent]

This function is a bit hard to read due to all of the conditions entangled. The good news is that it is private and you have a public method to call it, so you can rework it serenely.

I feel like it would be less complex to split this function into two different ones. I made the following, but have not tested:

  public async addFile(
    storeId: AttachmentStoreId | undefined,
    fileExtension: string,
    fileData: Buffer
  ): Promise<AddFileResult> {
    const fileIdent = await this._getFileIdentifier(fileExtension, Readable.from(fileData));
    return storeId ? 
      this._addFileToExternalStore(storeId, fileIdent, fileData) : 
      this._addFileToLocalStore(fileIdent, fileData);
  }


  // ...

  private async _addFileToExternalStore(
    destStoreId: AttachmentStoreId,
    fileIdent: string,
    fileData: Buffer
  ): Promise<AddFileResult> {
    const destStore = await this._getStore(destStoreId);
    if (!destStore) {
      this._log.warn({ fileIdent, storeId: destStoreId }, "tried to fetch attachment from an unavailable store");
      throw new StoreNotAvailableError(destStoreId);
    }
    const fileInfoNoData = await this._docStorage.getFileInfo(fileIdent, false);
    const fileExists = fileInfoNoData !== null;
    if (fileExists) {
      // File is already stored in a different store (e.g because store has changed and no migration has happened
      const isFileInTargetStore = destStoreId === fileInfoNoData.storageId;

      // Only exit early if the file is stored elsewhere of if the file exists in the store, 
      // otherwise we should allow users to fix any missing files
      // by proceeding to the normal upload logic.
      const fileAlreadyExistsInStore = isFileInTargetStore && await destStore.exists(this._getDocPoolId(), fileIdent);

      if (!isFileInTargetStore || fileAlreadyExistsInStore) {
        return {
          fileIdent,
          isNewFile: false,
        };
      }
    }
    // There's a possible race condition if anything changed the record between the initial checks
    // in this method, and the database being updated below - any changes will be overwritten.
    // However, the database will always end up referencing a valid file, and the pool-based file
    // deletion guarantees any files in external storage will be cleaned up eventually.
    await this._storeFileInAttachmentStore(destStore, fileIdent, fileData);

    return {
      fileIdent,
      isNewFile: !fileExists,
    };
  }

  private async _addFileToLocalStore(
    fileIdent: string,
    fileData: Buffer
  ): Promise<AddFileResult> {
    const fileInfoNoData = await this._docStorage.getFileInfo(fileIdent, false);
    const fileExists = fileInfoNoData !== null;
    if (!fileExists) {
      await this._storeFileInLocalStorage(fileIdent, fileData);
    }
    return {
      fileIdent,
      isNewFile: !fileExists,
    };
}

[Spoffy]

I've split this into two, and there's a few duplicate comments now - but I think that's reasonable, as it's definitely a lot easier to understand the logic now.

[fflorent]

Typo:

Suggested change

	// There's a possible race condition if anything changed the record between the initial checks
	// There's a possible race condition in anything changed the record between the initial checks

[Spoffy]

Not a typo here, but I'll clean up the comment to be more intelligible!

It's trying to say:

There's the potential for a race condition here, if any other code modifies the database record between the the checks at the start of this function, and this line of code. The checks could pass then, but fail if evaluated here (due to record changes).

[Spoffy]

Rephrased to this:

// A race condition can occur here, if the file's database record is modified between the
// `getFileInfoNoData` call earlier in this function and now.
// Any changes made after that point will be overwritten below.
// However, the database will always end up referencing a valid file, and the pool-based file
// deletion guarantees any files in external storage will be cleaned up eventually.