From ba932c10b5e51fa6eb10dc4fbb9fe997191b077a Mon Sep 17 00:00:00 2001 From: Trevor Rowe Date: Tue, 10 Feb 2015 13:04:28 -0800 Subject: [PATCH] Fixed an issue with block reads and non-200 responses. Fixes https://github.com/aws/aws-sdk-core-ruby/issues/207 --- CHANGELOG.md | 4 ++ aws-sdk-core/features/s3/objects.feature | 4 ++ aws-sdk-core/features/s3/step_definitions.rb | 16 +++++ .../aws-sdk-core/plugins/stub_responses.rb | 8 +-- aws-sdk-core/lib/seahorse.rb | 1 + aws-sdk-core/lib/seahorse/client/base.rb | 1 + .../client/plugins/response_target.rb | 58 +++++++++++++++++++ aws-sdk-core/lib/seahorse/client/request.rb | 29 +--------- .../spec/seahorse/client/base_spec.rb | 1 + .../spec/seahorse/client/request_spec.rb | 11 ++-- 10 files changed, 97 insertions(+), 36 deletions(-) create mode 100644 aws-sdk-core/lib/seahorse/client/plugins/response_target.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ce29ad079c..1cc4aeb6b9c 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/aws-sdk-core/features/s3/objects.feature b/aws-sdk-core/features/s3/objects.feature index 3d61a19b5b5..0f4b2875d22 100644 --- a/aws-sdk-core/features/s3/objects.feature +++ b/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" diff --git a/aws-sdk-core/features/s3/step_definitions.rb b/aws-sdk-core/features/s3/step_definitions.rb index d37ebb90da0..dcac7d2dc7b 100644 --- a/aws-sdk-core/features/s3/step_definitions.rb +++ b/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 diff --git a/aws-sdk-core/lib/aws-sdk-core/plugins/stub_responses.rb b/aws-sdk-core/lib/aws-sdk-core/plugins/stub_responses.rb index c7b528c623b..62a4e21265d 100644 --- a/aws-sdk-core/lib/aws-sdk-core/plugins/stub_responses.rb +++ b/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 diff --git a/aws-sdk-core/lib/seahorse.rb b/aws-sdk-core/lib/seahorse.rb index 188c08925d1..5c11f2a2739 100644 --- a/aws-sdk-core/lib/seahorse.rb +++ b/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 diff --git a/aws-sdk-core/lib/seahorse/client/base.rb b/aws-sdk-core/lib/seahorse/client/base.rb index 9c9ee2a8e6e..7fc28d5bc15 100644 --- a/aws-sdk-core/lib/seahorse/client/base.rb +++ b/aws-sdk-core/lib/seahorse/client/base.rb @@ -13,6 +13,7 @@ class Base Plugins::ParamConversion, Plugins::ParamValidation, Plugins::RaiseResponseErrors, + Plugins::ResponseTarget, ]) # @api private diff --git a/aws-sdk-core/lib/seahorse/client/plugins/response_target.rb b/aws-sdk-core/lib/seahorse/client/plugins/response_target.rb new file mode 100644 index 00000000000..49dbbcb110d --- /dev/null +++ b/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 diff --git a/aws-sdk-core/lib/seahorse/client/request.rb b/aws-sdk-core/lib/seahorse/client/request.rb index 3cf97bbc532..3239d9849b9 100644 --- a/aws-sdk-core/lib/seahorse/client/request.rb +++ b/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 diff --git a/aws-sdk-core/spec/seahorse/client/base_spec.rb b/aws-sdk-core/spec/seahorse/client/base_spec.rb index 64fb7571a4c..7e114b5edaa 100644 --- a/aws-sdk-core/spec/seahorse/client/base_spec.rb +++ b/aws-sdk-core/spec/seahorse/client/base_spec.rb @@ -272,6 +272,7 @@ module Client Plugins::ParamConversion, Plugins::ParamValidation, Plugins::RaiseResponseErrors, + Plugins::ResponseTarget, ]) end diff --git a/aws-sdk-core/spec/seahorse/client/request_spec.rb b/aws-sdk-core/spec/seahorse/client/request_spec.rb index c29668f5790..d1d96523b80 100644 --- a/aws-sdk-core/spec/seahorse/client/request_spec.rb +++ b/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