From 02f7035cbef2a20d3e1728d4f7657d664202d126 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 21 Sep 2021 08:41:07 +1000 Subject: [PATCH] DEV: Improve uppy plugin base and large file handling (#14395) We want to be able to skip plugins from doing any work under certain conditions, and to be able raise their own errors if a file being uploaded is completely incompatible with the concept of the plugin if it is enabled. For example, the UppyChecksum plugin is happy to skip hashing large files, but the UppyUploadEncrypt plugin from discourse-encrypt relies on the file being encrypted to do anything with the upload, so it is considered a blocking error if the user uploads a file that is too large. This improves the base functions available in uppy-plugin-base and extendable-uploader to handle this, as well as introducing a HUGE_FILE_THRESHOLD_BYTES variable which represents 100MB in bytes, matching the ExternalUploadManager::DOWNLOAD_LIMIT on the server side. discourse-encrypt to take advantage of this new functionality will follow in discourse/discourse-encrypt#141 --- .../discourse/app/lib/uppy-checksum-plugin.js | 11 ++++- .../app/lib/uppy-media-optimization-plugin.js | 6 ++- .../discourse/app/lib/uppy-plugin-base.js | 42 +++++++++++++++++-- .../app/mixins/composer-upload-uppy.js | 13 +++--- .../app/mixins/extendable-uploader.js | 23 +++++++--- .../discourse/app/mixins/uppy-upload.js | 2 + .../unit/lib/uppy-checksum-plugin-test.js | 38 ++++++++++++++--- 7 files changed, 108 insertions(+), 27 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/uppy-checksum-plugin.js b/app/assets/javascripts/discourse/app/lib/uppy-checksum-plugin.js index ec73104541d..977a01659b2 100644 --- a/app/assets/javascripts/discourse/app/lib/uppy-checksum-plugin.js +++ b/app/assets/javascripts/discourse/app/lib/uppy-checksum-plugin.js @@ -1,5 +1,6 @@ import { UploadPreProcessorPlugin } from "discourse/lib/uppy-plugin-base"; import { Promise } from "rsvp"; +import { HUGE_FILE_THRESHOLD_BYTES } from "discourse/mixins/uppy-upload"; export default class UppyChecksum extends UploadPreProcessorPlugin { static pluginId = "uppy-checksum"; @@ -34,14 +35,20 @@ export default class UppyChecksum extends UploadPreProcessorPlugin { _generateChecksum(fileIds) { if (!this._canUseSubtleCrypto()) { - return Promise.resolve(); + return this._skipAll(fileIds, true); } let promises = fileIds.map((fileId) => { let file = this._getFile(fileId); - this._emitProgress(file); + if (file.size > HUGE_FILE_THRESHOLD_BYTES) { + this._consoleWarn( + "The file provided is too large to checksum, skipping." + ); + return this._skip(file); + } + return file.data.arrayBuffer().then((arrayBuffer) => { return window.crypto.subtle .digest("SHA-1", arrayBuffer) diff --git a/app/assets/javascripts/discourse/app/lib/uppy-media-optimization-plugin.js b/app/assets/javascripts/discourse/app/lib/uppy-media-optimization-plugin.js index 4a00374468b..15ba5aa419b 100644 --- a/app/assets/javascripts/discourse/app/lib/uppy-media-optimization-plugin.js +++ b/app/assets/javascripts/discourse/app/lib/uppy-media-optimization-plugin.js @@ -22,17 +22,19 @@ export default class UppyMediaOptimization extends UploadPreProcessorPlugin { return this.optimizeFn(file, { stopWorkerOnError: !this.runParallel }) .then((optimizedFile) => { + let skipped = false; if (!optimizedFile) { this._consoleWarn( - "Nothing happened, possible error or other restriction." + "Nothing happened, possible error or other restriction, or the file format is not a valid one for compression." ); + skipped = true; } else { this._setFileState(fileId, { data: optimizedFile, size: optimizedFile.size, }); } - this._emitComplete(file); + this._emitComplete(file, skipped); }) .catch((err) => { this._consoleWarn(err); diff --git a/app/assets/javascripts/discourse/app/lib/uppy-plugin-base.js b/app/assets/javascripts/discourse/app/lib/uppy-plugin-base.js index 34eddeb050a..f7a0f871460 100644 --- a/app/assets/javascripts/discourse/app/lib/uppy-plugin-base.js +++ b/app/assets/javascripts/discourse/app/lib/uppy-plugin-base.js @@ -1,4 +1,5 @@ import { BasePlugin } from "@uppy/core"; +import { Promise } from "rsvp"; import { warn } from "@ember/debug"; export class UppyPluginBase extends BasePlugin { @@ -8,7 +9,14 @@ export class UppyPluginBase extends BasePlugin { } _consoleWarn(msg) { - warn(msg, { id: `discourse.${this.id}` }); + warn(`[${this.id}] ${msg}`, { id: `discourse.${this.id}` }); + } + + _consoleDebug(msg) { + if (this.siteSettings?.enable_upload_debug_mode) { + // eslint-disable-next-line no-console + console.log(`[${this.id}] ${msg}`); + } } _getFile(fileId) { @@ -41,10 +49,36 @@ export class UploadPreProcessorPlugin extends UppyPluginBase { } _emitProgress(file) { - this.uppy.emit("preprocess-progress", this.id, file); + this.uppy.emit("preprocess-progress", file, null, this.id); } - _emitComplete(file) { - this.uppy.emit("preprocess-complete", this.id, file); + _emitComplete(file, skipped = false) { + this.uppy.emit("preprocess-complete", file, skipped, this.id); + return Promise.resolve(); + } + + _emitAllComplete(fileIds, skipped = false) { + fileIds.forEach((fileId) => { + let file = this._getFile(fileId); + this._emitComplete(file, skipped); + }); + return Promise.resolve(); + } + + _emitError(file, errorMessage) { + // the error message is stored twice; once to show in a displayErrorForUpload + // modal, and on the .message property to show in the uppy logs + this.uppy.emit("upload-error", file, { + errors: [errorMessage], + message: `[${this.id}] ${errorMessage}`, + }); + } + + _skip(file) { + return this._emitComplete(file, true); + } + + _skipAll(file) { + return this._emitAllComplete(file, true); } } diff --git a/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js b/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js index 6bbbe659357..efbb0c75d1f 100644 --- a/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js +++ b/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js @@ -382,6 +382,8 @@ export default Mixin.create(ExtendableUploader, { limit: 10, createMultipartUpload(file) { + self._uppyInstance.emit("create-multipart", file.id); + const data = { file_name: file.name, file_size: file.size, @@ -402,6 +404,8 @@ export default Mixin.create(ExtendableUploader, { data, // uppy is inconsistent, an error here fires the upload-error event }).then((responseData) => { + self._uppyInstance.emit("create-multipart-success", file.id); + file.meta.unique_identifier = responseData.unique_identifier; return { uploadId: responseData.external_upload_identifier, @@ -430,6 +434,7 @@ export default Mixin.create(ExtendableUploader, { }, completeMultipartUpload(file, data) { + self._uppyInstance.emit("complete-multipart", file.id); const parts = data.parts.map((part) => { return { part_number: part.PartNumber, etag: part.ETag }; }); @@ -442,6 +447,7 @@ export default Mixin.create(ExtendableUploader, { }), // uppy is inconsistent, an error here fires the upload-error event }).then((responseData) => { + self._uppyInstance.emit("complete-multipart-success", file.id); return responseData; }); }, @@ -556,11 +562,4 @@ export default Mixin.create(ExtendableUploader, { showUploadSelector(toolbarEvent) { this.send("showUploadSelector", toolbarEvent); }, - - _debugLog(message) { - if (this.siteSettings.enable_upload_debug_mode) { - // eslint-disable-next-line no-console - console.log(message); - } - }, }); diff --git a/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js b/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js index 120abb13ad8..4c12f7ac80c 100644 --- a/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js +++ b/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js @@ -80,8 +80,10 @@ export default Mixin.create({ // // See: https://uppy.io/docs/writing-plugins/#Progress-events _onPreProcessProgress(callback) { - this._uppyInstance.on("preprocess-progress", (pluginId, file) => { - this._debugLog(`[${pluginId}] processing file ${file.name} (${file.id})`); + this._uppyInstance.on("preprocess-progress", (file, progress, pluginId) => { + this._consoleDebug( + `[${pluginId}] processing file ${file.name} (${file.id})` + ); this._preProcessorStatus[pluginId].activeProcessing++; @@ -90,16 +92,18 @@ export default Mixin.create({ }, _onPreProcessComplete(callback, allCompleteCallback) { - this._uppyInstance.on("preprocess-complete", (pluginId, file) => { - this._debugLog( - `[${pluginId}] completed processing file ${file.name} (${file.id})` + this._uppyInstance.on("preprocess-complete", (file, skipped, pluginId) => { + this._consoleDebug( + `[${pluginId}] ${skipped ? "skipped" : "completed"} processing file ${ + file.name + } (${file.id})` ); callback(file); this._completePreProcessing(pluginId, (allComplete) => { if (allComplete) { - this._debugLog("All upload preprocessors complete."); + this._consoleDebug("[uppy] All upload preprocessors complete!"); allCompleteCallback(); } }); @@ -168,4 +172,11 @@ export default Mixin.create({ } } }, + + _consoleDebug(msg) { + if (this.siteSettings.enable_upload_debug_mode) { + // eslint-disable-next-line no-console + console.log(msg); + } + }, }); diff --git a/app/assets/javascripts/discourse/app/mixins/uppy-upload.js b/app/assets/javascripts/discourse/app/mixins/uppy-upload.js index 5c9cbb8aeb9..ad8cd44084a 100644 --- a/app/assets/javascripts/discourse/app/mixins/uppy-upload.js +++ b/app/assets/javascripts/discourse/app/mixins/uppy-upload.js @@ -16,6 +16,8 @@ import UppyChecksum from "discourse/lib/uppy-checksum-plugin"; import { on } from "discourse-common/utils/decorators"; import { warn } from "@ember/debug"; +export const HUGE_FILE_THRESHOLD_BYTES = 104_857_600; // 100MB + export default Mixin.create({ uploading: false, uploadProgress: 0, diff --git a/app/assets/javascripts/discourse/tests/unit/lib/uppy-checksum-plugin-test.js b/app/assets/javascripts/discourse/tests/unit/lib/uppy-checksum-plugin-test.js index ae0c6fbeba4..58cd1b7e276 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/uppy-checksum-plugin-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/uppy-checksum-plugin-test.js @@ -10,10 +10,17 @@ class FakeUppy { "uppy-test/file/vv2/xvejg5w/blah/png-1d-1d-2v-1d-1e-image/jpeg-9043429-1624921727764": { meta: {}, data: createFile("test1.png"), + size: 1024, }, "uppy-test/file/blah1/ads37x2/blah1/png-1d-1d-2v-1d-1e-image/jpeg-99999-1837921727764": { meta: {}, data: createFile("test2.png"), + size: 2048, + }, + "uppy-test/file/mnb3/jfhrg43x/blah3/png-1d-1d-2v-1d-1e-image/jpeg-111111-1837921727764": { + meta: {}, + data: createFile("test2.png"), + size: 209715200, }, }; } @@ -62,8 +69,8 @@ module("Unit | Utility | UppyChecksum Plugin", function () { plugin.uppy.preprocessors[0]([fileId]).then(() => { assert.equal( plugin.uppy.emitted.length, - 0, - "no events were fired by the checksum plugin because it returned early" + 1, + "only the complete event was fired by the checksum plugin because it skipped the file" ); done(); }); @@ -85,8 +92,8 @@ module("Unit | Utility | UppyChecksum Plugin", function () { plugin.uppy.preprocessors[0]([fileId]).then(() => { assert.equal( plugin.uppy.emitted.length, - 0, - "no events were fired by the checksum plugin because it returned early" + 1, + "only the complete event was fired by the checksum plugin because it skipped the file" ); done(); }); @@ -106,13 +113,32 @@ module("Unit | Utility | UppyChecksum Plugin", function () { plugin.uppy.preprocessors[0]([fileId]).then(() => { assert.equal( plugin.uppy.emitted.length, - 0, - "no events were fired by the checksum plugin because it returned early" + 1, + "only the complete event was fired by the checksum plugin because it skipped the file" ); done(); }); }); + test("it does nothing if the file is > 100MB", function (assert) { + const capabilities = {}; + const fakeUppy = new FakeUppy(); + const plugin = new UppyChecksum(fakeUppy, { + capabilities, + }); + plugin.install(); + const done = assert.async(); + + const fileId = + "uppy-test/file/mnb3/jfhrg43x/blah3/png-1d-1d-2v-1d-1e-image/jpeg-111111-1837921727764"; + plugin.uppy.preprocessors[0]([fileId]).then(() => { + assert.equal(plugin.uppy.emitted[0].event, "preprocess-progress"); + assert.equal(plugin.uppy.emitted[1].event, "preprocess-complete"); + assert.equal(plugin.uppy.getFile(fileId).meta.sha1_checksum, null); + done(); + }); + }); + test("it gets a sha1 hash of each file and adds it to the file meta", function (assert) { const capabilities = {}; const fakeUppy = new FakeUppy();