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