From 60161aa197218ed68778fa21845d81ea65bed837 Mon Sep 17 00:00:00 2001
From: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Date: Wed, 22 Nov 2023 11:07:37 -0500
Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Re-arrange=20method=20stru?=
 =?UTF-8?q?cture=20in=20hopes=20of=20not=20swallowing=20exception?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We are observing that this job is raising an exception, which is then
swallowed (and "pooped out" to the mailboxer notifications for the
submitting user; which is not helpful when tracking down the
particulars).

Confounding part is that the reported errors, don't seem to be affecting
the attachment of files.  In other words, we get a notification of
failure but it worked (the majority of the time).

Co-authored-by: LaRita Robinson <larita@scientist.com>
---
 app/jobs/import_url_job_decorator.rb | 52 +++++++++++++++++++---------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/app/jobs/import_url_job_decorator.rb b/app/jobs/import_url_job_decorator.rb
index 4e41e35c..e240998d 100644
--- a/app/jobs/import_url_job_decorator.rb
+++ b/app/jobs/import_url_job_decorator.rb
@@ -1,28 +1,48 @@
 # frozen_string_literal: true
 
 # OVERRIDE in v2.9.6 of Hyrax
+# <2023-11-22 Wed> Aim is to hopefully not swallow exceptions, and get meaningful info regarding
+#                  the observed problem.
 module ImportUrlJobDecorator
-  # OVERRIDE to gain further insight into the StandardError that was reported but hidden.
-  def copy_remote_file(uri, name, headers = {})
+  def perform_af
+    name = file_set.label
+
+    copy_remote_file(name) do |f|
+      # reload the FileSet once the data is copied since this is a long running task
+      file_set.reload
+
+      # FileSetActor operates synchronously so that this tempfile is available.
+      # If asynchronous, the job might be invoked on a machine that did not have this temp file on its file system!
+      # NOTE: The return status may be successful even if the content never attaches.
+      log_import_status(f)
+    end
+  rescue => e
+    message = "ImportUrlJob: #{file_set.class} ID=#{file_set&.id} to_param=#{file_set&.to_param} with " \
+              "Parent #{file_set&.parent&.inspect} ID=#{file_set.parent&.id&.inspect} had error on URI #{uri.inspect}." \
+              "Error: #{e.class}.  Message: #{e.message}.  Backtrace:\n#{e.backtrace.join("\n")}"
+    Raven.capture_exception(e)
+    Hyrax.logger.error(message)
+    # Does send_error swallow the exception?
+    send_error(message)
+    raise e
+  end
+
+  # Download file from uri, yields a block with a file in a temporary directory.
+  # It is important that the file on disk has the same file name as the URL,
+  # because when the file in added into Fedora the file name will get persisted in the
+  # metadata.
+  # @param name [String] the human-readable name of the file
+  # @yield [IO] the stream to write to
+  def copy_remote_file(name)
     filename = File.basename(name)
     dir = Dir.mktmpdir
-    Rails.logger.debug("ImportUrlJob: Copying <#{uri}> to #{dir}")
+    Hyrax.logger.debug("ImportUrlJob: Copying <#{uri}> to #{dir}")
 
     File.open(File.join(dir, filename), 'wb') do |f|
-      begin
-        write_file(uri, f, headers)
-        yield f
-      rescue StandardError => e
-        # OVERRIDE adding Rails.logger.error call
-        Rails.logger.error(
-          %(ImportUrlJob: Error copying <#{uri}> to #{dir} with #{e.message}.  #{e.backtrace.join("\n")})
-        )
-        send_error(e.message)
-        # TODO: Should we re-raise the exception?  As written this copy_remote_file has a false
-        # success.
-      end
+      write_file(f)
+      yield f
+      Hyrax.logger.debug("ImportUrlJob: Closing #{File.join(dir, filename)}")
     end
-    Rails.logger.debug("ImportUrlJob: Copying <#{uri}> to #{dir}, closing #{File.join(dir, filename)}")
   end
 
   # OVERRIDE there are calls to send_error that send two arguments.