write directly to zipstream

fixes CNVS-37802

Change-Id: I715f6e6c2a45c505e1f3c9e3757ea5cfce0e632f
Reviewed-on: https://gerrit.instructure.com/116890
Reviewed-by: Cody Cutrer <cody@instructure.com>
QA-Review: Pedro Fajardo <pfajardo@instructure.com>
Product-Review: Dan Minkevitch <dan@instructure.com>
Reviewed-by: Dan Minkevitch <dan@instructure.com>
Tested-by: Jenkins

app/models/attachments/s3_storage.rb

@@ -97,23 +97,30 @@ class Attachments::S3Storage
   end
 
   def open(opts, &block)
-    if block_given?
-      attachment.s3object.get(&block)
-    else
-      # TODO: !need_local_file -- net/http and thus AWS::S3::S3Object don't
-      # natively support streaming the response, except when a block is given.
-      # so without Fibers, there's not a great way to return an IO-like object
-      # that streams the response. A separate thread, I guess. Bleck. Need to
-      # investigate other options.
-      if opts[:temp_folder].present? && !File.exist?(opts[:temp_folder])
-        FileUtils.mkdir_p(opts[:temp_folder])
-      end
-      tempfile = Tempfile.new(["attachment_#{attachment.id}", attachment.extension],
-                              opts[:temp_folder].presence || Dir::tmpdir)
-      tempfile.binmode
-      attachment.s3object.get(response_target: tempfile)
-      tempfile.rewind
-      tempfile
+    # TODO: !need_local_file -- net/http and thus AWS::S3::S3Object don't
+    # natively support streaming the response, except when a block is given.
+    # so without Fibers, there's not a great way to return an IO-like object
+    # that streams the response. A separate thread, I guess. Bleck. Need to
+    # investigate other options.
+    if opts[:temp_folder].present? && !File.exist?(opts[:temp_folder])
+      FileUtils.mkdir_p(opts[:temp_folder])
     end
+    tempfile = Tempfile.new(["attachment_#{attachment.id}", attachment.extension],
+                            opts[:temp_folder].presence || Dir::tmpdir)
+    tempfile.binmode
+    attachment.s3object.get(response_target: tempfile)
+    tempfile.rewind
+
+    if block_given?
+      File.open(tempfile.path, 'rb') do |file|
+        chunk = file.read(64000)
+        while chunk
+          yield chunk
+          chunk = file.read(64000)
+        end
+      end
+    end
+
+    tempfile
   end
 end

spec/models/attachment_spec.rb

@@ -1484,7 +1484,16 @@ describe Attachment do
 
       it "should stream data to the block given" do
         callback = false
-        @attachment.s3object.class.any_instance.expects(:get).yields("test")
+        data = ["test", false]
+        Tempfile.any_instance.expects(:binmode).once
+        Tempfile.any_instance.expects(:rewind).once
+        Tempfile.any_instance.expects(:path).once
+
+        # We specify at_least(2) here because the stub will return "test"
+        # first, then on the second run will be falsey, therefore
+        # ending the loop and stopping the calls
+        File.expects(:open).at_least(2).yields(stub(read: -> {data.shift}))
+        @attachment.s3object.class.any_instance.expects(:get).with(has_key(:response_target))
         @attachment.open { |data| expect(data).to eq "test"; callback = true }
         expect(callback).to eq true
       end