FEATURE: uploads are processed a faster

Also cleans up API to always return 422 on upload error. (previously returned 200)

Uploads are processed using new hijack pattern

app/assets/javascripts/discourse/components/composer-editor.js.es6

@@ -416,20 +416,9 @@ export default Ember.Component.extend({
       }
     });
 
-    $element.on("fileuploadfail", (e, data) => {
+    $element.on("fileuploaddone", (e, data) => {
-      this._resetUpload(true);
+      let upload = data.result;
 
-      const userCancelled = this._xhr && this._xhr._userCancelled;
-      this._xhr = null;
-
-      if (!userCancelled) {
-        displayErrorForUpload(data);
-      }
-    });
-
-    this.messageBus.subscribe("/uploads/composer", upload => {
-      // replace upload placeholder
-      if (upload && upload.url) {
       if (!this._xhr || !this._xhr._userCancelled) {
         const markdown = getUploadMarkdown(upload);
         cacheShortUploadUrl(upload.short_url, upload.url);
@@ -438,9 +427,16 @@ export default Ember.Component.extend({
       } else {
         this._resetUpload(true);
       }
-      } else {
+    });
+
+    $element.on("fileuploadfail", (e, data) => {
       this._resetUpload(true);
-        displayErrorForUpload(upload);
+
+      const userCancelled = this._xhr && this._xhr._userCancelled;
+      this._xhr = null;
+
+      if (!userCancelled) {
+        displayErrorForUpload(data.jqXHR.responseJSON);
       }
     });
 

app/assets/javascripts/discourse/mixins/upload.js.es6

@@ -18,12 +18,9 @@ export default Em.Mixin.create({
           uploadUrl = Discourse.getURL(this.getWithDefault("uploadUrl", "/uploads")),
           reset = () => this.setProperties({ uploading: false, uploadProgress: 0});
 
-    this.messageBus.subscribe("/uploads/" + this.get("type"), upload => {
+    $upload.on("fileuploaddone", (e, data) => {
-      if (upload && upload.url) {
+      let upload = data.result;
       this.uploadDone(upload);
-      } else {
-        displayErrorForUpload(upload);
-      }
       reset();
     });
 
@@ -59,7 +56,7 @@ export default Em.Mixin.create({
     });
 
     $upload.on("fileuploadfail", (e, data) => {
-      displayErrorForUpload(data);
+      displayErrorForUpload(data.jqXHR.responseJSON);
       reset();
     });
   }.on("didInsertElement"),

app/controllers/uploads_controller.rb

@@ -20,19 +20,23 @@ class UploadsController < ApplicationController
     file   = params[:file] || params[:files]&.first
     pasted = params[:pasted] == "true"
     for_private_message = params[:for_private_message] == "true"
+    is_api = is_api?
+    retain_hours = params[:retain_hours].to_i
 
-    if params[:synchronous] && (current_user.staff? || is_api?)
+    # note, atm hijack is processed in its own context and has not access to controller
-      data = create_upload(current_user, file, url, type, for_private_message, pasted)
+    # longer term we may change this
-      render json: serialize_upload(data)
+    hijack do
-    else
+      info = UploadsController.create_upload(
-      Scheduler::Defer.later("Create Upload") do
+        current_user: me,
-        begin
+        file: file,
-          data = create_upload(me, file, url, type, for_private_message, pasted)
+        url: url,
-        ensure
+        type: type,
-          MessageBus.publish("/uploads/#{type}", serialize_upload(data), client_ids: [params[:client_id]])
+        for_private_message: for_private_message,
-        end
+        pasted: pasted,
-      end
+        is_api: is_api,
-      render json: success_json
+        retain_hours: retain_hours
+      )
+      render json: UploadsController.serialize_upload(info), status: Upload === info ? 200 : 422
     end
   end
 
@@ -72,20 +76,20 @@ class UploadsController < ApplicationController
 
   protected
 
-  def serialize_upload(data)
+  def render_404
+    raise Discourse::NotFound
+  end
+
+  def self.serialize_upload(data)
     # as_json.as_json is not a typo... as_json in AM serializer returns keys as symbols, we need them
     # as strings here
     serialized = UploadSerializer.new(data, root: nil).as_json.as_json if Upload === data
     serialized ||= (data || {}).as_json
   end
 
-  def render_404
+  def self.create_upload(current_user:, file:, url:, type:, for_private_message:, pasted:, is_api:, retain_hours:)
-    raise Discourse::NotFound
-  end
-
-  def create_upload(current_user, file, url, type, for_private_message, pasted)
     if file.nil?
-      if url.present? && is_api?
+      if url.present? && is_api
         maximum_upload_size = [SiteSetting.max_image_size_kb, SiteSetting.max_attachment_size_kb].max.kilobytes
         tempfile = FileHelper.download(
           url,
@@ -112,7 +116,6 @@ class UploadsController < ApplicationController
     upload = UploadCreator.new(tempfile, filename, opts).create_for(current_user.id)
 
     if upload.errors.empty? && current_user.admin?
-      retain_hours = params[:retain_hours].to_i
       upload.update_columns(retain_hours: retain_hours) if retain_hours > 0
     end
 

lib/hijack.rb

@@ -25,9 +25,17 @@ module Hijack
       end
 
       if opts.key?(:plain)
-        @content_type = 'text/plain'
+        @content_type = 'text/plain; charset=utf-8'
         @body = opts[:plain].to_s
       end
+
+      if opts.key?(:json)
+        @content_type = 'application/json; charset=utf-8'
+        @body = opts[:json]
+        unless String === @body
+          @body = @body.to_json
+        end
+      end
     end
   end
 

spec/controllers/uploads_controller_spec.rb

@@ -40,12 +40,10 @@ describe UploadsController do
       it 'is successful with an image' do
         Jobs.expects(:enqueue).with(:create_avatar_thumbnails, anything)
 
-        message = MessageBus.track_publish('/uploads/avatar') do
         post :create, params: { file: logo, type: "avatar", format: :json }
-        end.first
 
         expect(response.status).to eq 200
-        expect(message.data["id"]).to be
+        expect(JSON.parse(response.body)["id"]).to be
       end
 
       it 'is successful with an attachment' do
@@ -53,12 +51,11 @@ describe UploadsController do
 
         Jobs.expects(:enqueue).never
 
-        message = MessageBus.track_publish('/uploads/composer') do
         post :create, params: { file: text_file, type: "composer", format: :json }
-        end.first
 
         expect(response.status).to eq 200
-        expect(message.data["id"]).to be
+        id = JSON.parse(response.body)["id"]
+        expect(id).to be
       end
 
       it 'is successful with synchronous api' do
@@ -88,28 +85,25 @@ describe UploadsController do
         log_in :admin
         Jobs.expects(:enqueue).with(:create_avatar_thumbnails, anything).never
 
-        message = MessageBus.track_publish('/uploads/profile_background') do
         post :create, params: {
           file: logo,
           retain_hours: 100,
           type: "profile_background",
           format: :json
         }
-        end.first
 
-        id = message.data["id"]
+        id = JSON.parse(response.body)["id"]
         expect(Upload.find(id).retain_hours).to eq(100)
       end
 
       it 'requires a file' do
         Jobs.expects(:enqueue).never
 
-        message = MessageBus.track_publish('/uploads/composer') do
         post :create, params: { type: "composer", format: :json }
-        end.first
 
-        expect(response.status).to eq 200
+        message = JSON.parse(response.body)
-        expect(message.data["errors"]).to contain_exactly(I18n.t("upload.file_missing"))
+        expect(response.status).to eq 422
+        expect(message["errors"]).to contain_exactly(I18n.t("upload.file_missing"))
       end
 
       it 'properly returns errors' do
@@ -117,12 +111,11 @@ describe UploadsController do
 
         Jobs.expects(:enqueue).never
 
-        message = MessageBus.track_publish("/uploads/avatar") do
         post :create, params: { file: text_file, type: "avatar", format: :json }
-        end.first
 
-        expect(response.status).to eq 200
+        expect(response.status).to eq 422
-        expect(message.data["errors"]).to be
+        errors = JSON.parse(response.body)["errors"]
+        expect(errors).to be
       end
 
       it 'ensures allow_uploaded_avatars is enabled when uploading an avatar' do
@@ -142,28 +135,26 @@ describe UploadsController do
         SiteSetting.allow_staff_to_upload_any_file_in_pm = true
         @user.update_columns(moderator: true)
 
-        message = MessageBus.track_publish('/uploads/composer') do
         post :create, params: {
           file: text_file,
           type: "composer",
           for_private_message: "true",
           format: :json
         }
-        end.first
 
         expect(response).to be_success
-        expect(message.data["id"]).to be
+        id = JSON.parse(response.body)["id"]
+        expect(id).to be
       end
 
       it 'returns an error when it could not determine the dimensions of an image' do
         Jobs.expects(:enqueue).with(:create_avatar_thumbnails, anything).never
 
-        message = MessageBus.track_publish('/uploads/composer') do
         post :create, params: { file: fake_jpg, type: "composer", format: :json }
-        end.first
 
-        expect(response.status).to eq 200
+        expect(response.status).to eq 422
-        expect(message.data["errors"]).to contain_exactly(I18n.t("upload.images.size_not_found"))
+        message = JSON.parse(response.body)["errors"]
+        expect(message).to contain_exactly(I18n.t("upload.images.size_not_found"))
       end
 
     end