Adds initial support for external attachments using the Filesystem or MinIO #1320

Spoffy · 2024-11-24T19:00:35Z

This PR is still being worked on, but open to initial review.

Context

When attachments are uploaded to Grist, they're stored in an SQLite table within the document.

The result is that large files (multi-GB) substantially increase the size of the SQLite file, and cause performance problems (for example, with snapshots).

Proposed solution

The current design / full scope of changes can be seen in this Google document..

This PR contains a minimal proof of concept for external attachments, with the following features:

External attachment storage using either MinIO or the local filesystem.
A "default attachment store" setting for documents, which sets the destination for newly uploaded attachments.
Defaulting to existing behaviour. Without additional configuration, Grist's attachment storage behaviour is unchanged.
MinIO attachment storage is automatically made available if it's configured for document storage, but not automatically used by documents.

No UI exists for configuring anything related to attachment storage at this time. To use a store, the API or an SQLite client needs using to set the idOfDefaultAttachmentStore document setting to the ID of the desired attachment store. Attachment store IDs are logged to the console.

Currently only a single store of each backend type (minio, filesystem) can exist, as they're automatically configured using the existing setting.

Related issues

#1021

Review guidance / code overview

Three significant components are added in this PR, with the rest of the PR being generally glue around them:

AttachmentFileManager (in "AttachmentFileManager.ts") handles attachment fetching / saving logic on behalf of ActiveDoc
An IAttachmentStore is an instantiated store, that handles saving / loading attachments from a specific provider.
AttachmentStoreProvider is used to resolve store IDs to stores, and is the main way of providing other components (e.g AttachmentFileManager) with access to store instances.

The available types of attachment store are defined in coreCreator.ts, under the name "attachmentStoreBackends".

Additionally:

A few existing classes have been extended with streaming methods, to simplify interaction with the existing attachment code and to facilitate large files in the future without keeping copies in memory.
Tests may not pass yet, and additional unit tests are WIP.

Migrations / schema changes

One migration to _gristsys_files has been added, which adds a column to track which store a document is in.
idOfDefaultAttachmentStore has been added to the DocumentSettings JSON blob.

Both of these should be non-breaking on older installations, as both should be ignored.

Has this been tested?

Behaviour has been manually tested in the following ways:

Filesystem storage
MinIO client with MinIO storage
MinIO client with S3 storage

There are currently no unit tests. These should be added before merging, but the goal.

Recommended local testing setup

MinIO

Set up Grist to use MinIO normally.
Start Grist
Retrieve the attachment store id from the console.
Download the Grist document.
Open the Grist document in a database client, and add "idOfDefaultAttachmentStore": "store id" to _grist_DocInfo -> documentSettings
Upload the Grist document
Try adding an attachment to the new document. The name should show up in MinIO's storage path on the local filesystem.

Known issues

When using S3 storage, attachments showed as broken until refreshing the page.

Avoids returning `undefined`, which simplifies a lot of the parent logic.

Helps document and prevent programmer errors by adding a utility function to turn it into a docId.

paulfitz

Just a batch of superficial comments from a skim. Feels like it is coming along pretty well!

paulfitz · 2024-11-25T15:25:19Z

app/common/DocumentSettings.ts

@@ -2,6 +2,7 @@ export interface DocumentSettings {
  locale: string;
  currency?: string;
  engine?: EngineCode;
+  idOfDefaultAttachmentStore?: string;


Small nit: there's a lot of places in Grist where something could be called idOfFoo and is instead called fooId. I think that would make sense here too to rhyme with the rest of the codebase, defaultAttachmentStoreId.

(attachmentStoreId would be fine too, locale and currency are also defaults but we don't spell that out)

My motivation for phrasing it this way specifically was the ambiguity of the variable if it ends in id.

It could be:
defaultAttachmentStore Id
or
default attachmentStoreId

Which are semantically different interpretations of the same name! I'd be happy to swap it around if you think defaultAttachmentStoreId is clear enough though, despite the above :)

attachmentStoreId is my favorite now I'm afraid

Good enough for me - it's less clear for purpose, but at least resolves the ambiguity. I wasn't a big fan of idOf anyway. 🙂

app/server/lib/ActiveDoc.ts

paulfitz · 2024-11-25T15:30:34Z

app/server/lib/ActiveDoc.ts

@@ -2822,17 +2841,21 @@ export class ActiveDoc extends EventEmitter {
    return this._dataEngine;
  }

+  private async _getDocumentSettings(): Promise<DocumentSettings | undefined> {


If there are no settings, should that be an error? In what circumstances might that happen?

Ah, I see the method gets used in a migration. That answers that. Might still make sense to throw an error, but accept that error as a non-erroneous possibility in the migration. Feels like in all other circs it should be an error? No big deal.

I can switch this to throwing an error, then wrap the code below in _makeEngine in a try-catch block to handle this case?

that'd be fine

app/server/lib/AttachmentFileManager.ts

paulfitz · 2024-11-25T15:48:16Z

app/server/lib/AttachmentStore.ts

+ * Gets the correct pool id for a given document, given the document's id and trunk id.
+ * Avoids other areas of the codebase having to understand how documents are mapped to pools.
+ * This is a key security point - documents which share a pool can access each others' attachments.
+ * Therefore documents with different security domains (e.g from different teams) need to be sure not to share


It would be good now to talk about our strategy, for snapshots and forks to have the same pool id and share attachments?

paulfitz · 2024-11-25T17:48:29Z

app/server/lib/AttachmentStore.ts

+ * This is a general-purpose interface that should abstract over many different storage providers,
+ * so shouldn't have methods which rely on one the features of one specific provider.
+ *
+ * This is distinct from `ExternalStorage`, in that it's more generic and doesn't support versioning.


Hmm "more generic" feels wrong here, since an interface mentioning docPoolIds: DocPoolId vs key: string feels much more specific in some ways. Not many things in the world have DocPoolIds. Maybe drop that text?

paulfitz · 2024-11-25T17:56:07Z

app/server/lib/AttachmentStore.ts

+export class MinIOAttachmentStore implements IAttachmentStore {
+  constructor(
+    public id: string,
+    private _storage: MinIOExternalStorage,


So the reason this is using MinIOExternalStorage specifically and won't work with S3ExternalStorage or AzureExternalStorage is new Stream methods, correct? It would still help I think to anticipate that developers will add those methods for those types of storage. It is totally fine just to implement for MinIO here, since this is core, but ideally the follow up work needed would be adding the methods and making small updates, and not doing a significant refactor. Trying to bear in mind for reminder of review to watch for specific dependencies on MinIO creeping in.

Read rest of PR. Feels fine, no MinIO creep 👍

100% - I can't remember if I left the "Make an interface for streamable external storage" in a TODO or not somewhere, but it's planned, so we can turn "MinIOAttachmentStore" into "ExternalStorageAttachmentStore"

This is done now - any external storage should be usable, if wrapped in ExternalStorageAttachmentStore, and made to implement the streaming interface

paulfitz · 2024-11-25T17:57:54Z

app/server/lib/AttachmentStoreProvider.ts

+  storeExists(id: AttachmentStoreId): Promise<boolean>;
+}
+
+export interface IAttachmentStoreBackendFactory {


Extreme Java code vibes just here. If you search your heart, can you find a shorter name? I do see what you're going for.

I looked deep into my heart and gave it a shot, let me know if this is better!

thanks :) yes

app/server/lib/AttachmentStoreProvider.ts

Spoffy · 2024-11-26T01:38:28Z

app/server/lib/AttachmentFileManager.ts

+  }
+
+  public async getFileData(fileIdent: string): Promise<Buffer | null> {
+    this._log.debug({ fileIdent }, "retrieving file data");


I'm concerned this logging might be a bit much. When a document is opened, it loads attachment previews. That results in 1 or 2 log lines for every attachment in a document.

That might indeed be a bit much.

Removed 1 of the log lines - the other one is quite useful for debugging as it mentions the attachment pool, and which storage (document / external) the file is going into.

test/server/lib/HostedStorageManager.ts

# Conflicts: # app/server/generateInitialDocSql.ts # app/server/lib/ActiveDoc.ts # app/server/lib/DocApi.ts # app/server/lib/FlexServer.ts # app/server/lib/coreCreator.ts # test/server/lib/HostedStorageManager.ts

paulfitz

Is copying of documents with external attachments addressed? Might have missed it. If not, would want to be sure we don't miss that in follow-up.

paulfitz · 2024-12-12T20:08:20Z

app/server/lib/ActiveDoc.ts

@@ -636,6 +648,7 @@ export class ActiveDoc extends EventEmitter {
    useExisting?: boolean,       // If set, document can be created as an overlay on
                                 // an existing sqlite file.
  }): Promise<ActiveDoc> {
+    console.log(`OPENOPENOPENOPENOPENOPENOPENOPEN ${this.docName} ANYTHING ELSE`);


Stray debugging message?

Yup, this and others. Thanks for catching this 😅

paulfitz · 2024-12-12T20:08:57Z

app/server/lib/ActiveDoc.ts

@@ -388,6 +392,14 @@ export class ActiveDoc extends EventEmitter {
      loadTable: this._rawPyCall.bind(this, 'load_table'),
    });

+    // This will throw errors if _doc or externalAttachmentStoreProvider aren't provided, and ActiveDoc tries to use


what is _doc?

paulfitz · 2024-12-12T20:12:35Z

app/server/lib/AttachmentFileManager.ts

+}
+
+/**
+ * Instantiated on a per-document to basis to provide a document with access to its attachments.


strike "to" in "to basis"?

paulfitz · 2024-12-12T20:17:51Z

app/server/lib/AttachmentFileManager.ts

+    // TODO - Decide if we keep an entry in SQLite after an upload error or not. Probably not?
+    await store.upload(this._getDocPoolId(), fileIdent, Readable.from(fileData));
+
+    // TODO - Confirm in doc storage that it's successfully uploaded? Need to decide how to handle a failed upload.


Since external storage is not entirely under our control (an admin might reasonably delete some files there) we need to degrade gracefully if some attachments are mysteriously missing from expected location. Presumably that means that even without this TODO, a failure is non-catastrophic in the sense that the document will continue to work.

It's not well explained in this PR, but the transfer PR cleans up these errors, and explicitly re-orders operations to ensure we're always in a valid state (if the upload fails, the whole "Upload an attachment" flow fails, and no entries go into the DB).

In the case that the file isn't accessible in external storage, it should show that the attachment isn't available in the UI. I think that's handled on the clientside when the API returns an error response?

paulfitz · 2024-12-12T20:18:42Z

app/server/lib/AttachmentStore.ts

+type FileId = string;
+
+
+// Minimum required info from a document


"Minimum required info from a document" for what purpose?

paulfitz · 2024-12-12T20:20:59Z

app/server/lib/AttachmentStore.ts

+ * Generally, the pool id of a document should be its trunk id if available (because it's a fork),
+ * or the document's id (if it isn't a fork).
+ *
+ * This means that attachments need copying to a new pool when a document is copied.


I don't think we've talked explicitly about the consequences of document copying now being a potentially slow operation. As long as a copy fails cleanly if there's a problem, I think that would be the main thing.

I don't think we have either - might be worth a chat?

I think the copy behaviour is missing from this PR to be honest (:grimacing:). It might be worth opening as a separate PR at this point?

paulfitz · 2024-12-12T20:24:55Z

app/server/lib/AttachmentStoreProvider.ts

+    _installationUuid: string
+  ) {
+    // In the current setup, we automatically generate store IDs based on the installation ID.
+    // The installation ID is guaranteed to be unique, and we only allow one store of each backend type.


we only allow one store of each backend type - I thought we agreed to move on from this as too restrictive? It makes no sense to me to allow say an s3 bucket and an azure bucket but then to kick up a fuss if the azure bucket is replaced with a distinct s3 bucket.

I remember discussing permitting a small label that would take precedence over storeSpec.name if given.

We've since discussed this, and will use a "JSON in environment variable" approach (adding comment here for posterity)

Spoffy added 19 commits October 27, 2024 17:13

Enables checksumFile to handle streams

2473375

Routes attachment handling through AttachmentFileManager

ff2fe69

Switches ActiveDoc to using AttachmentFileManager for attachments

fc35744

Fixes a casing error in DocStorage

ef13def

Adds working filesystem store (in use by default)

d19ff54

Avoids storing external attachments in the sqlite file

8295f5e

Renames some parameters in AttachmentStore

8f09573

Adds "removeAllWithPrefix?" to ExternalStorage and MinIOExternalStorage

72f2e5c

Adds a streaming API to MinIOExternalStorage

5e26ec8

Adds prototype MinIO attachment store

20a1a79

Makes AttachmentStoreProvider function correctly

482b5c7

Enables setting document attachment store

59d1633

Makes store creation error if not setup

c1058da

Avoids returning `undefined`, which simplifies a lot of the parent logic.

Refactors DocInfo to be reusable

74102d0

Helps document and prevent programmer errors by adding a utility function to turn it into a docId.

Fixes pool deletion not working for MinIO

6640133

Adds lifecycle management to external attachments

85ba845

Adds attachment upload/download logging

3902f82

Fixes test build errors

4085c06

Misc. cleanup for external attachments.

65bc7a9

paulfitz reviewed Nov 25, 2024

View reviewed changes

Spoffy mentioned this pull request Nov 25, 2024

Externalized attachments #1021

Open

Spoffy added 8 commits November 25, 2024 22:40

Fixes import ordering issues

ce5282f

Improves documentation of AttachmentFileManager

e53f6eb

Switches AttachmentFileManager to structured logging

819e0d6

Tidies up imports

52b7e24

Adds doc pool description to AttachmentStore

79396dc

Improves naming of attachment store backend options

d527e59

Adds explanation on store concepts

c3c82bd

Changes MinIOAttachmentStore to ExternalStorageAttachmentStore

dc19e54

Spoffy commented Nov 26, 2024

View reviewed changes

Spoffy added 8 commits December 2, 2024 10:35

Makes attachment store creation async, removes check methods

6af1818

Adds tests for AttachmentStoreProvider

8ca7196

Fixes type for FileInfo.storageId

2d59def

Adds listAllStoreIds to IAttachmentStoreProvider

fc9db0c

Fixes typing on AttachmentFileManagerLogInfo

72eecf3

Make makeTestingFilesystemStoreSpec reuse the directory

cdcfb0d

Adds AttachmentFileManager unit tests

0bddba2

Adds attachment store backend availability checks

b9d2f8c

Spoffy marked this pull request as ready for review December 3, 2024 18:47

Spoffy commented Dec 3, 2024

View reviewed changes

test/server/lib/HostedStorageManager.ts Outdated Show resolved Hide resolved

jordigh self-assigned this Dec 3, 2024

Spoffy and others added 15 commits December 3, 2024 23:07

Merge branch 'main' into spoffy/external-attachments-prototype

e5b508c

# Conflicts: # app/server/generateInitialDocSql.ts # app/server/lib/ActiveDoc.ts # app/server/lib/DocApi.ts # app/server/lib/FlexServer.ts # app/server/lib/coreCreator.ts # test/server/lib/HostedStorageManager.ts

Cleans up linter issues

f712ef8

Fixes findOrAttachFile test

364ac08

Removes unnecessary log line

ae78bba

Makes _getDocumentSettings error if not found.

4730d99

Undoes erroneous storage schema change

c39806f

Fix getDocumentSettings bad error condition

58a101d

Merge branch 'main' into spoffy/external-attachments-prototype

785971f

Updates migration tests with additional fixtures

f93948f

Fixes an error with missing storageId in doc schema

a4768a5

Attempt to change BlobMigrationV8 to V9 using upgradeDocument

c41b4db

Updates BlobMigrationV9 and associated tooling

f0222ca

Fixes failing DocStorage tests

3008fdd

Renames "idOfDefaultAttachmentStore" to "attachmentStoreId"

4a1c112

Makes getFileData throw an error if file is missing

adac94b

paulfitz reviewed Dec 12, 2024

View reviewed changes

paulfitz requested a review from jordigh December 19, 2024 13:51

paulfitz added the Attachments label Dec 19, 2024

Updates comments to address feedback

3b0f603

		type FileId = string;


		// Minimum required info from a document

Adds initial support for external attachments using the Filesystem or MinIO #1320

Are you sure you want to change the base?

Adds initial support for external attachments using the Filesystem or MinIO #1320

Conversation

Spoffy commented Nov 24, 2024 • edited Loading

Context

Proposed solution

Related issues

Review guidance / code overview

Migrations / schema changes

Has this been tested?

Recommended local testing setup

MinIO

Known issues

paulfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Spoffy Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Spoffy commented Nov 24, 2024 •

edited

Loading

Spoffy Nov 27, 2024 •

edited

Loading

Spoffy Nov 25, 2024 •

edited

Loading

Spoffy Dec 4, 2024 •

edited

Loading

Spoffy Dec 30, 2024 •

edited

Loading

Spoffy Dec 30, 2024 •

edited

Loading