Adds zip downloads for all of a document's attachments #1471

Spoffy · 2025-02-20T20:09:29Z

Context

When using internal storage for attachments, all attachments are included in the .grist file when a document is downloaded. They can be extracted if needed using standard SQLite tools.

When using external storage, attachments are stored outside of Grist (e.g. MinIO, S3, filesystem) and can't be easily retrieved.

Proposed solution

This PR adds an API endpoint to download a .zip of all attachments in the document.

This is streamed where possible, resulting in no more than a single file being kept in memory at once (due to SQLite restrictions - external storage keeps no copies in memory).

Related issues

#1021

Has this been tested?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help

paulfitz

Quick q about access control, not a full review since this is still draft.

paulfitz · 2025-02-25T16:12:29Z

app/server/lib/ActiveDoc.ts

@@ -959,6 +960,32 @@ export class ActiveDoc extends EventEmitter {
    return data;
  }

+  public async getAttachmentsArchive(): Promise<Archive> {


There's no access control here. Enough ActiveDoc methods take responsibility for access control that I think you might want to add a comment that you're not doing access control (or take an OptDocSession and do access control). Specifically, so far the caller just has a canView, which isn't a very high bar. The easy path would be to use ActiveDoc.canDownload. More advanced would be to check view permissions on every attachment.

paulfitz

Makes sense! Minor comments so far. Ping me when ready for re-review and everything done. Will make time to kick tires and try it out.

paulfitz · 2025-02-27T18:46:20Z

app/server/lib/ActiveDoc.ts

+      const filesSeen = new Set<string>();
+      for (const attachment of attachments) {
+        if (filesSeen.has(attachment.fileIdent)) {
+          continue;


I think we've agreed to not dedupe attachments that happen to have the same hash (but may have different names)

paulfitz · 2025-02-27T18:46:45Z

app/server/lib/ActiveDoc.ts

+          name: attachment.fileName,
+          data: file.contentStream,
+        });
+        // TODO - Abort this on shutdown by throwing an error


There's a TODO here.

paulfitz · 2025-02-27T18:48:42Z

app/server/lib/Archive.ts

+
+export interface Archive {
+  dataStream: stream.Readable,
+  completed: Promise<void>


Can you format the end of lines of your interfaces consistently?

paulfitz · 2025-02-27T18:49:31Z

app/server/lib/Archive.ts

+
+  return {
+    dataStream: archive,
+    // TODO - Should we add a default 'catch' here that logs errors?


There's a TODO here, I don't quite understand it.

Right now, this promise swallows errors if there isn't a .catch or await used by the calling code. I'm not sure if we should add some default logging here just to be safe.

The comment claims that await stream.promises.finished(archive); will throw if there's an error? With the error caught error? If that's true, that seems enough to me. A docstring saying that the caller owns the Archive promise could be worthwhile just to draw attention? But apart from that, it would be just normal to not leave any promise you are given unhandled.

Perfect - thanks for clarifying!

app/server/lib/AttachmentFileManager.ts

paulfitz · 2025-02-27T19:04:49Z

app/server/lib/AttachmentStore.ts

 import * as fse from 'fs-extra';
 import * as stream from 'node:stream';
 import * as path from 'path';
+import {Readable} from 'node:stream';


import order

Oh man, I'm really disappointed with this one - I did a review of imports before posting the PR for review.

app/server/lib/MinIOExternalStorage.ts

paulfitz · 2025-02-27T19:12:35Z

test/server/lib/Archive.ts

+  }
+}
+
+describe('create_zip_archive', function () {


Can you wrap everything in a describe Archive? It is helpful heuristically to have a top level test name that matches the filename. Not 100% done in codebase but done enough that it is annoying when not done.

paulfitz

A note from testing.

paulfitz · 2025-02-27T21:50:51Z

app/server/lib/DocApi.ts

+      res.status(200)
+        .type("application/zip")
+        // Construct a content-disposition header of the form 'attachment; filename="NAME"'
+        .set('Content-Disposition', contentDisposition(`${activeDoc.docName}.zip`, {type: 'attachment'}))


Gave this a try. Everything worked well. The name was a bit jarring, compared to other downloads associated with a document. What do you think about a more human friendly name, that rhymes with what other /download methods do?

grist-core/app/server/lib/DocApi.ts

Lines 1834 to 1844 in 14b9147

private async _getDownloadFilename(req: Request, tableId?: string, optDoc?: Document): Promise<string> {

let filename = optStringParam(req.query.title, 'title');

if (!filename) {

// Query DB for doc metadata to get the doc data.

const doc = optDoc || await this._dbManager.getDoc(req);

const docTitle = doc.name;

const suffix = tableId ? (tableId === docTitle ? '' : `-${tableId}`) : '';

filename = docTitle + suffix || 'document';

}

return filename;

}

Something a bit weird seems to happen if I download with multiple distinct files that have the same name, but we've talked about changing the name structure so no need to poke at that...

Spoffy · 2025-02-28T14:57:59Z

app/server/lib/ActiveDoc.ts

      for (const attachment of attachments) {
-        if (filesSeen.has(attachment.fileIdent)) {
-          continue;
+        if (doc._shuttingDown) {
+          throw new ApiError("Document is shutting down, archiving aborted", 500);
        }


I don't really like this approach, because there's not technically an ActiveDoc dependency here - there's a DocStorage dependency, which happens to be shut down when ActiveDoc is. I'd like us to refactor shutdown handling at some point across ActiveDoc and all dependencies, but this feels okay for now?

paulfitz

Thanks @Spoffy !

paulfitz · 2025-02-28T16:18:32Z

app/server/lib/ActiveDoc.ts

+      !await this._granularAccess.canReadEverything(docSession) &&
+      !await this.canDownload(docSession)
+    ) {
+      throw new ApiError("No access to download attachments", 403);


Perhaps "Insufficient access"

paulfitz · 2025-02-28T17:12:27Z

test/server/lib/DocApi.ts

+          {...chimpy, responseType: 'arraybuffer'});
+        assert.equal(resp.status, 200);
+        assert.deepEqual(resp.headers['content-type'], 'application/zip');
+        assert.deepEqual(resp.headers['content-disposition'], `attachment; filename="TestDocExternalAttachments-Attachments.zip"`);


Some long line linting around here.

paulfitz

Thanks @Spoffy !

Spoffy added 2 commits February 17, 2025 21:13

Makes downloadStream fetch metadata

b3416ee

Refactors attachment downloads to be streaming, and include metadata

e45b093

Spoffy marked this pull request as draft February 20, 2025 20:09

Adds Zip archive downloads for attachments

001004b

Spoffy force-pushed the spoffy/attachment-downloads branch from 6d68c6d to 001004b Compare February 20, 2025 20:11

Spoffy added 4 commits February 25, 2025 02:28

Fixes test types for attachments

ca8f3b2

Attempts a fix for content-length causing tests to fail

f7dd1cd

Adds TODO

306261a

Fixes test error in ExternalStorageAttachmentStore

30f9556

paulfitz reviewed Feb 25, 2025

View reviewed changes

Spoffy added 6 commits February 25, 2025 21:20

Adds archive tests and fixes

dafb2fb

Changes zip-stream version to 6.0.1

dc29771

Adds permissions check to getAttachmentsArchive

742cac3

Adds DocApi tests

99513f1

Fixes bad test expectation for DocApi

336a678

Fixes import order in Archive.ts

6668797

Spoffy requested a review from paulfitz February 27, 2025 16:06

Spoffy marked this pull request as ready for review February 27, 2025 16:06

Spoffy added 2 commits February 27, 2025 16:10

Merge branch 'main' into spoffy/attachment-downloads

72f3e1e

Refreshes yarn.lock

d95b2d9

paulfitz reviewed Feb 27, 2025

View reviewed changes

Addresses formatting code review feedback

3a825f3

paulfitz reviewed Feb 27, 2025

View reviewed changes

Spoffy added 5 commits February 27, 2025 22:10

Changes file naming convention for attachments

b76655f

Shuts down attachment archiving if document shuts down

6f707bb

Adds ActiveDoc archive tests

efe15f7

Cleans archive assertion in DocAPi

e67c259

Swaps TODO for doc comment

1f845fe

Spoffy commented Feb 28, 2025

View reviewed changes

Re-adds accidentally removed import

2038992

Spoffy added 2 commits February 28, 2025 15:18

Prevents name collisions due to multiple identical attachments

c460ab6

Changes attachment file name to match doc name

a6daa51

paulfitz reviewed Feb 28, 2025

View reviewed changes

Spoffy added 2 commits February 28, 2025 17:34

Changes 'no access' to 'insufficient access'

bfc9312

Fixes over-length line

d4eaadc

paulfitz approved these changes Feb 28, 2025

View reviewed changes

Spoffy merged commit 5285628 into main Mar 3, 2025
12 checks passed

Spoffy deleted the spoffy/attachment-downloads branch March 3, 2025 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds zip downloads for all of a document's attachments #1471

Adds zip downloads for all of a document's attachments #1471

Spoffy commented Feb 20, 2025 •

edited

Loading

paulfitz left a comment

paulfitz Feb 25, 2025

paulfitz left a comment

paulfitz Feb 27, 2025

paulfitz Feb 27, 2025

paulfitz Feb 27, 2025

paulfitz Feb 27, 2025

Spoffy Feb 27, 2025

paulfitz Feb 27, 2025

Spoffy Feb 28, 2025

paulfitz Feb 27, 2025

Spoffy Feb 27, 2025

paulfitz Feb 27, 2025

paulfitz left a comment

paulfitz Feb 27, 2025

Spoffy Feb 28, 2025

paulfitz left a comment

paulfitz Feb 28, 2025

paulfitz Feb 28, 2025

paulfitz left a comment

	private async _getDownloadFilename(req: Request, tableId?: string, optDoc?: Document): Promise<string> {
	let filename = optStringParam(req.query.title, 'title');
	if (!filename) {
	// Query DB for doc metadata to get the doc data.
	const doc = optDoc \|\| await this._dbManager.getDoc(req);
	const docTitle = doc.name;
	const suffix = tableId ? (tableId === docTitle ? '' : `-${tableId}`) : '';
	filename = docTitle + suffix \|\| 'document';
	}
	return filename;
	}

Adds zip downloads for all of a document's attachments #1471

Adds zip downloads for all of a document's attachments #1471

Conversation

Spoffy commented Feb 20, 2025 • edited Loading

Context

Proposed solution

Related issues

Has this been tested?

paulfitz left a 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

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

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

paulfitz left a comment

Choose a reason for hiding this comment

Spoffy commented Feb 20, 2025 •

edited

Loading