From c5a2e28ecf57ca74a5df29d844ed4d75a53b372d Mon Sep 17 00:00:00 2001
From: Julik Tarkhanov <me@julik.nl>
Date: Mon, 12 Feb 2024 14:40:32 +0100
Subject: [PATCH] Print into Rails log when streaming fails

It can happen that the serving gets aborted, and in a lot of cases streaming Rack bodies will not relay exceptions into the APM system like Appsignal etc. It can be useful to at least print the message into the log then.

Closes https://github.com/fringd/zipline/issues/74
---
 lib/zipline/zip_generator.rb     | 22 +++++++++++++++++++++
 spec/lib/zipline/zipline_spec.rb | 34 ++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/lib/zipline/zip_generator.rb b/lib/zipline/zip_generator.rb
index cefa0e0..61fd861 100644
--- a/lib/zipline/zip_generator.rb
+++ b/lib/zipline/zip_generator.rb
@@ -15,11 +15,23 @@ def initialize(files,  **kwargs_for_streamer)
     def each(&block)
       return to_enum(:each) unless block_given?
       @body.each(&block)
+    rescue => e
+      # Since most APM packages do not trace errors occurring within streaming
+      # Rack bodies, it can be helpful to print the error to the Rails log at least
+      error_message = "zipline: an exception (#{e.inspect}) was raised  when serving the ZIP body."
+      error_message += " The error occurred when handling #{@filename.inspect}" if @filename
+      logger.error(error_message)
+      raise
     end
 
     def handle_file(streamer, file, name, options)
       file = normalize(file)
+
+      # Store the filename so that a sensible error message can be displayed in the log
+      # if writing this particular file fails
+      @filename = name
       write_file(streamer, file, name, options)
+      @filename = nil
     end
 
     # This extracts either a url or a local file from the provided file.
@@ -86,6 +98,16 @@ def is_io?(io_ish)
 
     private
 
+    def logger
+      # Rails is not defined in our tests, and might as well not be defined
+      # elsewhere - or the logger might not be configured correctly
+      if defined?(Rails.logger) && Rails.logger
+        Rails.logger
+      else
+        Logger.new($stderr)
+      end
+    end
+
     def is_active_storage_attachment?(file)
       defined?(ActiveStorage::Attachment) && file.is_a?(ActiveStorage::Attachment)
     end
diff --git a/spec/lib/zipline/zipline_spec.rb b/spec/lib/zipline/zipline_spec.rb
index e49b4ca..b828a39 100644
--- a/spec/lib/zipline/zipline_spec.rb
+++ b/spec/lib/zipline/zipline_spec.rb
@@ -13,6 +13,20 @@ def download_zip
       ]
       zipline(files, 'myfiles.zip', auto_rename_duplicate_filenames: false)
     end
+
+    class FailingIO < StringIO
+      def read(*)
+        raise "Something wonky"
+      end
+    end
+
+    def download_zip_with_error_during_streaming
+      files = [
+        [StringIO.new("File content goes here"), "one.txt"],
+        [FailingIO.new("This will fail half-way"), "two.txt"]
+      ]
+      zipline(files, 'myfiles.zip', auto_rename_duplicate_filenames: false)
+    end
   end
 
   it 'passes keyword parameters to ZipTricks::Streamer' do
@@ -31,4 +45,24 @@ def download_zip
 
     expect(headers['Content-Disposition']).to eq("attachment; filename=\"myfiles.zip\"; filename*=UTF-8''myfiles.zip")
   end
+
+  it 'sends the exception raised in the streaming body to the Rails logger' do
+    fake_rack_env = {
+      "HTTP_VERSION" => "HTTP/1.0",
+      "REQUEST_METHOD" => "GET",
+      "SCRIPT_NAME" => "",
+      "PATH_INFO" => "/download",
+      "QUERY_STRING" => "",
+      "SERVER_NAME" => "host.example",
+      "rack.input" => StringIO.new,
+    }
+    expect(ZipTricks::Streamer).to receive(:new).with(anything, auto_rename_duplicate_filenames: false).and_call_original
+    fake_logger = double()
+    expect(Logger).to receive(:new).and_return(fake_logger)
+    expect(fake_logger).to receive(:error).with(instance_of(String))
+
+    expect {
+      FakeController.action(:download_zip_with_error_during_streaming).call(fake_rack_env)
+    }.to raise_error(/Something wonky/)
+  end
 end