DEV: Improve uppy plugin base and large file handling (#14395)

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

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 ec73104..977a016 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 4a00374..15ba5aa 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 34eddeb..f7a0f87 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, 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);
   }
 
-  _emitComplete(file) {
-    this.uppy.emit("preprocess-complete", this.id, file);
+  _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 6bbbe65..efbb0c7 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 120abb1..4c12f7a 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(

[... diff too long, it was truncated ...]

GitHub sha: 02f7035cbef2a20d3e1728d4f7657d664202d126

This commit appears in #14395 which was approved by lis2. It was merged by martin.