Adds support for downloading attachments as a .tar#1495
Conversation
paulfitz
left a comment
paulfitz
left a comment
There was a problem hiding this comment.
This looks good!
I think endpoint forwarders are needed, and the tests may need a tweak.
app/server/lib/Archive.ts
Outdated
| } | ||
|
|
||
| export const CreatableArchiveFormats = StringUnion('zip', 'tar'); | ||
| export type CreatableArchiveFormats = typeof CreatableArchiveFormats.type |
There was a problem hiding this comment.
trivial: our codebase normally puts semicolon at end of type?
There was a problem hiding this comment.
Fixed, wonder why this one wasn't caught by the linting rules?
| ]); | ||
| }); | ||
|
|
||
| it("GET /docs/{did}/attachments/download downloads all attachments as a .tar", async function () { |
There was a problem hiding this comment.
I noticed on previous work that @berhalak later needed to add forwarders for new attachment endpoints:
https://github.com/gristlabs/grist-core/pull/1348/files#diff-531fd1025fc38b2b06fc3bafb8972112a3480fa807426e12e7b047374576c807
This is normal, in multi-server Grist all doc workers interact with the outside world via forwarding from home servers for ... reasons.
I was wondering why the tests you're adding are passing, despite being run for multiple configurations, including separate home server and doc worker.
I think it may be that by using serverUrl versus homeUrl, the tests are talking directly to the doc worker.
There was a problem hiding this comment.
I've added this - but not quite sure how to test it!
There was a problem hiding this comment.
I think the trick will be to use homeUrl rather than serverUrl to better mimic real conditions. But I could be misremembering.
Spoffy
force-pushed
the
spoffy/attachment-downloads-tar
branch
from
07013e6 to
b088df1
Compare
| app.use('/api/docs/:docId/create-fork', withDoc); | ||
| app.use('/api/docs/:docId/apply', withDoc); | ||
| app.use('/api/docs/:docId/attachments', withDoc); | ||
| app.use('/api/docs/:docId/attachments/download', withDoc); |
There was a problem hiding this comment.
I wonder if the /attachments forwarders at the end of this list could all be herded together. Btw, do tests now fail if you remove this forwarder?
2 participants
Context
Currently only Zip is supported for downloading attachments. In the future, we want to be able to support re-uploading attachments. Zip will make this difficult, as the structure of the zip file is stored at the end of file, requiring the entire file to be uploaded before it can be parsed.
Proposed solution
This PR adds downloading attachments as a .tar, which uses inline headers for each file it contains. This means it can be easily parsed and processed through node's streams, minimizing server disk and memory usage.
Related issues
#1021
Has this been tested?