Do not override message errors and use magic message or default instead (#316)

Author: mullermp

gems/smithy-client/lib/smithy-client/rpc_v2_cbor/error_handler.rb

@@ -10,9 +10,9 @@ def call(context)
           # 200 range for error handling only for this case.
           @handler.call(context).on_done(200..599) do |response|
             if !valid_response?(context)
-              code, message, data = http_status_error(context)
-              response.error = build_error(context, code, message, data)
-            elsif (300..599).cover?(context.http_response.status_code)
+              code, data = http_status_error(context)
+              response.error = build_error(context, code, data)
+            elsif (400..599).cover?(context.http_response.status_code)
               response.error = error(context)
             end
           end
@@ -26,60 +26,52 @@ def valid_response?(context)
           req_header == resp_header
         end
 
-        # TODO: Fix this
-        # This is not correct per protocol tests. Some headers will determine the error code.
-        # If the body is empty, there is still potentially an error code from the header, but
-        # we are making a generic http status error instead. In a new major version, we should
-        # always try to extract header, and during extraction, check headers and body.
         def error(context)
           body = context.http_response.body.read
           if body.empty?
-            code, message, data = http_status_error(context)
+            code, data = http_status_error(context)
           else
-            code, message, data = extract_error(body, context)
+            code, data = extract_error(body, context)
           end
-          build_error(context, code, message, data)
+          build_error(context, code, data)
         end
 
         def extract_error(body, context)
           data = CBOR.decode(body)
-          type = data['__type']
-          code = error_code(type, context)
-          message = data['message']
-          data = parse_error_data(context, body, type)
-          [code, message, data]
+          code = error_code(context, data)
+          data = parse_error_data(context, body, code)
+          [code, data]
         rescue CBOR::Error
-          [http_status_error_code(context), '', Schema::EmptyStructure.new]
+          [http_status_error_code(context), Schema::EmptyStructure.new]
         end
 
         def parse_error_data(context, body, code)
           data = Schema::EmptyStructure.new
           context.operation.errors.each do |ref|
-            next unless ref.shape.id == code
+            next unless ref.shape.name == code
 
             data = context.config.cbor_codec.deserialize(ref, body, ref.shape.type.new)
           end
           data
         end
 
-        def error_code(type, context)
-          return type.split('#').last if type
-
-          http_status_error_code(context)
+        def error_code(context, data)
+          code = data['__type']
+          code ||= http_status_error_code(context)
+          code.split('#').last.split('$').first
         end
 
-        def build_error(context, code, message, data)
+        def build_error(context, code, data)
           errors_module = context.client.class.errors_module
-          errors_module.error_class(code).new(context, message, data)
+          errors_module.error_class(code).new(context, data)
         end
 
         def http_status_error(context)
-          [http_status_error_code(context), '', Schema::EmptyStructure.new]
+          [http_status_error_code(context), Schema::EmptyStructure.new]
         end
 
         def http_status_error_code(context)
-          status_code = context.http_response.status_code
-          "HTTP#{status_code}Error"
+          "HTTP#{context.http_response.status_code}Error"
         end
       end
     end

gems/smithy-client/lib/smithy-client/service_error.rb

@@ -8,14 +8,13 @@ module Client
     # service and not one generated by the client.
     class ServiceError < RuntimeError
       # @param [Smithy::Client::HandlerContext] context
-      # @param [String] message
       # @param [Structure] data
-      def initialize(context, message, data)
+      def initialize(context, data)
         @code = self.class.code
         @context = context
-        @message = extract_message(message, data)
         @data = data
-        super(@message)
+        message = data.message if data.respond_to?(:message)
+        super(message)
       end
 
       # @return [String, nil] The error code returned by the service.
@@ -25,10 +24,6 @@ def initialize(context, message, data)
       #  that triggered the remote service to return this error.
       attr_reader :context
 
-      # @return [String] The error message returned by the service.
-      #  Defaults to the class name if no message is provided or can be parsed.
-      attr_reader :message
-
       # @return [Structure] Additional data returned by the service.
       attr_accessor :data
 
@@ -46,12 +41,6 @@ def retryable?
       def throttling?
         false
       end
-
-      private
-
-      def extract_message(message, data)
-        message || (data.message if data.respond_to?(:message)) || self.class.name.to_s
-      end
     end
   end
 end

gems/smithy-client/lib/smithy-client/stubbing/rpc_v2_cbor.rb

@@ -19,7 +19,7 @@ def stub_error(_config, error_code)
           response.status_code = 400
           response.headers['Smithy-Protocol'] = 'rpc-v2-cbor'
           response.headers['Content-Type'] = 'application/cbor'
-          data = { '__type' => error_code, 'message' => 'stubbed-error-message' }
+          data = { '__type' => "smithy.ruby.tests##{error_code}", 'message' => 'stubbed-error-message' }
           response.body = CBOR.encode(data)
           response
         end

gems/smithy-client/sig/smithy-client/service_error.rbs

@@ -1,11 +1,10 @@
 module Smithy
   module Client
     class ServiceError < RuntimeError
-      def initialize: (HandlerContext?, String?, untyped?) -> void
+      def initialize: (HandlerContext, Schema::Structure) -> void
 
       attr_reader code: String?
-      attr_reader context: HandlerContext?
-      attr_reader message: String?
+      attr_reader context: HandlerContext
       attr_accessor data: untyped?
 
       def self.code: () -> String?

gems/smithy-client/spec/smithy-client/http/error_inspector_spec.rb

@@ -4,11 +4,9 @@ module Smithy
   module Client
     module HTTP
       describe ErrorInspector do
-        let(:error) do
-          ServiceError.new(HandlerContext.new, 'error', {})
-        end
-        let(:status_code) { 404 }
+        let(:error) { ServiceError.new(HandlerContext.new, Schema::EmptyStructure.new) }
         let(:http_response) { HTTP::Response.new(status_code: status_code) }
+        let(:status_code) { 404 }
 
         subject { ErrorInspector.new(error, http_response) }
 

gems/smithy-client/spec/smithy-client/plugins/retry_errors_spec.rb

@@ -89,8 +89,9 @@ module Plugins
             config.build!
           end
 
-          let(:response) { Response.new(context: HandlerContext.new(config: config)) }
-          let(:service_error) { ServiceError.new(nil, nil, nil) }
+          let(:context) { HandlerContext.new(config: config) }
+          let(:response) { Response.new(context: context) }
+          let(:service_error) { ServiceError.new(context, Schema::EmptyStructure.new) }
 
           let(:retry_strategy) { config.retry_strategy }
           let(:quota) { retry_strategy.instance_variable_get(:@quota) }

gems/smithy-client/spec/smithy-client/service_error_spec.rb

@@ -6,44 +6,38 @@ module Smithy
   module Client
     describe ServiceError do
       let(:context) { HandlerContext.new }
-      let(:message) { 'message' }
       let(:data) { double('Structure') }
       let(:code) { 'ServiceErrorCode' }
 
-      subject { ServiceError.new(context, message, data) }
+      subject { ServiceError.new(context, data) }
 
-      before do
-        ServiceError.code = code
-      end
+      before { ServiceError.code = code }
 
       it 'is a subclass of RuntimeError' do
         expect(ServiceError.superclass).to be(RuntimeError)
       end
 
       describe '#initialize' do
-        it 'sets the code' do
+        it 'sets the code using the class accessor' do
           expect(subject.code).to be(ServiceError.code)
         end
 
         it 'sets the context' do
           expect(subject.context).to be(context)
         end
 
-        it 'sets the message' do
-          expect(subject.message).to be(message)
-        end
-
-        it 'defaults the message to the class name' do
-          error = ServiceError.new(context, nil, data)
-          expect(error.message).to include('ServiceError')
-        end
-
         it 'sets the data' do
           expect(subject.data).to be(data)
         end
 
-        it 'calls super with the message' do
-          expect { raise subject }.to raise_error(RuntimeError, message)
+        it 'parses a message from data' do
+          data = double('Structure', message: 'message')
+          subject = ServiceError.new(context, data)
+          expect(subject.message).to be('message')
+        end
+
+        it 'defaults the message to the class name' do
+          expect(subject.message).to include('ServiceError')
         end
       end
 

gems/smithy/lib/smithy/templates/client/errors.erb

@@ -37,18 +37,17 @@ module <%= module_name %>
 <% end -%>
     class <%= error.name %> < Smithy::Client::ServiceError
       # @param [Smithy::Client::HandlerContext] context
-      # @param [String] message
       # @param [<%= module_name %>::Types::<%= error.name %>] data
-      def initialize(context, message, data = Smithy::Schema::EmptyStructure.new)
-        super(context, message, data)
+      def initialize(context, data = Smithy::Schema::EmptyStructure.new)
+        super(context, data)
       end
 <% error.members.each do |member| -%>
 
 <% member.docstrings.each do |docstring| -%>
       # <%= docstring %>
 <% end -%>
       def <%= member.name %>
-        <%= '@message || ' if member.message? %>@data[:<%= member.name %>]
+        @data[:<%= member.name %>]
       end
 <% end -%>
 <% if error.retryable? -%>

gems/smithy/lib/smithy/views/client/errors.rb

@@ -66,10 +66,6 @@ def initialize(name, shape)
 
             attr_reader :shape
 
-            def message?
-              @name == 'message'
-            end
-
             def docstrings
               @shape
                 .fetch('traits', {})

gems/smithy/spec/interfaces/client/errors_spec.rb

@@ -49,22 +49,15 @@
 
       it 'generates errors with messages from data' do
         data = Errors::Types::ServiceError.new(message: 'message')
-        error = Errors::Errors::ServiceError.new(nil, nil, data)
+        error = Errors::Errors::ServiceError.new(nil, data)
         expect(error.message).to eq('message')
         expect { raise error }.to raise_error(Errors::Errors::ServiceError, 'message')
       end
 
-      it 'allows overriding the message' do
-        data = Errors::Types::ServiceError.new(message: 'message')
-        error = Errors::Errors::ServiceError.new(nil, 'new message', data)
-        expect(error.message).to eq('new message')
-        expect { raise error }.to raise_error(Errors::Errors::ServiceError, 'new message')
-      end
-
       it 'can return data members' do
         structure = Errors::Types::Structure.new(value: 'foo')
         data = Errors::Types::ServiceError.new(structure: structure)
-        error = Errors::Errors::ServiceError.new(nil, nil, data)
+        error = Errors::Errors::ServiceError.new(nil, data)
         expect(error.data.structure.value).to eq('foo')
       end