Merge pull request #100 from julik/replace-zip-tricks-with-zip-kit

Replace zip_tricks with zip_kit

README.md

@@ -7,7 +7,7 @@ A gem to stream dynamically generated zip files from a rails application. Unlike
 - Removes need for large disk space or memory allocation to generate zips, even huge zips. So it works on Heroku.
 - The user begins downloading immediately, which decreaceses latency, download time, and timeouts on Heroku.
 
-Zipline now depends on [zip tricks](https://github.com/WeTransfer/zip_tricks), and you might want to just use that directly if you have more advanced use cases.
+Zipline now depends on [zip_kit](https://github.com/julik/zip_kit), and you might want to just use that directly if you have more advanced use cases.
 
 ## Installation
 
@@ -43,7 +43,7 @@ class MyController < ApplicationController
     files = users.map{ |user| [user.avatar, "#{user.username}.png", modification_time: 1.day.ago] }
 
     # we can force duplicate file names to be renamed, or raise an error
-    # we can also pass in our own writer if required to conform with the Delegated [ZipTricks::Streamer object](https://github.com/WeTransfer/zip_tricks/blob/main/lib/zip_tricks/streamer.rb#L147) object.
+    # we can also pass in our own writer if required to conform with the delegated [ZipKit::Streamer object](https://github.com/julik/zip_kit/blob/main/lib/zip_kit/streamer.rb#L147) object.
     zipline(files, 'avatars.zip', auto_rename_duplicate_filenames: true) 
   end
 end
@@ -93,7 +93,7 @@ For directories, just give the files names like "directory/file".
 
 ```Ruby
 avatars = [
-  # remote_url                          zip_path             zip_tricks_options
+  # remote_url                          zip_path             write_file options for Streamer
   [ 'http://www.example.com/user1.png', 'avatars/user1.png', modification_time: Time.now.utc ]
   [ 'http://www.example.com/user2.png', 'avatars/user2.png', modification_time: 1.day.ago ]
   [ 'http://www.example.com/user3.png', 'avatars/user3.png' ]

lib/zipline.rb

@@ -1,9 +1,7 @@
 require 'content_disposition'
+require 'zip_kit'
 require 'zipline/version'
-require 'zip_tricks'
 require 'zipline/zip_generator'
-require 'zipline/chunked_body'
-require 'zipline/tempfile_body'
 
 # class MyController < ApplicationController
 #   include Zipline
@@ -15,7 +13,6 @@
 # end
 module Zipline
   def zipline(files, zipname = 'zipline.zip', **kwargs_for_new)
-    zip_generator = ZipGenerator.new(files, **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
@@ -24,32 +21,14 @@ def zipline(files, zipname = 'zipline.zip', **kwargs_for_new)
     # 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
-
-      # If we use Rack::ContentLength it would run through our ZIP block twice - once to calculate the content length
-      # of the response, and once - to serve. We can trade performance for disk space and buffer the response into a Tempfile
-      # since we are already buffering.
-      tempfile_body = TempfileBody.new(request.env, zip_generator)
-      headers["Content-Length"] = tempfile_body.size.to_s
-      headers["X-Zipline-Output"] = "buffered"
-      self.response_body = tempfile_body
-    else
-      # Disable buffering for both nginx and Google Load Balancer, see
-      # https://cloud.google.com/appengine/docs/flexible/how-requests-are-handled?tab=python#x-accel-buffering
-      response.headers["X-Accel-Buffering"] = "no"
-
-      # Make sure Rack::ContentLength does not try to compute a content length,
-      # and remove the one already set
-      headers.delete("Content-Length")
-
-      # and send out in chunked encoding
-      headers["Transfer-Encoding"] = "chunked"
-      headers["X-Zipline-Output"] = "streamed"
-      self.response_body = Chunked.new(zip_generator)
     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/version.rb

@@ -1,3 +1,3 @@
 module Zipline
-  VERSION = "1.6.0"
+  VERSION = "1.7.0"
 end

lib/zipline/zip_generator.rb

@@ -3,25 +3,22 @@
 module Zipline
   class ZipGenerator
     # takes an array of pairs [[uploader, filename], ... ]
-    def initialize(files,  **kwargs_for_streamer)
-      # Use RackBody as it has buffering built-in in zip_tricks 5.x+
-      @body = ZipTricks::RackBody.new(**kwargs_for_streamer) do |streamer|
+    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
     end
 
     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
+      recording_stream_exceptions { @body.each(&block) }
     end
 
     def handle_file(streamer, file, name, options)
@@ -72,7 +69,7 @@ def normalize(file)
     end
 
     def write_file(streamer, file, name, options)
-      streamer.write_deflated_file(name, **options.slice(:modification_time)) do |writer_for_file|
+      streamer.write_file(name, **options.slice(:modification_time)) do |writer_for_file|
         if file[:url]
           the_remote_uri = URI(file[:url])
 
@@ -92,12 +89,27 @@ def write_file(streamer, file, name, options)
       end
     end
 
-    def is_io?(io_ish)
-      io_ish.respond_to? :read
+    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

spec/lib/zipline/zip_generator_spec.rb

@@ -38,7 +38,7 @@ def to_s
   let(:file){ directory.files.create(file_attributes) }
 
   describe '.normalize' do
-    let(:generator){ Zipline::ZipGenerator.new([])}
+    let(:generator){ Zipline::ZipGenerator.new(_env = {}, _files = [])}
     context "CarrierWave" do
       context "Remote" do
         let(:file){ CarrierWave::Storage::Fog::File.new(nil,nil,nil) }

spec/lib/zipline/zipline_spec.rb

@@ -29,7 +29,7 @@ def download_zip_with_error_during_streaming
     end
   end
 
-  it 'passes keyword parameters to ZipTricks::Streamer' do
+  it 'passes keyword parameters to ZipKit::Streamer' do
     fake_rack_env = {
       "HTTP_VERSION" => "HTTP/1.0",
       "REQUEST_METHOD" => "GET",
@@ -39,7 +39,7 @@ def download_zip_with_error_during_streaming
       "SERVER_NAME" => "host.example",
       "rack.input" => StringIO.new,
     }
-    expect(ZipTricks::Streamer).to receive(:new).with(anything, auto_rename_duplicate_filenames: false).and_call_original
+    expect(ZipKit::Streamer).to receive(:new).with(anything, auto_rename_duplicate_filenames: false).and_call_original
 
     status, headers, body = FakeController.action(:download_zip).call(fake_rack_env)
 
@@ -56,13 +56,13 @@ def download_zip_with_error_during_streaming
       "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)
+      status, headers, body = FakeController.action(:download_zip_with_error_during_streaming).call(fake_rack_env)
+      body.each { }
     }.to raise_error(/Something wonky/)
   end
 end

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_tricks', ['>= 4.8.3', '< 6'] # Minimum to 4.8.3 which is the last-released MIT version
+  gem.add_dependency 'zip_kit', ['~> 6', '>= 6.0.1', '< 7']
 
   gem.add_development_dependency 'rspec', '~> 3'
   gem.add_development_dependency 'fog-aws'