Adds archive uploading to add missing attachments #1506
Conversation
There was a problem hiding this comment.
This was renamed to streams.ts
| if (err instanceof MismatchedFileHashError) { | ||
| this._log.warn(docSession, `Failed to upload attachment: ${err.message}`); | ||
| } | ||
| this._log.error(docSession, `Failed to upload attachment: ${err}`); |
There was a problem hiding this comment.
Should this also just be a warning?
There was a problem hiding this comment.
There's no particular standard. This seems ok. The log isn't very structured for ingestion, but that's ok.
There was a problem hiding this comment.
I wonder about maybe returning the stringified errors in a list in the results, since otherwise the problem(s) will be very opaque to user. Optional, easy to add later.
There was a problem hiding this comment.
I was hesitant about doing that, just in case they're cryptic and unhelpful runtime errors (like the classic "access property of undefined"). Easy either way from my perspective.
app/server/lib/ActiveDoc.ts
Outdated
| const fileIdentParts = attachment.fileIdent.split("."); | ||
| const fileHash = fileIdentParts[0]; | ||
| const fileIdentExt = path.extname(attachment.fileIdent); | ||
| const fileNameExt = path.extname(attachment.fileName); | ||
| const fileNameNoExt = path.basename(attachment.fileName, fileNameExt); | ||
| // We need to make sure the downloaded attachment's extension matches Grist's internal | ||
| // file extension, otherwise we can't recreate the file identifier when uploading. | ||
| // They might not match, as the upload process considers things like mime type when | ||
| // adding the extension to the file identifier. | ||
| const name = `${fileHash}_${fileNameNoExt}${fileIdentExt}`; |
There was a problem hiding this comment.
Small tweak to the names of files in downloaded archives (see comment). Shouldn't make any difference 99% of the time, but covers an edge case.
| if (err instanceof MismatchedFileHashError) { | ||
| this._log.warn(docSession, `Failed to upload attachment: ${err.message}`); | ||
| } | ||
| this._log.error(docSession, `Failed to upload attachment: ${err}`); |
There was a problem hiding this comment.
There's no particular standard. This seems ok. The log isn't very structured for ingestion, but that's ok.
| if (err instanceof MismatchedFileHashError) { | ||
| this._log.warn(docSession, `Failed to upload attachment: ${err.message}`); | ||
| } | ||
| this._log.error(docSession, `Failed to upload attachment: ${err}`); |
There was a problem hiding this comment.
I wonder about maybe returning the stringified errors in a list in the results, since otherwise the problem(s) will be very opaque to user. Optional, easy to add later.
| return this._addFile(storeId, fileIdent, fileData); | ||
| } | ||
|
|
||
| public async addMissingFileData( |
There was a problem hiding this comment.
There are enough nuances in this method that it could do with a solid docstring.
Context
When a document using external attachments is downloaded then uploaded, none of the attachments will work. This is due to external attachments being stored on a per-document basis.
Proposed solution
This PR allows any missing files to be populated in the external store, by uploading a .tar archive containing the missing attachments.
It only supports .tar archives downloaded from Grist, as it uses the file's name to check if the file is missing.
Related issues
#1021
Has this been tested?