diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cc22f37a..887135197 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ * [#2698](https://github.com/ruby-grape/grape/pull/2698): Collapse `DSL::RequestResponse#extract_handler` type-dispatch into a `case`/`when` - [@ericproulx](https://github.com/ericproulx). * [#2697](https://github.com/ruby-grape/grape/pull/2697): Extract `Grape::Util::PathNormalizer` from `Grape::Router`; `Grape::Router.normalize_path` is now a deprecated alias - [@ericproulx](https://github.com/ericproulx). * [#2696](https://github.com/ruby-grape/grape/pull/2696): Reduce per-request allocations on the request hot path; migrate middleware options to `attr_reader` and freeze `@options` post-init - [@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` - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/UPGRADING.md b/UPGRADING.md index dea74a8a5..93fafc50e 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -29,6 +29,12 @@ end Reading `options[...]` is unchanged. +#### Throw `:error` payloads are now `Grape::Exceptions::ErrorResponse` + +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. + +Returning or throwing a `Hash` with `:message`, `:status`, and `:headers` from a `rescue_from` handler is now deprecated and will be removed in a future release. Use `error!(...)` or return/throw a `Grape::Exceptions::ErrorResponse` instead. + #### `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/error_response.rb b/lib/grape/exceptions/error_response.rb new file mode 100644 index 000000000..7b4e07c6c --- /dev/null +++ b/lib/grape/exceptions/error_response.rb @@ -0,0 +1,64 @@ +# 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 to_s + "#<#{self.class.name} status=#{status.inspect} message=#{message.inspect} headers=#{headers.inspect}>" + end + + def ==(other) + eql?(other) + end + + def eql?(other) + self.class == other.class && + other.status == status && + other.message == message && + other.headers == headers && + other.backtrace == backtrace && + other.original_exception == original_exception + end + + def hash + [self.class, status, message, headers, backtrace, original_exception].hash + 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 58b73993b..ca412b65a 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -62,11 +62,12 @@ def format_message(message, backtrace, original_exception = nil) formatter = Grape::ErrorFormatter.formatter_for(current_format, error_formatters, default_error_formatter) return formatter.call(message, backtrace, options, env, original_exception) if formatter - throw :error, - status: 406, - message: "The requested format '#{current_format}' is not supported.", - backtrace:, - original_exception: + throw :error, Grape::Exceptions::ErrorResponse.new( + status: 406, + message: "The requested format '#{current_format}' is not supported.", + backtrace:, + original_exception: + ) end def find_handler(klass) @@ -76,20 +77,26 @@ def find_handler(klass) raise end - def error_response(error = {}) - status = error[:status] || 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] || 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) @@ -144,9 +151,20 @@ def error!(message, status = default_status, headers = {}, backtrace = [], origi 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 + true + when Hash + return false unless response.key?(:message) && response.key?(:status) && response.key?(:headers) + + Grape.deprecator.warn( + 'Returning or throwing a Hash from a rescue handler is deprecated. ' \ + 'Use `error!(...)` or a `Grape::Exceptions::ErrorResponse` instead.' + ) + true + else + false + end end end end diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index d51d73ffc..66dcdf9a0 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -60,7 +60,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) @@ -100,8 +100,8 @@ def read_rack_input(body) media_type = rack_request.media_type fmt = media_type ? mime_types[media_type] : default_format - throw :error, status: 415, message: "The provided content-type '#{media_type}' is not supported." unless content_type_for(fmt) - parser = Grape::Parser.parser_for fmt, parsers + 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 begin @@ -117,7 +117,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 @@ -139,7 +139,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 b2b4151ce..54032e727 100644 --- a/lib/grape/middleware/versioner/base.rb +++ b/lib/grape/middleware/versioner/base.rb @@ -48,7 +48,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/base_spec.rb b/spec/grape/exceptions/base_spec.rb index b9b8bc82d..fd8142056 100644 --- a/spec/grape/exceptions/base_spec.rb +++ b/spec/grape/exceptions/base_spec.rb @@ -17,6 +17,26 @@ it { is_expected.to eq(message) } end + describe '#[]' do + subject(:exception) { described_class.new(status: 418, message: 'a_message', headers: { 'X-Foo' => 'bar' }) } + + it 'reads status via symbol index' do + expect(exception[:status]).to eq(418) + end + + it 'reads message via symbol index' do + expect(exception[:message]).to eq('a_message') + end + + it 'reads headers via symbol index' do + expect(exception[:headers]).to eq('X-Foo' => 'bar') + end + + it 'raises NoMethodError for an unknown index' do + expect { exception[:unknown] }.to raise_error(NoMethodError) + end + end + describe '#compose_message' do subject { described_class.new.__send__(:compose_message, key, **attributes) } diff --git a/spec/grape/exceptions/error_response_spec.rb b/spec/grape/exceptions/error_response_spec.rb new file mode 100644 index 000000000..b3da66f76 --- /dev/null +++ b/spec/grape/exceptions/error_response_spec.rb @@ -0,0 +1,107 @@ +# 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 '#to_s' do + it 'renders status, message, and headers in a readable form' do + headers = { 'X-Foo' => 'bar' } + payload = described_class.new(status: 422, message: 'boom', headers:) + + expect(payload.to_s).to eq(%(#)) + end + end + + describe '#==' do + let(:exception) { StandardError.new('inner') } + let(:attrs) { { status: 422, message: 'boom', headers: { 'X-Foo' => 'bar' }, backtrace: ['line 1'], original_exception: exception } } + let(:payload) { described_class.new(**attrs) } + let(:twin) { described_class.new(**attrs) } + + it 'is equal when every attribute matches' do + expect(payload).to eq(twin) + end + + it 'is not equal when any attribute differs' do + expect(payload).not_to eq(described_class.new(**attrs, status: 500)) + end + + it 'is not equal to a non-ErrorResponse with the same shape' do + expect(described_class.new(status: 422)).not_to eq(Object.new) + end + + it 'returns the same hash for equal instances' do + expect(payload.hash).to eq(twin.hash) + 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/error_spec.rb b/spec/grape/middleware/error_spec.rb index e12a76156..58633af83 100644 --- a/spec/grape/middleware/error_spec.rb +++ b/spec/grape/middleware/error_spec.rb @@ -64,4 +64,38 @@ def call(_env) expect(last_response.body).to eq({ code: 200 }.to_json) end end + + context 'when a rescue handler returns a Hash with :message, :status, :headers' do + let(:raising_app) do + Class.new do + def self.call(_env) + raise StandardError, 'boom' + end + end + end + + let(:options) do + { + rescue_handlers: { + StandardError => -> { { message: 'oops', status: 500, headers: {} } } + } + } + end + + let(:app) do + opts = options + context = self + Rack::Builder.app do + use Spec::Support::EndpointFaker + use Grape::Middleware::Error, **opts # rubocop:disable RSpec/DescribedClass + run context.raising_app + end + end + + it 'emits a deprecation warning' do + expect { get '/' }.to raise_error( + ActiveSupport::DeprecationException, /rescue handler is deprecated/ + ) + end + end end diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb index b0ece4a91..9cf382a50 100644 --- a/spec/grape/middleware/formatter_spec.rb +++ b/spec/grape/middleware/formatter_spec.rb @@ -297,8 +297,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 @@ -481,9 +481,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