From 69a5b963a2b2c377ae8f12558bcfa837d2601e32 Mon Sep 17 00:00:00 2001 From: Julik Tarkhanov Date: Mon, 11 Mar 2024 19:50:29 +0100 Subject: [PATCH] Replace ZipGenerator with ZipHandler Move more work into ZipKit, since it already handles the streaming body and the headers. What is important here is that we handle the `files` correctly - which is what the new ZipHandler will do, inside of the streaming ZipKit body. This reduces the number of interacting classes and objects and the number of conditionals. ZipKit, in turn, will now not apply the `chunked` transfer encoding unless explicitly asked to. This is a follow-through on https://github.com/julik/zip_kit/issues/2 because using ZipKit with an HTTP/2 webserver requires the transfer encoding to be disabled - HTTP/2 framing will be used instead. Following the suggestions from Samuel I have removed the chunking, but it is still possible to force it. Logging streaming exceptions is also easier now, as the entire block handling the ZIP is inside the `each` iteration. There is no tempfile spooling either because it changed the flow drastically (the file would write out at body instantiation, not at iteration over the response) --- lib/zipline.rb | 28 +++----- .../{zip_generator.rb => zip_handler.rb} | 70 +++++-------------- ..._generator_spec.rb => zip_handler_spec.rb} | 28 ++++---- spec/lib/zipline/zipline_spec.rb | 15 ++-- zipline.gemspec | 2 +- 5 files changed, 52 insertions(+), 91 deletions(-) rename lib/zipline/{zip_generator.rb => zip_handler.rb} (61%) rename spec/lib/zipline/{zip_generator_spec.rb => zip_handler_spec.rb} (86%) diff --git a/lib/zipline.rb b/lib/zipline.rb index 594e4c4..4e0552f 100644 --- a/lib/zipline.rb +++ b/lib/zipline.rb @@ -1,7 +1,7 @@ require 'content_disposition' require 'zip_kit' require 'zipline/version' -require 'zipline/zip_generator' +require 'zipline/zip_handler' # class MyController < ApplicationController # include Zipline @@ -12,23 +12,17 @@ # end # end module Zipline - def zipline(files, zipname = 'zipline.zip', **kwargs_for_new) - headers['Content-Disposition'] = ContentDisposition.format(disposition: 'attachment', filename: zipname) - headers['Content-Type'] = Mime::Type.lookup_by_extension('zip').to_s - response.sending_file = true - response.cache_control[:public] ||= false + def self.included(into_controller) + into_controller.include(ZipKit::RailsStreaming) + super + end - # Disables Rack::ETag if it is enabled (prevent buffering) - # see https://github.com/rack/rack/issues/1619#issuecomment-606315714 - self.response.headers['Last-Modified'] = Time.now.httpdate - if request.get_header("HTTP_VERSION") == "HTTP/1.0" - # If HTTP/1.0 is used it is not possible to stream, and if that happens it usually will be - # unclear why buffering is happening. Some info in the log is the least one can do. - logger.warn { "The downstream HTTP proxy/LB insists on HTTP/1.0 protocol, ZIP response will be buffered." } if logger + def zipline(files, zipname = 'zipline.zip', **kwargs_for_zip_kit_stream) + zip_kit_stream(filename: zipname, **kwargs_for_zip_kit_stream) do |zip_kit_streamer| + handler = Zipline::ZipHandler.new(zip_kit_streamer, logger) + files.each do |file, name, options = {}| + handler.handle_file(file, name.to_s, options) + end end - - zip_generator = ZipGenerator.new(request.env, files, **kwargs_for_new) - response.headers.merge!(zip_generator.headers) - self.response_body = zip_generator end end diff --git a/lib/zipline/zip_generator.rb b/lib/zipline/zip_handler.rb similarity index 61% rename from lib/zipline/zip_generator.rb rename to lib/zipline/zip_handler.rb index cd15801..8c51357 100644 --- a/lib/zipline/zip_generator.rb +++ b/lib/zipline/zip_handler.rb @@ -1,34 +1,21 @@ -# this class acts as a streaming body for rails -# initialize it with an array of the files you want to zip module Zipline - class ZipGenerator + class ZipHandler # takes an array of pairs [[uploader, filename], ... ] - def initialize(env, files, **kwargs_for_streamer) - zip_enumerator = ZipKit::OutputEnumerator.new(**kwargs_for_streamer) do |streamer| - files.each do |file, name, options = {}| - handle_file(streamer, file, name.to_s, options) - end - end - - @headers, @body = recording_stream_exceptions do - # If buffering is used, the streaming block will be executed immediately to compute Content-Length - zip_enumerator.to_headers_and_rack_response_body(env) - end + def initialize(streamer, logger) + @streamer = streamer + @logger = logger end - def each(&block) - return to_enum(:each) unless block_given? - recording_stream_exceptions { @body.each(&block) } - 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 + def handle_file(file, name, options) + normalized_file = normalize(file) + write_file(normalized_file, name, options) + 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 file #{name.inspect}" + @logger.error(error_message) if @logger + raise end # This extracts either a url or a local file from the provided file. @@ -68,8 +55,8 @@ def normalize(file) end end - def write_file(streamer, file, name, options) - streamer.write_file(name, **options.slice(:modification_time)) do |writer_for_file| + def write_file(file, name, options) + @streamer.write_file(name, **options.slice(:modification_time)) do |writer_for_file| if file[:url] the_remote_uri = URI(file[:url]) @@ -89,37 +76,12 @@ def write_file(streamer, file, name, options) end end - def headers - @headers - end - private - def recording_stream_exceptions - yield - 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 is_io?(io_ish) io_ish.respond_to? :read end - 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/zip_generator_spec.rb b/spec/lib/zipline/zip_handler_spec.rb similarity index 86% rename from spec/lib/zipline/zip_generator_spec.rb rename to spec/lib/zipline/zip_handler_spec.rb index c56db71..7b396db 100644 --- a/spec/lib/zipline/zip_generator_spec.rb +++ b/spec/lib/zipline/zip_handler_spec.rb @@ -18,7 +18,7 @@ def to_s end end -describe Zipline::ZipGenerator do +describe Zipline::ZipHandler do before { Fog.mock! } let(:file_attributes){ { key: 'fog_file_tests', @@ -38,21 +38,21 @@ def to_s let(:file){ directory.files.create(file_attributes) } describe '.normalize' do - let(:generator){ Zipline::ZipGenerator.new(_env = {}, _files = [])} + let(:handler){ Zipline::ZipHandler.new(_streamer = double(), _logger = nil)} context "CarrierWave" do context "Remote" do let(:file){ CarrierWave::Storage::Fog::File.new(nil,nil,nil) } it "extracts the url" do allow(file).to receive(:url).and_return('fakeurl') expect(File).not_to receive(:open) - expect(generator.normalize(file)).to eq({url: 'fakeurl'}) + expect(handler.normalize(file)).to eq({url: 'fakeurl'}) end end context "Local" do let(:file){ CarrierWave::SanitizedFile.new(Tempfile.new('t')) } it "creates a File" do allow(file).to receive(:path).and_return('spec/fakefile.txt') - normalized = generator.normalize(file) + normalized = handler.normalize(file) expect(normalized.keys).to include(:file) expect(normalized[:file]).to be_a File end @@ -66,7 +66,7 @@ def to_s allow(uploader).to receive(:file).and_return(file) allow(file).to receive(:url).and_return('fakeurl') expect(File).not_to receive(:open) - expect(generator.normalize(uploader)).to eq({url: 'fakeurl'}) + expect(handler.normalize(uploader)).to eq({url: 'fakeurl'}) end end @@ -75,7 +75,7 @@ def to_s it "creates a File" do allow(uploader).to receive(:file).and_return(file) allow(file).to receive(:path).and_return('spec/fakefile.txt') - normalized = generator.normalize(uploader) + normalized = handler.normalize(uploader) expect(normalized.keys).to include(:file) expect(normalized[:file]).to be_a File end @@ -87,7 +87,7 @@ def to_s let(:file){ Paperclip::Attachment.new(:name, :instance) } it "creates a File" do allow(file).to receive(:path).and_return('spec/fakefile.txt') - normalized = generator.normalize(file) + normalized = handler.normalize(file) expect(normalized.keys).to include(:file) expect(normalized[:file]).to be_a File end @@ -97,7 +97,7 @@ def to_s it "creates a URL" do allow(file).to receive(:expiring_url).and_return('fakeurl') expect(File).to_not receive(:open) - expect(generator.normalize(file)).to include(url: 'fakeurl') + expect(handler.normalize(file)).to include(url: 'fakeurl') end end end @@ -107,7 +107,7 @@ def to_s attached = create_attached_one allow_any_instance_of(Object).to receive(:defined?).and_return(true) - normalized = generator.normalize(attached) + normalized = handler.normalize(attached) expect(normalized.keys).to include(:blob) expect(normalized[:blob]).to be_a(ActiveStorage::Blob) @@ -119,7 +119,7 @@ def to_s attachment = create_attachment allow_any_instance_of(Object).to receive(:defined?).and_return(true) - normalized = generator.normalize(attachment) + normalized = handler.normalize(attachment) expect(normalized.keys).to include(:blob) expect(normalized[:blob]).to be_a(ActiveStorage::Blob) @@ -131,7 +131,7 @@ def to_s blob = create_blob allow_any_instance_of(Object).to receive(:defined?).and_return(true) - normalized = generator.normalize(blob) + normalized = handler.normalize(blob) expect(normalized.keys).to include(:blob) expect(normalized[:blob]).to be_a(ActiveStorage::Blob) @@ -169,19 +169,19 @@ def create_filename it "extracts url" do allow(file).to receive(:url).and_return('fakeurl') expect(File).not_to receive(:open) - expect(generator.normalize(file)).to eq(url: 'fakeurl') + expect(handler.normalize(file)).to eq(url: 'fakeurl') end end context "IOStream" do let(:file){ StringIO.new('passthrough')} it "passes through" do - expect(generator.normalize(file)).to eq(file: file) + expect(handler.normalize(file)).to eq(file: file) end end context "invalid" do let(:file){ Thread.new{} } it "raises error" do - expect{generator.normalize(file)}.to raise_error(ArgumentError) + expect{handler.normalize(file)}.to raise_error(ArgumentError) end end end diff --git a/spec/lib/zipline/zipline_spec.rb b/spec/lib/zipline/zipline_spec.rb index cc88cc3..db2196e 100644 --- a/spec/lib/zipline/zipline_spec.rb +++ b/spec/lib/zipline/zipline_spec.rb @@ -2,7 +2,10 @@ require 'action_controller' describe Zipline do - before { Fog.mock! } + before do + Fog.mock! + FakeController.logger = nil + end class FakeController < ActionController::Base include Zipline @@ -29,7 +32,7 @@ def download_zip_with_error_during_streaming end end - it 'passes keyword parameters to ZipKit::Streamer' do + it 'passes keyword parameters to ZipKit::OutputEnumerator' do fake_rack_env = { "HTTP_VERSION" => "HTTP/1.0", "REQUEST_METHOD" => "GET", @@ -39,7 +42,7 @@ def download_zip_with_error_during_streaming "SERVER_NAME" => "host.example", "rack.input" => StringIO.new, } - expect(ZipKit::Streamer).to receive(:new).with(anything, auto_rename_duplicate_filenames: false).and_call_original + expect(ZipKit::OutputEnumerator).to receive(:new).with(auto_rename_duplicate_filenames: false).and_call_original status, headers, body = FakeController.action(:download_zip).call(fake_rack_env) @@ -57,8 +60,10 @@ def download_zip_with_error_during_streaming "rack.input" => StringIO.new, } fake_logger = double() - expect(Logger).to receive(:new).and_return(fake_logger) - expect(fake_logger).to receive(:error).with(instance_of(String)) + allow(fake_logger).to receive(:warn) + expect(fake_logger).to receive(:error).with(a_string_matching(/when serving the ZIP/)) + + FakeController.logger = fake_logger expect { status, headers, body = FakeController.action(:download_zip_with_error_during_streaming).call(fake_rack_env) diff --git a/zipline.gemspec b/zipline.gemspec index 85577bb..331682c 100644 --- a/zipline.gemspec +++ b/zipline.gemspec @@ -20,7 +20,7 @@ Gem::Specification.new do |gem| gem.add_dependency 'actionpack', ['>= 6.0', '< 8.0'] gem.add_dependency 'content_disposition', '~> 1.0' - gem.add_dependency 'zip_kit', ['~> 6', '>= 6.0.1', '< 7'] + gem.add_dependency 'zip_kit', ['~> 6', '>= 6.2.0', '< 7'] gem.add_development_dependency 'rspec', '~> 3' gem.add_development_dependency 'fog-aws'