Fixed an issue with block reads and non-200 responses.

Fixes amazon-archives/aws-sdk-core-ruby#207

CHANGELOG.md

@@ -1,6 +1,10 @@
 Unreleased Changes
 ------------------
 
+* Issue - Aws::S3 - Resolved an issue where calling `Aws::S3::Client#get_object`
+  with a block would yield error data for non-200 HTTP responses.
+  Resolves [GitHub issue #207](https://github.com/aws/aws-sdk-core-ruby/issues/207).
+
 * Issue - CloudFormation - Resolved an issue with `Aws::CloudFormation` resource
   interfaces for accessing associated "has" associations. Fixes
   [GitHub issue #209](https://github.com/aws/aws-sdk-core-ruby/issues/209).

aws-sdk-core/features/s3/objects.feature

@@ -27,6 +27,10 @@ Feature: S3 Objects
     Then the body should be an IO object
     And the body#read method should return "hello"
 
+  Scenario: Using block mode with GET object and errors
+    When I get an object that doesn't exist with a read block
+    Then an error should be raise and the block should not yield
+
   @paging
   Scenario: Paging responses
     Given I put nothing to the key "photos/camping/cascades.jpg"

aws-sdk-core/features/s3/step_definitions.rb

@@ -215,3 +215,19 @@ def create_bucket(options = {})
   url = "https://#{@bucket_name}.s3.amazonaws.com/#{key}"
   @resp = Net::HTTP.get(URI(url))
 end
+
+When(/^I get an object that doesn't exist with a read block$/) do
+  @yielded = []
+  begin
+    @client.get_object(bucket: @bucket_name, key: 'bad-key') do |chunk|
+      @yielded << chunk
+    end
+  rescue => error
+    @error = error
+  end
+end
+
+Then(/^an error should be raise and the block should not yield$/) do
+  expect(@error).to be_kind_of(Aws::S3::Errors::NoSuchKey)
+  expect(@yielded).to eq([])
+end

aws-sdk-core/lib/aws-sdk-core/plugins/stub_responses.rb

@@ -69,10 +69,10 @@ def streaming?(resp)
 
         def stub_http_body(resp)
           payload = resp.context.operation.output.payload
-          body = resp.context.http_response.body
-          body.write(resp.data[payload])
-          body.rewind if body.respond_to?(:rewind)
-          resp.data[payload] = body
+          resp.context.http_response.signal_headers(200, {})
+          resp.context.http_response.signal_data(resp.data[payload])
+          resp.context.http_response.signal_done
+          resp.data[payload] = resp.context.http_response.body
         end
 
       end

aws-sdk-core/lib/seahorse.rb

@@ -48,6 +48,7 @@ module Plugins
       autoload :ParamConversion, 'seahorse/client/plugins/param_conversion'
       autoload :ParamValidation, 'seahorse/client/plugins/param_validation'
       autoload :RaiseResponseErrors, 'seahorse/client/plugins/raise_response_errors'
+      autoload :ResponseTarget, 'seahorse/client/plugins/response_target'
       autoload :RestfulBindings, 'seahorse/client/plugins/restful_bindings'
     end
 

aws-sdk-core/lib/seahorse/client/base.rb

@@ -13,6 +13,7 @@ class Base
         Plugins::ParamConversion,
         Plugins::ParamValidation,
         Plugins::RaiseResponseErrors,
+        Plugins::ResponseTarget,
       ])
 
       # @api private

aws-sdk-core/lib/seahorse/client/plugins/response_target.rb

@@ -0,0 +1,58 @@
+module Seahorse
+  module Client
+    module Plugins
+      # @api private
+      class ResponseTarget < Plugin
+
+        # This handler is responsible for replacing the HTTP response body IO
+        # object with custom targets, such as a block, or a file. It is important
+        # to not write data to the custom target in the case of a non-success
+        # response. We do not want to write an XML error message to someone's
+        # file.
+        class Handler < Client::Handler
+
+          def call(context)
+            target = context.params.delete(:response_target)
+            target ||= context[:response_target]
+            add_event_listeners(context, target) if target
+            @handler.call(context)
+          end
+
+          private
+
+          def add_event_listeners(context, target)
+            handler = self
+            context.http_response.on_headers(200) do
+              context.http_response.body = handler.send(:io, target)
+            end
+
+            context.http_response.on_success(200) do
+              body = context.http_response.body
+              if ManagedFile === body && body.open?
+                body.close
+              end
+            end
+
+            context.http_response.on_error do
+              body = context.http_response.body
+              File.unlink(body) if ManagedFile === body
+              context.http_response.body = StringIO.new
+            end
+          end
+
+          def io(target)
+            case target
+            when Proc then BlockIO.new(&target)
+            when String, Pathname then ManagedFile.new(target, 'w+b')
+            else target
+            end
+          end
+
+        end
+
+        handler(Handler, step: :initialize, priority: 90)
+
+      end
+    end
+  end
+end

aws-sdk-core/lib/seahorse/client/request.rb

@@ -66,35 +66,8 @@ def initialize(handlers, context)
       # @return [Response]
       #
       def send_request(options = {}, &block)
-        set_response_target(options, &block)
+        @context[:response_target] = options[:target] || block
         @handlers.to_stack.call(@context)
-      ensure
-        close_managed_files
-      end
-
-      private
-
-      def set_response_target(options, &block)
-        target = options[:target]
-        target ||= block
-        target ||= context.params.delete(:response_target)
-        if target
-          @context.http_response.body =
-            case target
-            when Proc then BlockIO.new(&target)
-            when String, Pathname then ManagedFile.new(target, 'w+b')
-            else target
-          end
-        end
-      end
-
-      def close_managed_files
-        [
-          @context.http_request.body,
-          @context.http_response.body,
-        ].each do |io|
-          io.close if io.is_a?(ManagedFile) && io.open?
-        end
       end
 
     end

aws-sdk-core/spec/seahorse/client/base_spec.rb

@@ -272,6 +272,7 @@ module Client
             Plugins::ParamConversion,
             Plugins::ParamValidation,
             Plugins::RaiseResponseErrors,
+            Plugins::ResponseTarget,
           ])
         end
 

aws-sdk-core/spec/seahorse/client/request_spec.rb

@@ -68,15 +68,18 @@ module Client
 
         let(:handler) do
           Proc.new do
-            context.http_response.body.write('part1')
-            context.http_response.body.write('part2')
-            context.http_response.body.write('part3')
+            context.http_response.signal_headers(200, {})
+            context.http_response.signal_data('part1')
+            context.http_response.signal_data('part2')
+            context.http_response.signal_data('part3')
+            context.http_response.signal_done
             Response.new(context: context)
           end
         end
 
         before(:each) do
-          allow(handlers).to receive(:to_stack).and_return(handler)
+          handlers.add(Plugins::ResponseTarget::Handler, step: :initialize)
+          handlers.add(double('send-handler-class', new: handler))
         end
 
         describe 'String target' do