From daa06a1c00692456c37ea1874ce705244e32038a Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Fri, 30 Aug 2024 10:25:04 +1000
Subject: [PATCH] DEV: Improve external upload debugging (#28627)

* Do not delete created external upload stubs for 2 days
  instead of 1 hour if enable_upload_debug_mode is true,
  this aids with server-side debugging.
* If using an API call, return the detailed error message
  if enable_upload_debug_mode is true. In this case the user
  is not using the UI, so a more detailed message is appropriate.
* Add a prefix to log messages in ExternalUploadHelpers, to
  make it easier to find these in logster.
---
 app/models/external_upload_stub.rb |  2 +-
 lib/external_upload_helpers.rb     | 47 ++++++++++++++++++++----------
 spec/jobs/clean_up_uploads_spec.rb | 16 ++++++++++
 3 files changed, 48 insertions(+), 17 deletions(-)

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