Merge pull request #97 from julik/rescue-each-with-rails-log

Print into Rails log when streaming fails

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

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