Replace ZipGenerator with ZipHandler

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

lib/zipline/zip_generator.rb → 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

spec/lib/zipline/zip_generator_spec.rb → 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

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)

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'