diff --git a/app/models/external_upload_stub.rb b/app/models/external_upload_stub.rb index baa9bbfb9c1..29d79cc36eb 100644 --- a/app/models/external_upload_stub.rb +++ b/app/models/external_upload_stub.rb @@ -21,7 +21,7 @@ class ExternalUploadStub < ActiveRecord::Base where( "status = ? AND created_at <= ?", ExternalUploadStub.statuses[:created], - CREATED_EXPIRY_HOURS.hours.ago, + (SiteSetting.enable_upload_debug_mode ? 48 : CREATED_EXPIRY_HOURS).hours.ago, ) end diff --git a/lib/external_upload_helpers.rb b/lib/external_upload_helpers.rb index 823925afc1f..92a20503b8f 100644 --- a/lib/external_upload_helpers.rb +++ b/lib/external_upload_helpers.rb @@ -136,7 +136,8 @@ module ExternalUploadHelpers ExternalUploadStub.find_by(unique_identifier: unique_identifier, created_by: current_user) return render_404 if external_upload_stub.blank? - return render_404 if !multipart_upload_exists?(external_upload_stub) + message = check_multipart_upload_exists(external_upload_stub) + return render_404(message) if message.present? store = multipart_store(external_upload_stub.upload_type) @@ -152,7 +153,7 @@ module ExternalUploadHelpers render json: { presigned_urls: presigned_urls } end - def multipart_upload_exists?(external_upload_stub) + def check_multipart_upload_exists(external_upload_stub) store = multipart_store(external_upload_stub.upload_type) begin store.list_multipart_parts( @@ -161,16 +162,20 @@ module ExternalUploadHelpers max_parts: 1, ) rescue Aws::S3::Errors::NoSuchUpload => err - debug_upload_error( - err, - I18n.t( - "upload.external_upload_not_found", - additional_detail: "path: #{external_upload_stub.key}", - ), + return( + debug_upload_error( + err, + I18n.t( + "upload.external_upload_not_found", + additional_detail: "path: #{external_upload_stub.key}", + ), + ) ) - return false end - true + + # Intentional to indicate that there is no error, if there is a message + # then there is an error. + nil end def abort_multipart @@ -226,7 +231,8 @@ module ExternalUploadHelpers ExternalUploadStub.find_by(unique_identifier: unique_identifier, created_by: current_user) return render_404 if external_upload_stub.blank? - return render_404 if !multipart_upload_exists?(external_upload_stub) + message = check_multipart_upload_exists(external_upload_stub) + return render_404(message) if message.present? store = multipart_store(external_upload_stub.upload_type) parts = @@ -353,9 +359,14 @@ module ExternalUploadHelpers end def debug_upload_error(err, friendly_message) - return if !SiteSetting.enable_upload_debug_mode - Discourse.warn_exception(err, message: friendly_message) - (Rails.env.development? || Rails.env.test?) ? friendly_message : I18n.t("upload.failed") + return I18n.t("upload.failed") if !SiteSetting.enable_upload_debug_mode + Discourse.warn_exception(err, message: "[ExternalUploadError] #{friendly_message}") + + if Rails.env.local? || is_api? + friendly_message + else + I18n.t("upload.failed") + end end def multipart_store(upload_type) @@ -385,7 +396,11 @@ module ExternalUploadHelpers metadata.permit("sha1-checksum").to_h end - def render_404 - raise Discourse::NotFound + def render_404(message = nil) + if message + render_json_error(message, status: 404) + else + raise Discourse::NotFound + end end end diff --git a/spec/jobs/clean_up_uploads_spec.rb b/spec/jobs/clean_up_uploads_spec.rb index 654309b4841..42fef8e1063 100644 --- a/spec/jobs/clean_up_uploads_spec.rb +++ b/spec/jobs/clean_up_uploads_spec.rb @@ -404,4 +404,20 @@ RSpec.describe Jobs::CleanUpUploads do Jobs::CleanUpUploads.new.execute(nil) expect(ExternalUploadStub.pluck(:id)).to contain_exactly(external_stub1.id, external_stub3.id) end + + it "does not delete create external upload stubs for 2 days if debug mode is on" do + SiteSetting.enable_upload_debug_mode = true + external_stub1 = + Fabricate( + :external_upload_stub, + status: ExternalUploadStub.statuses[:created], + created_at: 2.hours.ago, + ) + Jobs::CleanUpUploads.new.execute(nil) + expect(ExternalUploadStub.pluck(:id)).to contain_exactly(external_stub1.id) + + SiteSetting.enable_upload_debug_mode = false + Jobs::CleanUpUploads.new.execute(nil) + expect(ExternalUploadStub.pluck(:id)).to be_empty + end end