diff --git a/CHANGELOG.md b/CHANGELOG.md index 2652f40d7..4a40118ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ * [#2690](https://github.com/ruby-grape/grape/pull/2690): Avoid allocating an empty array on every `StackableValues#[]` miss - [@ericproulx](https://github.com/ericproulx). * [#2691](https://github.com/ruby-grape/grape/pull/2691): Precompute the prefix list in `Middleware::Versioner::Path` - [@ericproulx](https://github.com/ericproulx). * [#2692](https://github.com/ruby-grape/grape/pull/2692): Replace per-request `Proc` allocation in `Router#transaction` with a `halt?` helper - [@ericproulx](https://github.com/ericproulx). +* [#2693](https://github.com/ruby-grape/grape/pull/2693): Introduce `Grape::Exceptions::ErrorResponse` value object to replace the implicit-schema Hash thrown via `throw :error, ...` - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/UPGRADING.md b/UPGRADING.md index c078a175b..8ea46df2d 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,22 @@ Upgrading Grape ### Upgrading to >= 3.3 +#### `Grape::Exceptions::Base#[]` removed; throw payloads are now `Grape::Exceptions::ErrorResponse` + +`Grape::Exceptions::Base#[]` (which delegated to `__send__` so callers could read `exception[:status]`, `exception[:message]`, etc.) has been removed. Use the named accessors instead: + +```ruby +# Before +exception[:status] +exception[:message] + +# After +exception.status +exception.message +``` + +Internally, the payload thrown via `throw :error, ...` is now a `Grape::Exceptions::ErrorResponse` value object instead of a `Hash`. If you `catch(:error)` and inspect the payload, switch from `payload[:status]` to `payload.status` (or `payload[:message]` to `payload.message`, etc.). User-defined `throw :error, hash` calls continue to work — `Middleware::Error#error_response` coerces Hashes, exceptions, and `ErrorResponse` instances at the boundary. + #### `Grape::Request#grape_routing_args` has been removed `grape_routing_args` was previously public to support third-party `params_builder` extensions, which have since been removed. With no remaining callers, the method has been removed. If you were calling it externally, read `env[Grape::Env::GRAPE_ROUTING_ARGS]` directly. diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 89cee5c39..134645ad0 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -29,12 +29,9 @@ def configuration def error!(message, status = nil, additional_headers = nil, backtrace = nil, original_exception = nil) status = self.status(status || inheritable_setting.namespace_inheritable[:default_error_status]) headers = additional_headers.present? ? header.merge(additional_headers) : header - throw :error, - message:, - status:, - headers:, - backtrace:, - original_exception: + throw :error, Grape::Exceptions::ErrorResponse.new( + message:, status:, headers:, backtrace:, original_exception: + ) end # Redirect to a new url. diff --git a/lib/grape/exceptions/base.rb b/lib/grape/exceptions/base.rb index afe9fd4ed..055688aee 100644 --- a/lib/grape/exceptions/base.rb +++ b/lib/grape/exceptions/base.rb @@ -16,10 +16,6 @@ def initialize(status: nil, message: nil, headers: nil) @headers = headers end - def [](index) - __send__ index - end - private def compose_message(key, **) diff --git a/lib/grape/exceptions/error_response.rb b/lib/grape/exceptions/error_response.rb new file mode 100644 index 000000000..a476106e4 --- /dev/null +++ b/lib/grape/exceptions/error_response.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Grape + module Exceptions + # Value object representing the payload thrown via `throw :error, ...` + # and consumed by `Middleware::Error#error_response`. Replaces the + # implicit-schema Hash that previously circulated between throw sites + # and the error middleware. + class ErrorResponse + attr_reader :status, :message, :headers, :backtrace, :original_exception + + def initialize(status: nil, message: nil, headers: nil, backtrace: nil, original_exception: nil) + @status = status + @message = message + @headers = headers + @backtrace = backtrace + @original_exception = original_exception + end + + def self.from_exception(exception) + new( + status: exception.status, + message: exception.message, + headers: exception.headers, + backtrace: exception.backtrace, + original_exception: exception + ) + end + + # Normalize heterogeneous inputs into an ErrorResponse. Preserves the + # public contract that users can still `throw :error, hash` from their + # own middleware or `rescue_from` handlers. + def self.coerce(input) + case input + when ErrorResponse then input + when Grape::Exceptions::Base then from_exception(input) + when Hash then new(**input.slice(:status, :message, :headers, :backtrace, :original_exception)) + else new + end + end + end + end +end diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index 709166eba..fa60dfaac 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -37,11 +37,12 @@ def format_message(message, backtrace, original_exception = nil) formatter = Grape::ErrorFormatter.formatter_for(format, options[:error_formatters], options[:default_error_formatter]) return formatter.call(message, backtrace, options, env, original_exception) if formatter - throw :error, - status: 406, - message: "The requested format '#{format}' is not supported.", - backtrace:, - original_exception: + throw :error, Grape::Exceptions::ErrorResponse.new( + status: 406, + message: "The requested format '#{format}' is not supported.", + backtrace:, + original_exception: + ) end def find_handler(klass) @@ -51,20 +52,26 @@ def find_handler(klass) raise end - def error_response(error = {}) - status = error[:status] || options[:default_status] + def error_response(error = nil) + payload = Grape::Exceptions::ErrorResponse.coerce(error) + + status = payload.status || options[:default_status] env[Grape::Env::API_ENDPOINT].status(status) # error! may not have been called - message = error[:message] || options[:default_message] - headers = { Rack::CONTENT_TYPE => content_type }.tap do |h| - h.merge!(error[:headers]) if error[:headers].is_a?(Hash) - end - backtrace = error[:backtrace] || error[:original_exception]&.backtrace || [] - original_exception = error.is_a?(Exception) ? error : error[:original_exception] - rack_response(status, headers, format_message(message, backtrace, original_exception)) + message = payload.message || options[:default_message] + headers = { Rack::CONTENT_TYPE => content_type } + headers.merge!(payload.headers) if payload.headers.is_a?(Hash) + backtrace = payload.backtrace || payload.original_exception&.backtrace || [] + rack_response(status, headers, format_message(message, backtrace, payload.original_exception)) end def default_rescue_handler(exception) - error_response(message: exception.message, backtrace: exception.backtrace, original_exception: exception) + error_response( + Grape::Exceptions::ErrorResponse.new( + message: exception.message, + backtrace: exception.backtrace, + original_exception: exception + ) + ) end def registered_rescue_handler(klass) @@ -119,9 +126,11 @@ def error!(message, status = options[:default_status], headers = {}, backtrace = end def error?(response) - return false unless response.is_a?(Hash) - - response.key?(:message) && response.key?(:status) && response.key?(:headers) + case response + when Grape::Exceptions::ErrorResponse then true + when Hash then response.key?(:message) && response.key?(:status) && response.key?(:headers) + else false + end end end end diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index d3e47518d..40850b47f 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -46,7 +46,7 @@ def build_formatted_response(status, headers, bodies) Rack::Response.new(bodymap, status, headers) end rescue Grape::Exceptions::InvalidFormatter => e - throw :error, status: 500, message: e.message, backtrace: e.backtrace, original_exception: e + throw :error, Grape::Exceptions::ErrorResponse.new(status: 500, message: e.message, backtrace: e.backtrace, original_exception: e) end def fetch_formatter(headers, options) @@ -86,7 +86,7 @@ def read_rack_input(body) media_type = rack_request.media_type fmt = media_type ? mime_types[media_type] : options[:default_format] - throw :error, status: 415, message: "The provided content-type '#{media_type}' is not supported." unless content_type_for(fmt) + throw :error, Grape::Exceptions::ErrorResponse.new(status: 415, message: "The provided content-type '#{media_type}' is not supported.") unless content_type_for(fmt) parser = Grape::Parser.parser_for fmt, options[:parsers] return env[Grape::Env::API_REQUEST_BODY] = body unless parser @@ -103,7 +103,7 @@ def read_rack_input(body) rescue Grape::Exceptions::Base => e raise e rescue StandardError => e - throw :error, status: 400, message: e.message, backtrace: e.backtrace, original_exception: e + throw :error, Grape::Exceptions::ErrorResponse.new(status: 400, message: e.message, backtrace: e.backtrace, original_exception: e) end end @@ -125,7 +125,7 @@ def negotiate_content_type if content_type_for(fmt) env[Grape::Env::API_FORMAT] = fmt.to_sym else - throw :error, status: 406, message: "The requested format '#{fmt}' is not supported." + throw :error, Grape::Exceptions::ErrorResponse.new(status: 406, message: "The requested format '#{fmt}' is not supported.") end end diff --git a/lib/grape/middleware/versioner/accept_version_header.rb b/lib/grape/middleware/versioner/accept_version_header.rb index eca0c6342..c6605c344 100644 --- a/lib/grape/middleware/versioner/accept_version_header.rb +++ b/lib/grape/middleware/versioner/accept_version_header.rb @@ -30,7 +30,7 @@ def before private def not_acceptable!(message) - throw :error, status: 406, headers: error_headers, message: + throw :error, Grape::Exceptions::ErrorResponse.new(status: 406, headers: error_headers, message:) end end end diff --git a/lib/grape/middleware/versioner/base.rb b/lib/grape/middleware/versioner/base.rb index 91188be21..40986543d 100644 --- a/lib/grape/middleware/versioner/base.rb +++ b/lib/grape/middleware/versioner/base.rb @@ -51,7 +51,7 @@ def potential_version_match?(potential_version) end def version_not_found! - throw :error, status: 404, message: '404 API Version Not Found', headers: CASCADE_PASS_HEADER + throw :error, Grape::Exceptions::ErrorResponse.new(status: 404, message: '404 API Version Not Found', headers: CASCADE_PASS_HEADER) end private diff --git a/spec/grape/exceptions/error_response_spec.rb b/spec/grape/exceptions/error_response_spec.rb new file mode 100644 index 000000000..1f4918c8c --- /dev/null +++ b/spec/grape/exceptions/error_response_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +describe Grape::Exceptions::ErrorResponse do + describe '#initialize' do + it 'accepts all known fields and exposes them as readers' do + payload = described_class.new( + status: 422, + message: 'boom', + headers: { 'X-Foo' => 'bar' }, + backtrace: ['line 1'], + original_exception: StandardError.new('inner') + ) + + expect(payload.status).to eq(422) + expect(payload.message).to eq('boom') + expect(payload.headers).to eq('X-Foo' => 'bar') + expect(payload.backtrace).to eq(['line 1']) + expect(payload.original_exception).to be_a(StandardError) + end + + it 'defaults all fields to nil' do + payload = described_class.new + + expect(payload.status).to be_nil + expect(payload.message).to be_nil + expect(payload.headers).to be_nil + expect(payload.backtrace).to be_nil + expect(payload.original_exception).to be_nil + end + end + + describe '.from_exception' do + it 'extracts status, message, headers, and backtrace from a Grape exception' do + exception = Grape::Exceptions::Base.new(status: 418, message: 'teapot', headers: { 'X-T' => '1' }) + payload = described_class.from_exception(exception) + + expect(payload.status).to eq(418) + expect(payload.message).to eq('teapot') + expect(payload.headers).to eq('X-T' => '1') + expect(payload.original_exception).to eq(exception) + end + end + + describe '.coerce' do + it 'returns the input unchanged when it is already an ErrorResponse' do + input = described_class.new(status: 500) + + expect(described_class.coerce(input)).to equal(input) + end + + it 'wraps a Grape exception via from_exception' do + exception = Grape::Exceptions::Base.new(status: 404, message: 'gone') + payload = described_class.coerce(exception) + + expect(payload).to be_a(described_class) + expect(payload.status).to eq(404) + expect(payload.message).to eq('gone') + expect(payload.original_exception).to eq(exception) + end + + it 'builds a new ErrorResponse from a Hash, picking only known keys' do + payload = described_class.coerce(status: 503, message: 'down', irrelevant: 'ignored') + + expect(payload.status).to eq(503) + expect(payload.message).to eq('down') + end + + it 'returns an empty ErrorResponse for unsupported input' do + payload = described_class.coerce(nil) + + expect(payload).to be_a(described_class) + expect(payload.status).to be_nil + end + end +end diff --git a/spec/grape/exceptions/invalid_accept_header_spec.rb b/spec/grape/exceptions/invalid_accept_header_spec.rb index 29884f0dd..a74ced5d4 100644 --- a/spec/grape/exceptions/invalid_accept_header_spec.rb +++ b/spec/grape/exceptions/invalid_accept_header_spec.rb @@ -44,7 +44,7 @@ before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false subject.rescue_from :all do |e| - error! 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e.headers end subject.get '/beer' do 'beer received' @@ -114,7 +114,7 @@ def app before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false subject.rescue_from :all do |e| - error! 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e.headers end subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], @@ -194,7 +194,7 @@ def app before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true subject.rescue_from :all do |e| - error! 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e.headers end subject.get '/beer' do 'beer received' @@ -273,7 +273,7 @@ def app before do subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true subject.rescue_from :all do |e| - error! 'message was processed', 400, e[:headers] + error! 'message was processed', 400, e.headers end subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb index dda260449..500f2289c 100644 --- a/spec/grape/middleware/formatter_spec.rb +++ b/spec/grape/middleware/formatter_spec.rb @@ -299,8 +299,8 @@ def to_xml 'CONTENT_LENGTH' => io.length.to_s ) end - expect(error[:status]).to eq(415) - expect(error[:message]).to eq("The provided content-type 'application/atom+xml' is not supported.") + expect(error.status).to eq(415) + expect(error.message).to eq("The provided content-type 'application/atom+xml' is not supported.") end end end @@ -483,9 +483,9 @@ def self.call(_, _) ) end - expect(error[:message]).to eq 'fail' - expect(error[:backtrace].size).to be >= 1 - expect(error[:original_exception].class).to eq StandardError + expect(error.message).to eq 'fail' + expect(error.backtrace.size).to be >= 1 + expect(error.original_exception.class).to eq StandardError end end end diff --git a/spec/grape/middleware/versioner/accept_version_header_spec.rb b/spec/grape/middleware/versioner/accept_version_header_spec.rb index cf5fab10a..3c8624dd0 100644 --- a/spec/grape/middleware/versioner/accept_version_header_spec.rb +++ b/spec/grape/middleware/versioner/accept_version_header_spec.rb @@ -19,9 +19,11 @@ end it 'does not raise an error' do - expect do - subject.call('HTTP_ACCEPT_VERSION' => "\x80") - end.to throw_symbol(:error, status: 406, headers: { 'X-Cascade' => 'pass' }, message: 'The requested version is not supported.') + payload = catch(:error) { subject.call('HTTP_ACCEPT_VERSION' => "\x80") } + expect(payload).to be_a(Grape::Exceptions::ErrorResponse) + expect(payload.status).to eq(406) + expect(payload.headers).to eq('X-Cascade' => 'pass') + expect(payload.message).to eq('The requested version is not supported.') end end @@ -43,14 +45,11 @@ end it 'fails with 406 Not Acceptable if version is not supported' do - expect do - subject.call('HTTP_ACCEPT_VERSION' => 'v2').last - end.to throw_symbol( - :error, - status: 406, - headers: { 'X-Cascade' => 'pass' }, - message: 'The requested version is not supported.' - ) + payload = catch(:error) { subject.call('HTTP_ACCEPT_VERSION' => 'v2').last } + expect(payload).to be_a(Grape::Exceptions::ErrorResponse) + expect(payload.status).to eq(406) + expect(payload.headers).to eq('X-Cascade' => 'pass') + expect(payload.message).to eq('The requested version is not supported.') end end @@ -72,25 +71,19 @@ end it 'fails with 406 Not Acceptable if header is not set' do - expect do - subject.call({}).last - end.to throw_symbol( - :error, - status: 406, - headers: { 'X-Cascade' => 'pass' }, - message: 'Accept-Version header must be set.' - ) + payload = catch(:error) { subject.call({}).last } + expect(payload).to be_a(Grape::Exceptions::ErrorResponse) + expect(payload.status).to eq(406) + expect(payload.headers).to eq('X-Cascade' => 'pass') + expect(payload.message).to eq('Accept-Version header must be set.') end it 'fails with 406 Not Acceptable if header is empty' do - expect do - subject.call('HTTP_ACCEPT_VERSION' => '').last - end.to throw_symbol( - :error, - status: 406, - headers: { 'X-Cascade' => 'pass' }, - message: 'Accept-Version header must be set.' - ) + payload = catch(:error) { subject.call('HTTP_ACCEPT_VERSION' => '').last } + expect(payload).to be_a(Grape::Exceptions::ErrorResponse) + expect(payload.status).to eq(406) + expect(payload.headers).to eq('X-Cascade' => 'pass') + expect(payload.message).to eq('Accept-Version header must be set.') end it 'succeeds if proper header is set' do @@ -106,25 +99,19 @@ end it 'fails with 406 Not Acceptable if header is not set' do - expect do - subject.call({}).last - end.to throw_symbol( - :error, - status: 406, - headers: {}, - message: 'Accept-Version header must be set.' - ) + payload = catch(:error) { subject.call({}).last } + expect(payload).to be_a(Grape::Exceptions::ErrorResponse) + expect(payload.status).to eq(406) + expect(payload.headers).to eq({}) + expect(payload.message).to eq('Accept-Version header must be set.') end it 'fails with 406 Not Acceptable if header is empty' do - expect do - subject.call('HTTP_ACCEPT_VERSION' => '').last - end.to throw_symbol( - :error, - status: 406, - headers: {}, - message: 'Accept-Version header must be set.' - ) + payload = catch(:error) { subject.call('HTTP_ACCEPT_VERSION' => '').last } + expect(payload).to be_a(Grape::Exceptions::ErrorResponse) + expect(payload.status).to eq(406) + expect(payload.headers).to eq({}) + expect(payload.message).to eq('Accept-Version header must be set.') end it 'succeeds if proper header is set' do diff --git a/spec/grape/middleware/versioner/param_spec.rb b/spec/grape/middleware/versioner/param_spec.rb index 00099dfc9..d772c610b 100644 --- a/spec/grape/middleware/versioner/param_spec.rb +++ b/spec/grape/middleware/versioner/param_spec.rb @@ -42,7 +42,7 @@ it 'throws an error if a non-allowed version is specified' do env = Rack::MockRequest.env_for('/awesome', params: { 'apiver' => 'v3' }) - expect(catch(:error) { subject.call(env) }[:status]).to eq(404) + expect(catch(:error) { subject.call(env) }.status).to eq(404) end it 'allows versions that have been specified' do diff --git a/spec/grape/middleware/versioner/path_spec.rb b/spec/grape/middleware/versioner/path_spec.rb index 8aec7d748..c91d316a8 100644 --- a/spec/grape/middleware/versioner/path_spec.rb +++ b/spec/grape/middleware/versioner/path_spec.rb @@ -35,7 +35,7 @@ let(:options) { { versions: } } it 'throws an error if a non-allowed version is specified' do - expect(catch(:error) { subject.call(Rack::PATH_INFO => '/v3/awesome') }[:status]).to eq(404) + expect(catch(:error) { subject.call(Rack::PATH_INFO => '/v3/awesome') }.status).to eq(404) end it 'allows versions that have been specified' do