From eb428ef54dcc1d0d74f5d4a0d3fd12f1122dab65 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 27 Nov 2017 12:43:18 +1100 Subject: [PATCH] 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 --- .../components/composer-editor.js.es6 | 32 ++++---- .../discourse/mixins/upload.js.es6 | 11 +-- app/controllers/uploads_controller.rb | 43 ++++++----- lib/hijack.rb | 10 ++- spec/controllers/uploads_controller_spec.rb | 73 ++++++++----------- 5 files changed, 82 insertions(+), 87 deletions(-) diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index d5579086a0e..a30f119104e 100644 --- a/app/assets/javascripts/discourse/components/composer-editor.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6 @@ -416,6 +416,19 @@ export default Ember.Component.extend({ } }); + $element.on("fileuploaddone", (e, data) => { + let upload = data.result; + + if (!this._xhr || !this._xhr._userCancelled) { + const markdown = getUploadMarkdown(upload); + cacheShortUploadUrl(upload.short_url, upload.url); + this.appEvents.trigger('composer:replace-text', uploadPlaceholder, markdown); + this._resetUpload(false); + } else { + this._resetUpload(true); + } + }); + $element.on("fileuploadfail", (e, data) => { this._resetUpload(true); @@ -423,24 +436,7 @@ export default Ember.Component.extend({ 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); - this.appEvents.trigger('composer:replace-text', uploadPlaceholder, markdown); - this._resetUpload(false); - } else { - this._resetUpload(true); - } - } else { - this._resetUpload(true); - displayErrorForUpload(upload); + displayErrorForUpload(data.jqXHR.responseJSON); } }); diff --git a/app/assets/javascripts/discourse/mixins/upload.js.es6 b/app/assets/javascripts/discourse/mixins/upload.js.es6 index ec76aa1e660..1dc55eeb851 100644 --- a/app/assets/javascripts/discourse/mixins/upload.js.es6 +++ b/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 => { - if (upload && upload.url) { - this.uploadDone(upload); - } else { - displayErrorForUpload(upload); - } + $upload.on("fileuploaddone", (e, data) => { + let upload = data.result; + this.uploadDone(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"), diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 72b9fc98593..8ea6c766eb2 100644 --- a/app/controllers/uploads_controller.rb +++ b/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?) - data = create_upload(current_user, file, url, type, for_private_message, pasted) - render json: serialize_upload(data) - else - Scheduler::Defer.later("Create Upload") do - begin - data = create_upload(me, file, url, type, for_private_message, pasted) - ensure - MessageBus.publish("/uploads/#{type}", serialize_upload(data), client_ids: [params[:client_id]]) - end - end - render json: success_json + # note, atm hijack is processed in its own context and has not access to controller + # longer term we may change this + hijack do + info = UploadsController.create_upload( + current_user: me, + file: file, + url: url, + type: type, + for_private_message: for_private_message, + pasted: pasted, + is_api: is_api, + 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 - raise Discourse::NotFound - end - - def create_upload(current_user, file, url, type, for_private_message, pasted) + def self.create_upload(current_user:, file:, url:, type:, for_private_message:, pasted:, is_api:, retain_hours:) 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 diff --git a/lib/hijack.rb b/lib/hijack.rb index 1d057336534..1eb27a0cf37 100644 --- a/lib/hijack.rb +++ b/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 diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 2b2b664fa94..988b7f2dc45 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/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 + post :create, params: { file: logo, type: "avatar", format: :json } 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 + post :create, params: { file: text_file, type: "composer", format: :json } 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 + post :create, params: { + file: logo, + retain_hours: 100, + type: "profile_background", + format: :json + } - 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 + post :create, params: { type: "composer", format: :json } - expect(response.status).to eq 200 - expect(message.data["errors"]).to contain_exactly(I18n.t("upload.file_missing")) + message = JSON.parse(response.body) + 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 + post :create, params: { file: text_file, type: "avatar", format: :json } - expect(response.status).to eq 200 - expect(message.data["errors"]).to be + expect(response.status).to eq 422 + 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 + post :create, params: { + file: text_file, + type: "composer", + for_private_message: "true", + format: :json + } 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 + post :create, params: { file: fake_jpg, type: "composer", format: :json } - expect(response.status).to eq 200 - expect(message.data["errors"]).to contain_exactly(I18n.t("upload.images.size_not_found")) + expect(response.status).to eq 422 + message = JSON.parse(response.body)["errors"] + expect(message).to contain_exactly(I18n.t("upload.images.size_not_found")) end end