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!

app/common/DocumentSettings.ts

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

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 😅

Do you plan on removing it last minute? Just checking why it is still present.

Nah, likely an oversight on my part when doing the other bits of your feedback. Glad this wasn't ticked off at least!

app/server/lib/ActiveDoc.ts

app/server/lib/AttachmentFileManager.ts

app/server/lib/AttachmentStore.ts

app/server/lib/AttachmentStoreProvider.ts

jordigh

It's a fairly complex setup, but given the problem, I think you have convinced me that all of this complexity is required. I could not find many opportunities for simplification.

jordigh · 2024-12-20T05:05:06Z

app/server/lib/AttachmentStore.ts

+// Compatible with Document entity for ease of use
+export interface AttachmentStoreDocInfo {
+  id: string;
+  // This should NOT be an optional: the programmer should make an explicit choice for this value to prevent


So why make it optional if it shouldn't be?

It's inherently a nullable value - a document might not have trunkId. But it's not "optional" in the sense of typescript's "optional".

If I make it an optional (i.e typescripts ? syntax), code like this won't error:

getDocPoolIdFromDocInfo({ id: _someDoc.id })

The above is incorrect - and would result in attachments ending up in the wrong place (the fork's attachment file pool, instead of the original document's file pool).

This is correct:

getDocPoolIdFromDocInfo({ id: _someDoc.id, trunkId: _someDoc.trunkId })

So by omitting ? and using string | null | undefined instead, it's hopefully catching a class of errors early.

That's what this comment is trying (and failing?) to explain. :D

Oh, boy, "optional" vs "an optional", subtle reading thing there.

Maybe rephrase, "We explicitly make this a union type instead of making the attribute optional because the programmer must make a conscious choice to mark it as null or undefined, not merely omit it."

app/server/lib/AttachmentStore.ts

jordigh · 2024-12-20T05:09:30Z

app/server/lib/AttachmentStore.ts

+/**
+ * Gets the correct pool id for a given document, given the document's id and trunk id.
+ *
+ * Attachments are stored in a "Document Pool", which is used to manage the attachments' lifecycle.


I have silly niggle: can we wrap these comments at fewer columns? They're already awkward to read as github diffs:

Looks like you picked 120 columns. How about 100 columns instead? (or my favourite: 80)

120 is what we've got configured in .eslint.js. Happy to go lower personally, but is it worth discussing at a Grist level? We could lower the max length for comments only.

It won't be much of a problem beyond diffs. If you can easily warp comments at a lower character count, nice. If you can't, don't worry about it and let's move on.

Updated IDE with settings for this for the future, probably a bit too big to go through and update all comments right now :)

jordigh · 2024-12-20T05:20:21Z

app/server/lib/AttachmentStore.ts

+    private _storage: StreamingExternalStorage,
+    private _prefixParts: string[]
+  ) {
+    if (_storage.removeAllWithPrefix == undefined) {


Do you really want to allow the possibility of null == undefined in this comparison?

Yup! Honestly, a simple !_storage.removeAllWithPrefix would do. removeAllWithPrefix needs to be a valid function for this to work.

Would !_storage.removeAllWithPrefix be clearer?

I think so. I kind of consider the == operator to be "legacy". At least checking for falsey is clearer in intent.

app/server/lib/AttachmentStore.ts

app/server/lib/MinIOExternalStorage.ts

test/server/lib/ActiveDoc.ts

app/server/utils/MemoryWritableStream.ts

app/server/lib/coreCreator.ts

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

jordigh reviewed Jan 2, 2025

View reviewed changes

Spoffy added 6 commits January 2, 2025 19:38

Removes leftover debugging log

2ba5848

Updates AttachmentStore comments

635a92c

Makes removeAllWithPrefix check clearer

dc55180

Simplifies MinIOExternalStorage "_listObjects" method

ae87fa1

Fixes test import

b46b320

Removes TODO from coreCreator.ts

e44aadf

jordigh approved these changes Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Spoffy commented Nov 24, 2024 •

edited

Loading

paulfitz left a comment

paulfitz Nov 25, 2024

paulfitz Nov 25, 2024

Spoffy Nov 25, 2024

paulfitz Nov 27, 2024

paulfitz Nov 25, 2024

paulfitz Nov 25, 2024

paulfitz Nov 25, 2024

paulfitz Nov 25, 2024

Spoffy Nov 25, 2024 •

edited

Loading

Spoffy Dec 4, 2024 •

edited

Loading

Spoffy Nov 26, 2024

paulfitz Nov 27, 2024

Spoffy Dec 4, 2024

paulfitz left a comment

paulfitz Dec 12, 2024

Spoffy Dec 17, 2024

paulfitz Jan 2, 2025

Spoffy Jan 2, 2025

jordigh left a comment

jordigh Dec 20, 2024

Spoffy Jan 2, 2025 •

edited

Loading

jordigh Jan 2, 2025

jordigh Dec 20, 2024

Spoffy Jan 2, 2025

jordigh Jan 2, 2025

Spoffy Jan 3, 2025

jordigh Dec 20, 2024

Spoffy Jan 2, 2025 •

edited

Loading

jordigh Jan 2, 2025

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

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

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

jordigh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy Jan 2, 2025 • 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

Spoffy Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy commented Nov 24, 2024 •

edited

Loading

Spoffy Nov 25, 2024 •

edited

Loading

Spoffy Dec 4, 2024 •

edited

Loading

Spoffy Jan 2, 2025 •

edited

Loading

Spoffy Jan 2, 2025 •

edited

Loading