diff --git a/CHANGELOG.md b/CHANGELOG.md index 61e3e6eca..486f64340 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,11 +27,14 @@ * [#2707](https://github.com/ruby-grape/grape/pull/2707): Tighten six guard conditions in `lib/` via De Morgan and `blank?`/`present?`/`include?` rewrites; no behaviour change - [@ericproulx](https://github.com/ericproulx). * [#2708](https://github.com/ruby-grape/grape/pull/2708): Tighten dynamic `define_method` in `DSL::Callbacks` and `DSL::Routing` - [@ericproulx](https://github.com/ericproulx). * [#2709](https://github.com/ruby-grape/grape/pull/2709): Lift trailing `if/else` into guard clauses; tighten `Util::Lazy::ValueEnumerable` - [@ericproulx](https://github.com/ericproulx). +* [#2716](https://github.com/ruby-grape/grape/pull/2716): Refactor `DSL::Routing#version`: guard clause, explicit kwargs in place of `**options`, and a `Grape::DSL::VersionOptions` value object stored internally - [@ericproulx](https://github.com/ericproulx). * [#2702](https://github.com/ruby-grape/grape/pull/2702): Add `oneof:` option for `requires`/`optional` to accept a Hash parameter matching one of several variant schemas (resolves [#2385](https://github.com/ruby-grape/grape/issues/2385)) - [@ericproulx](https://github.com/ericproulx). * [#2715](https://github.com/ruby-grape/grape/pull/2715): Normalize `==` / `eql?` aliasing across value-like classes - [@ericproulx](https://github.com/ericproulx). * [#2710](https://github.com/ruby-grape/grape/pull/2710): Tidy up `Grape::DeclaredParamsHandler` - [@ericproulx](https://github.com/ericproulx). +* [#2712](https://github.com/ruby-grape/grape/pull/2712): Pass a `Grape::Exceptions::ErrorResponse` value object to `error_formatter#call` instead of separate kwargs - [@ericproulx](https://github.com/ericproulx). * [#2714](https://github.com/ruby-grape/grape/pull/2714): Drop unused `Grape::Middleware::Globals` and its `grape.request*` env constants - [@ericproulx](https://github.com/ericproulx). * [#2717](https://github.com/ruby-grape/grape/pull/2717): Convert `Grape::Exceptions::ErrorResponse` to a `Data` value object - [@ericproulx](https://github.com/ericproulx). +* [#2719](https://github.com/ruby-grape/grape/pull/2719): Move content-type helpers from `Middleware::Base` into `PrecomputedContentTypes` - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/README.md b/README.md index 1ba89ffcb..963764b9b 100644 --- a/README.md +++ b/README.md @@ -2739,12 +2739,12 @@ end The error format will match the request format. See "Content-Types" below. -Custom error formatters for existing and additional types can be defined with a proc. +Custom error formatters for existing and additional types can be defined with a proc. The formatter receives a `Grape::Exceptions::ErrorResponse` value object as `error:` plus three context kwargs — `env:`, `include_backtrace:`, `include_original_exception:`. Pull just the keys you need with `**` to ignore the rest: ```ruby class Twitter::API < Grape::API - error_formatter :txt, ->(message, backtrace, options, env, original_exception) { - "error: #{message} from #{backtrace}" + error_formatter :txt, ->(error:, **) { + "error #{error.status}: #{error.message} from #{error.backtrace}" } end ``` @@ -2753,8 +2753,8 @@ You can also use a module or class. ```ruby module CustomFormatter - def self.call(message, backtrace, options, env, original_exception) - { message: message, backtrace: backtrace } + def self.call(error:, **) + { status: error.status, message: error.message, backtrace: error.backtrace } end end diff --git a/UPGRADING.md b/UPGRADING.md index 9b3e644af..6b45573ac 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -25,6 +25,55 @@ end The original implementation is preserved in git history at [`6b4111b3:lib/grape/middleware/globals.rb`](https://github.com/ruby-grape/grape/blob/6b4111b3/lib/grape/middleware/globals.rb). +#### `error_formatter` now receives a `Grape::Exceptions::ErrorResponse` value object + +Custom error formatters now receive a frozen `Grape::Exceptions::ErrorResponse` as the `error:` keyword argument, alongside three request-time context kwargs. The new signature: + +```ruby +def call(error:, env: nil, include_backtrace: false, include_original_exception: false) +``` + +`error` is the same value object the middleware uses internally, with `status` / `message` / `headers` / `backtrace` / `original_exception` accessors. The two `include_*` booleans are forwarded from the matching `rescue_from` options (previously buried inside `options[:rescue_options]`). + +Existing positional formatters break and need to be updated: + +```ruby +# Before +error_formatter :txt, ->(message, backtrace, options, env, original_exception) { ... } + +module CustomFormatter + def self.call(message, backtrace, options, env, original_exception) + ... + end +end + +# After — pick fields off `error` +error_formatter :txt, ->(error:, **) { "[#{error.status}] #{error.message}" } + +module CustomFormatter + def self.call(error:, **) + { status: error.status, message: error.message, backtrace: error.backtrace } + end +end +``` + +Migration: + +| Old positional arg | New | +| --- | --- | +| `message` | `error.message` | +| `backtrace` | `error.backtrace` | +| `original_exception` | `error.original_exception` | +| `options[:rescue_options][:backtrace]` | `include_backtrace` (kwarg) | +| `options[:rescue_options][:original_exception]` | `include_original_exception` (kwarg) | +| `env` | `env` (kwarg, still passed) | +| HTTP status | `error.status` (newly exposed) | +| Response headers | `error.headers` (newly exposed) | + +The remaining middleware-options keys (`default_status`, `format`, `rescue_handlers`, …) were framework-internal and have never been part of the documented contract. + +The change resolves [#2527](https://github.com/ruby-grape/grape/issues/2527): the HTTP `status` and the response `headers` are now part of the formatter contract, so JSON:API–style error bodies (which embed the status code) and header-aware formatters can be written without reaching into `env[Grape::Env::API_ENDPOINT]`. + #### `Grape::Middleware::Base#options` is now frozen `@options` is frozen at the end of `Grape::Middleware::Base#initialize` (after `merge_default_options`). The hash is initialized once and treated as immutable for the lifetime of the middleware. Custom middleware that mutates `options[...]` at runtime will now raise `FrozenError`. diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index c471b8766..e08d3199a 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -132,7 +132,7 @@ def call(env) def cascade? namespace_inheritable = self.class.inheritable_setting.namespace_inheritable return namespace_inheritable[:cascade] if namespace_inheritable.key?(:cascade) - return namespace_inheritable[:version_options][:cascade] if namespace_inheritable[:version_options]&.key?(:cascade) + return namespace_inheritable[:version_options].cascade if namespace_inheritable[:version_options] true end diff --git a/lib/grape/dsl/request_response.rb b/lib/grape/dsl/request_response.rb index 33e6d255e..f4963e0a7 100644 --- a/lib/grape/dsl/request_response.rb +++ b/lib/grape/dsl/request_response.rb @@ -83,12 +83,15 @@ def default_error_status(new_status = nil) # @param [Array] exception_classes A list of classes that you want to rescue, or # the symbol :all to rescue from all exceptions. # @param [Block] block Execution block to handle the given exception. - # @param [Hash] options Options for the rescue usage. - # @option options [Boolean] :backtrace Include a backtrace in the rescue response. - # @option options [Boolean] :rescue_subclasses Also rescue subclasses of exception classes - # @param [Proc] handler Execution proc to handle the given exception as an - # alternative to passing a block. - def rescue_from(*args, with: nil, **options, &block) + # @param [Proc] with Execution proc to handle the given exception as an alternative + # to passing a block. + # @param [Boolean] rescue_subclasses Also rescue subclasses of exception classes; + # defaults to +true+. + # @param [Boolean] backtrace Include the rescued exception's backtrace in the + # rescue response body. + # @param [Boolean] original_exception Include +inspect+ of the rescued exception + # in the rescue response body. + def rescue_from(*args, with: nil, rescue_subclasses: true, backtrace: false, original_exception: false, &block) handler = extract_handler(args, with:, block:) if args.include?(:all) @@ -101,18 +104,11 @@ def rescue_from(*args, with: nil, **options, &block) elsif args.include?(:internal_grape_exceptions) inheritable_setting.namespace_inheritable[:internal_grape_exceptions_rescue_handler] = handler else - handler_type = - case options[:rescue_subclasses] - when nil, true - :rescue_handlers - else - :base_only_rescue_handlers - end - + handler_type = rescue_subclasses ? :rescue_handlers : :base_only_rescue_handlers inheritable_setting.namespace_reverse_stackable[handler_type] = args.to_h { |arg| [arg, handler] } end - inheritable_setting.namespace_stackable[:rescue_options] = options + inheritable_setting.namespace_stackable[:rescue_options] = RescueOptions.new(backtrace:, original_exception:) end # Allows you to specify a default representation entity for a diff --git a/lib/grape/dsl/rescue_options.rb b/lib/grape/dsl/rescue_options.rb new file mode 100644 index 000000000..f44a27569 --- /dev/null +++ b/lib/grape/dsl/rescue_options.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Grape + module DSL + # Immutable value object holding the response-shaping booleans accepted + # by +Grape::DSL::RequestResponse#rescue_from+. Stored on the + # inheritable settings as +namespace_stackable[:rescue_options]+ and + # delegated to by +Grape::Middleware::Error+ (which forwards + # +backtrace+/+original_exception+ to the formatter as + # +include_backtrace+/+include_original_exception+). + # + # Defaults are duplicated on +#initialize+ here and on +#rescue_from+'s + # signature on purpose: keeping them on both sides means each entry point + # is self-documenting without needing to import a shared constant — the + # DSL signature shows what a user sees in the IDE, and the Data object + # has working defaults when constructed directly (middleware + # `DEFAULT_OPTIONS`, spec fixtures, etc.). The two must stay in lockstep. + RescueOptions = Data.define(:backtrace, :original_exception) do + def initialize(backtrace: false, original_exception: false) + super + end + end + end +end diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index 288efc43c..9badcf1f7 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -23,6 +23,12 @@ def cascade(value = nil) # Specify an API version. # + # Called without arguments, returns the most recently declared version + # (or +nil+). Called with one or more version strings, registers them + # and stores a {Grape::DSL::VersionOptions} value object on the + # inheritable settings; when given a block, the registration applies + # within a nested namespace. + # # @example API with legacy support. # class MyAPI < Grape::API # version 'v2' @@ -38,26 +44,41 @@ def cascade(value = nil) # end # end # - def version(*args, **options, &block) - if args.any? - options = options.reverse_merge(using: :path) - requested_versions = args.flatten.map(&:to_s) - - raise Grape::Exceptions::MissingVendorOption.new if options[:using] == :header && !options.key?(:vendor) - - @versions = versions | requested_versions - - if block - within_namespace do - inheritable_setting.namespace_inheritable[:version] = requested_versions - inheritable_setting.namespace_inheritable[:version_options] = options - - instance_eval(&block) - end - else + # @param args [Array] one or more version identifiers. + # @param using [Symbol] versioning strategy — one of +:path+ (default), + # +:header+, +:param+, or +:accept_version_header+. + # @param cascade [Boolean] forward to subsequent routes via the + # +X-Cascade+ header on version mismatch. Defaults to +true+. + # @param parameter [String] name of the query/body parameter that + # carries the version when +using: :param+. Defaults to +'apiver'+. + # @param strict [Boolean] reject requests that don't supply a usable + # version (header strategies). Defaults to +false+. + # @param vendor [String, nil] vendor segment for the +:header+ + # strategy (+application/vnd.-+); required when + # +using: :header+. + # @yield optional block to scope routes under this version. + # @return [String, nil] the most recently declared version. + # @raise [Grape::Exceptions::MissingVendorOption] when +using: :header+ + # is supplied without a +:vendor+. + def version(*args, using: :path, cascade: true, parameter: 'apiver', strict: false, vendor: nil, &block) + return @versions&.last if args.empty? + + raise Grape::Exceptions::MissingVendorOption.new if using == :header && vendor.nil? + + requested_versions = args.flatten.map(&:to_s) + options = VersionOptions.new(using:, cascade:, parameter:, strict:, vendor:) + + @versions = versions | requested_versions + + if block + within_namespace do inheritable_setting.namespace_inheritable[:version] = requested_versions inheritable_setting.namespace_inheritable[:version_options] = options + instance_eval(&block) end + else + inheritable_setting.namespace_inheritable[:version] = requested_versions + inheritable_setting.namespace_inheritable[:version_options] = options end @versions&.last diff --git a/lib/grape/dsl/version_options.rb b/lib/grape/dsl/version_options.rb new file mode 100644 index 000000000..0cd6c6598 --- /dev/null +++ b/lib/grape/dsl/version_options.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Grape + module DSL + # Immutable value object holding the resolved options from + # +Grape::DSL::Routing#version+. Stored on the inheritable settings as + # +namespace_inheritable[:version_options]+ and read by internal call + # sites (`Path`, `Endpoint`, `API::Instance#cascade?`, + # `Middleware::Versioner::Base`) via accessors. + # + # Defaults are duplicated on +#initialize+ here and on +#version+'s + # signature on purpose: keeping them on both sides means each entry point + # is self-documenting without needing to import a shared constant — the + # DSL signature shows what a user sees in the IDE, and the Data object + # has working defaults when constructed directly (middleware + # `DEFAULT_OPTIONS`, spec fixtures, etc.). The two must stay in lockstep. + VersionOptions = Data.define(:using, :cascade, :parameter, :strict, :vendor) do + def initialize(using: :path, cascade: true, parameter: 'apiver', strict: false, vendor: nil) + super + end + end + end +end diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index f65f28b47..ec6038155 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -310,9 +310,10 @@ def build_stack stack.concat inheritable_setting.namespace_stackable[:middleware] if inheritable_setting.namespace_inheritable[:version].present? - stack.use Grape::Middleware::Versioner.using(inheritable_setting.namespace_inheritable[:version_options][:using]), + version_options = inheritable_setting.namespace_inheritable[:version_options] + stack.use Grape::Middleware::Versioner.using(version_options.using), versions: inheritable_setting.namespace_inheritable[:version].flatten, - version_options: inheritable_setting.namespace_inheritable[:version_options], + version_options:, prefix: inheritable_setting.namespace_inheritable[:root_prefix], mount_path: inheritable_setting.namespace_stackable[:mount_path].first end @@ -340,7 +341,7 @@ def error_middleware_options(format, content_types) rescue_grape_exceptions: ns_inh[:rescue_grape_exceptions], default_error_formatter: ns_inh[:default_error_formatter], error_formatters: ns_stack.namespace_stackable_with_hash(:error_formatters), - rescue_options: ns_stack.namespace_stackable_with_hash(:rescue_options), + rescue_options: ns_stack.namespace_stackable[:rescue_options]&.last, rescue_handlers:, base_only_rescue_handlers: ns_stack.namespace_stackable_with_hash(:base_only_rescue_handlers), all_rescue_handler: ns_inh[:all_rescue_handler], diff --git a/lib/grape/error_formatter/base.rb b/lib/grape/error_formatter/base.rb index 050ea0603..643332841 100644 --- a/lib/grape/error_formatter/base.rb +++ b/lib/grape/error_formatter/base.rb @@ -4,11 +4,18 @@ module Grape module ErrorFormatter class Base class << self - def call(message, backtrace, options = {}, env = nil, original_exception = nil) - wrapped_message = wrap_message(present(message, env)) + # Custom error formatters override +call+. The +error+ is a frozen + # {Grape::Exceptions::ErrorResponse} carrying +status+/+message+/ + # +headers+/+backtrace+/+original_exception+. +env+ is the Rack env + # (needed by entity-presenter resolution). +include_backtrace+ and + # +include_original_exception+ are the request-time toggles set by + # +rescue_from+; the base implementation embeds the corresponding + # fields in the response body when they are true. + def call(error:, env: nil, include_backtrace: false, include_original_exception: false) + wrapped_message = wrap_message(present(error.message, env)) if wrapped_message.is_a?(Hash) - wrapped_message[:backtrace] = backtrace if backtrace.present? && options.dig(:rescue_options, :backtrace) - wrapped_message[:original_exception] = original_exception.inspect if original_exception && options.dig(:rescue_options, :original_exception) + wrapped_message[:backtrace] = error.backtrace if include_backtrace && error.backtrace.present? + wrapped_message[:original_exception] = error.original_exception.inspect if include_original_exception && error.original_exception end format_structured_message(wrapped_message) diff --git a/lib/grape/middleware/base.rb b/lib/grape/middleware/base.rb index a0c8ee9a9..56f6ebbef 100644 --- a/lib/grape/middleware/base.rb +++ b/lib/grape/middleware/base.rb @@ -8,10 +8,13 @@ class Base attr_reader :app, :env, :options # @param [Rack Application] app The standard argument for a Rack middleware. - # @param [Hash] options A hash of options, simply stored for use by subclasses. + # @param [Hash] options Options forwarded to the subclass. When the + # subclass declares an `Options` Data class, the kwargs are routed + # through it. Otherwise they are deep-merged with the subclass's + # `DEFAULT_OPTIONS` Hash (legacy path) and frozen. def initialize(app, **options) @app = app - @options = merge_default_options(options).freeze + @options = build_options(options) @app_response = nil end @@ -61,22 +64,6 @@ def response @app_response = Rack::Response[*@app_response] end - def content_types - @content_types ||= Grape::ContentTypes.content_types_for(options[:content_types]) - end - - def mime_types - @mime_types ||= Grape::ContentTypes.mime_types_for(content_types) - end - - def content_type_for(format) - content_types_indifferent_access[format] - end - - def content_type - content_type_for(env[Grape::Env::API_FORMAT] || options[:format]) || 'text/html' - end - def query_params rack_request.GET rescue *Grape::RACK_ERRORS @@ -85,10 +72,6 @@ def query_params private - def content_types_indifferent_access - @content_types_indifferent_access ||= content_types.with_indifferent_access - end - def merge_headers(response) return if @header.blank? @@ -98,6 +81,14 @@ def merge_headers(response) end end + def build_options(options) + # Search ancestors so subclasses (e.g. Versioner::Path → Versioner::Base) + # inherit their parent's Options Data class without redeclaring it. + return self.class::Options.new(**options) if self.class.const_defined?(:Options) + + merge_default_options(options).freeze + end + def merge_default_options(options) if respond_to?(:default_options) default_options.deep_merge(options) diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index 8bcdb1207..8f4338f5b 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -3,48 +3,45 @@ module Grape module Middleware class Error < Base + extend Forwardable include PrecomputedContentTypes - DEFAULT_OPTIONS = { - all_rescue_handler: nil, - base_only_rescue_handlers: nil, - default_error_formatter: nil, - default_message: '', - default_status: 500, - error_formatters: nil, - format: :txt, - grape_exceptions_rescue_handler: nil, - internal_grape_exceptions_rescue_handler: nil, - rescue_all: false, - rescue_grape_exceptions: false, - rescue_handlers: nil, - rescue_options: { - backtrace: false, - original_exception: false - }.freeze - }.freeze - - attr_reader :all_rescue_handler, :base_only_rescue_handlers, :default_error_formatter, - :default_message, :default_status, :error_formatters, :format, - :grape_exceptions_rescue_handler, :internal_grape_exceptions_rescue_handler, - :rescue_all, :rescue_grape_exceptions, :rescue_handlers - - def initialize(app, **options) - super - @all_rescue_handler = @options[:all_rescue_handler] - @base_only_rescue_handlers = @options[:base_only_rescue_handlers] - @default_error_formatter = @options[:default_error_formatter] - @default_message = @options[:default_message] - @default_status = @options[:default_status] - @error_formatters = @options[:error_formatters] - @format = @options[:format] - @grape_exceptions_rescue_handler = @options[:grape_exceptions_rescue_handler] - @internal_grape_exceptions_rescue_handler = @options[:internal_grape_exceptions_rescue_handler] - @rescue_all = @options[:rescue_all] - @rescue_grape_exceptions = @options[:rescue_grape_exceptions] - @rescue_handlers = @options[:rescue_handlers] + Options = Data.define( + :all_rescue_handler, :base_only_rescue_handlers, :content_types, + :default_error_formatter, :default_message, :default_status, + :error_formatters, :format, + :grape_exceptions_rescue_handler, :internal_grape_exceptions_rescue_handler, + :rescue_all, :rescue_grape_exceptions, :rescue_handlers, :rescue_options + ) do + def initialize( + all_rescue_handler: nil, base_only_rescue_handlers: nil, content_types: nil, + default_error_formatter: nil, default_message: '', default_status: 500, + error_formatters: nil, format: :txt, + grape_exceptions_rescue_handler: nil, internal_grape_exceptions_rescue_handler: nil, + rescue_all: false, rescue_grape_exceptions: false, rescue_handlers: nil, + rescue_options: nil + ) + # `rescue_options:` arrives nil from `Endpoint#error_middleware_options` + # when no `rescue_from` has been called — fall back to the documented + # defaults rather than letting nil propagate to `def_delegator + # :rescue_options, :backtrace`. + rescue_options ||= Grape::DSL::RescueOptions.new + super + end end + def_delegators :options, + :all_rescue_handler, :base_only_rescue_handlers, :default_error_formatter, + :default_message, :default_status, :error_formatters, :format, + :grape_exceptions_rescue_handler, :internal_grape_exceptions_rescue_handler, + :rescue_all, :rescue_grape_exceptions, :rescue_handlers, :rescue_options + + # +:backtrace+ / +:original_exception+ on the rescue options become + # +#include_backtrace+ / +#include_original_exception+ on the middleware, + # which is what the formatter call site reads. + def_delegator :rescue_options, :backtrace, :include_backtrace + def_delegator :rescue_options, :original_exception, :include_original_exception + def call!(env) @env = env error_response(catch(:error) { return @app.call(@env) }) @@ -59,16 +56,16 @@ def rack_response(status, headers, message) Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), Grape::Util::Header.new.merge(headers)) end - def format_message(message, backtrace, original_exception = nil) + def format_message(error) current_format = env[Grape::Env::API_FORMAT] || format formatter = Grape::ErrorFormatter.formatter_for(current_format, error_formatters, default_error_formatter) - return formatter.call(message, backtrace, options, env, original_exception) if formatter + return formatter.call(error:, env:, include_backtrace:, include_original_exception:) if formatter throw :error, Grape::Exceptions::ErrorResponse.new( status: 406, message: "The requested format '#{current_format}' is not supported.", - backtrace:, - original_exception: + backtrace: error.backtrace, + original_exception: error.original_exception ) end @@ -80,15 +77,17 @@ def find_handler(klass) end 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 = payload.message || options[:default_message] + raw = Grape::Exceptions::ErrorResponse.coerce(error) 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)) + headers.merge!(raw.headers) if raw.headers.is_a?(Hash) + payload = raw.with( + status: raw.status || default_status, + message: raw.message || default_message, + headers:, + backtrace: raw.backtrace || raw.original_exception&.backtrace || [] + ) + env[Grape::Env::API_ENDPOINT].status(payload.status) # error! may not have been called + rack_response(payload.status, payload.headers, format_message(payload)) end def default_rescue_handler(exception) @@ -188,10 +187,11 @@ def framework_default(endpoint) def error!(message, status = default_status, headers = {}, backtrace = [], original_exception = nil) env[Grape::Env::API_ENDPOINT].status(status) # not error! inside route - rack_response( - status, headers.reverse_merge(Rack::CONTENT_TYPE => content_type), - format_message(message, backtrace, original_exception) + merged_headers = headers.reverse_merge(Rack::CONTENT_TYPE => content_type) + error = Grape::Exceptions::ErrorResponse.new( + status:, message:, headers: merged_headers, backtrace:, original_exception: ) + rack_response(status, merged_headers, format_message(error)) end def error?(response) diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index 66dcdf9a0..1a505b938 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -3,27 +3,18 @@ module Grape module Middleware class Formatter < Base + extend Forwardable include PrecomputedContentTypes - DEFAULT_OPTIONS = { - content_types: nil, - default_format: :txt, - format: nil, - formatters: nil, - parsers: nil - }.freeze + Options = Data.define(:content_types, :default_format, :format, :formatters, :parsers) do + def initialize(content_types: nil, default_format: :txt, format: nil, formatters: nil, parsers: nil) + super + end + end ALL_MEDIA_TYPES = '*/*' - attr_reader :default_format, :format, :formatters, :parsers - - def initialize(app, **options) - super - @default_format = @options[:default_format] - @format = @options[:format] - @formatters = @options[:formatters] - @parsers = @options[:parsers] - end + def_delegators :options, :default_format, :format, :formatters, :parsers def before negotiate_content_type @@ -101,7 +92,7 @@ def read_rack_input(body) fmt = media_type ? mime_types[media_type] : default_format 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] + parser = Grape::Parser.parser_for fmt, parsers return env[Grape::Env::API_REQUEST_BODY] = body unless parser begin diff --git a/lib/grape/middleware/precomputed_content_types.rb b/lib/grape/middleware/precomputed_content_types.rb index e8dc1b641..f22d2e17c 100644 --- a/lib/grape/middleware/precomputed_content_types.rb +++ b/lib/grape/middleware/precomputed_content_types.rb @@ -2,14 +2,16 @@ module Grape module Middleware - # Include in a middleware subclass to warm the content-type caches on the - # parent instance at initialization. Per-request dups inherit the cached - # values via +dup+'s ivar copy, avoiding ~1 µs per request of - # +with_indifferent_access+ recomputation. + # Include in a middleware subclass that needs content-type negotiation. + # Provides +content_types+ / +mime_types+ / +content_type_for+ / + # +content_type+ resolved from +options.content_types+ and + # +options.format+ — so the consuming middleware's +Options+ Data class + # must declare both fields. Warms those caches on the parent instance + # at initialization so per-request +dup+s inherit them (avoiding + # ~1 µs/request of +with_indifferent_access+ recomputation). # - # Opt-in: plain +Grape::Middleware::Base+ subclasses continue to compute - # +content_types+ / +mime_types+ / +content_type_for+ lazily on first - # access. + # Opt-in: plain +Grape::Middleware::Base+ subclasses that don't need + # content-type-aware helpers don't pay for them. module PrecomputedContentTypes def initialize(app, **options) super @@ -17,6 +19,28 @@ def initialize(app, **options) mime_types content_types_indifferent_access end + + def content_types + @content_types ||= Grape::ContentTypes.content_types_for(options.content_types) + end + + def mime_types + @mime_types ||= Grape::ContentTypes.mime_types_for(content_types) + end + + def content_type_for(format) + content_types_indifferent_access[format] + end + + def content_type + content_type_for(env[Grape::Env::API_FORMAT] || options.format) || 'text/html' + end + + private + + def content_types_indifferent_access + @content_types_indifferent_access ||= content_types.with_indifferent_access + end end end end diff --git a/lib/grape/middleware/versioner/base.rb b/lib/grape/middleware/versioner/base.rb index 54032e727..59ccd16a3 100644 --- a/lib/grape/middleware/versioner/base.rb +++ b/lib/grape/middleware/versioner/base.rb @@ -4,19 +4,19 @@ module Grape module Middleware module Versioner class Base < Grape::Middleware::Base + extend Forwardable include Grape::Middleware::PrecomputedContentTypes - DEFAULT_OPTIONS = { - mount_path: nil, - pattern: /.*/i, - prefix: nil, - version_options: { - cascade: true, - parameter: 'apiver', - strict: false, - vendor: nil - }.freeze - }.freeze + Options = Data.define( + :content_types, :format, :mount_path, :pattern, :prefix, :version_options, :versions + ) do + def initialize( + content_types: nil, format: nil, mount_path: nil, pattern: /.*/i, prefix: nil, + version_options: Grape::DSL::VersionOptions.new, versions: nil + ) + super + end + end CASCADE_PASS_HEADER = { 'X-Cascade' => 'pass' }.freeze @@ -25,21 +25,15 @@ def self.inherited(klass) Versioner.register(klass) end - attr_reader :available_media_types, :cascade, :error_headers, :mount_path, :parameter, - :pattern, :prefix, :strict, :vendor, :versions + attr_reader :available_media_types, :error_headers, :versions + + def_delegators :options, :mount_path, :pattern, :prefix, :version_options + def_delegators :version_options, :cascade, :parameter, :strict, :vendor def initialize(app, **options) super - version_options = @options[:version_options] - @cascade = version_options[:cascade] - @mount_path = @options[:mount_path] - @parameter = version_options[:parameter] - @pattern = @options[:pattern] - @prefix = @options[:prefix] - @strict = version_options[:strict] - @vendor = version_options[:vendor] - @versions = @options[:versions]&.map(&:to_s) # making sure versions are strings to ease potential match - @error_headers = @cascade ? CASCADE_PASS_HEADER : {} + @versions = self.options.versions&.map(&:to_s) # making sure versions are strings to ease potential match + @error_headers = cascade ? CASCADE_PASS_HEADER : {} @available_media_types = build_available_media_types end diff --git a/lib/grape/path.rb b/lib/grape/path.rb index 516c9ef3c..27da1e46e 100644 --- a/lib/grape/path.rb +++ b/lib/grape/path.rb @@ -52,9 +52,9 @@ def uses_specific_format?(settings) end def uses_path_versioning?(settings) - return false unless settings.key?(:version) && settings[:version_options]&.key?(:using) + return false unless settings.key?(:version) && settings[:version_options] - settings[:version] && settings[:version_options][:using] == :path + settings[:version] && settings[:version_options].using == :path end def valid_part?(part) diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index c844d3906..7f7194615 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -2511,8 +2511,8 @@ def rescue_all_errors context 'class' do let(:custom_error_formatter) do Class.new do - def self.call(message, _backtrace, _options, _env, _original_exception) - "message: #{message} @backtrace" + def self.call(error:, **) + "message: #{error.message} @backtrace" end end end @@ -2548,6 +2548,21 @@ def self.call(message, _backtrace, _options, _env, _original_exception) end end + context 'with status and headers exposed (issue 2527)' do + it 'passes the HTTP status and headers into a custom error formatter' do + subject.format :txt + subject.error_formatter :txt, ->(error:, **) { "[#{error.status}] #{error.message} (#{error.headers['x-marker']})" } + subject.rescue_from :all do + error!('boom', 418, 'x-marker' => 'hit') + end + subject.get('/exception') { raise 'rain!' } + + get '/exception' + expect(last_response.status).to eq(418) + expect(last_response.body).to eq('[418] boom (hit)') + end + end + it 'rescues all errors and return :json' do subject.rescue_from :all subject.format :json diff --git a/spec/grape/dsl/request_response_spec.rb b/spec/grape/dsl/request_response_spec.rb index b1d5f8687..0ee449e20 100644 --- a/spec/grape/dsl/request_response_spec.rb +++ b/spec/grape/dsl/request_response_spec.rb @@ -199,37 +199,39 @@ end describe 'list of exceptions is passed' do + let(:default_rescue_options) { [Grape::DSL::RescueOptions.new] } + it 'sets hash of exceptions as rescue handlers' do subject.rescue_from StandardError expect(subject.inheritable_setting.namespace_reverse_stackable[:rescue_handlers]).to eq([{ StandardError => nil }]) - expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq([{}]) + expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options) end it 'rescues only base handlers if rescue_subclasses: false option is passed' do subject.rescue_from StandardError, rescue_subclasses: false expect(subject.inheritable_setting.namespace_reverse_stackable[:base_only_rescue_handlers]).to eq([{ StandardError => nil }]) - expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq([{ rescue_subclasses: false }]) + expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options) end it 'sets given proc as rescue handler for each key in hash' do rescue_handler_proc = proc {} subject.rescue_from StandardError, rescue_handler_proc expect(subject.inheritable_setting.namespace_reverse_stackable[:rescue_handlers]).to eq([{ StandardError => rescue_handler_proc }]) - expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq([{}]) + expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options) end it 'sets given block as rescue handler for each key in hash' do rescue_handler_proc = proc {} subject.rescue_from StandardError, &rescue_handler_proc expect(subject.inheritable_setting.namespace_reverse_stackable[:rescue_handlers]).to eq([{ StandardError => rescue_handler_proc }]) - expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq([{}]) + expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options) end it 'sets a rescue handler declared through :with option for each key in hash' do with_block = -> { 'hello' } subject.rescue_from StandardError, with: with_block expect(subject.inheritable_setting.namespace_reverse_stackable[:rescue_handlers]).to eq([{ StandardError => with_block }]) - expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq([{}]) + expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options) end end end diff --git a/spec/grape/dsl/routing_spec.rb b/spec/grape/dsl/routing_spec.rb index a173b8418..415a02779 100644 --- a/spec/grape/dsl/routing_spec.rb +++ b/spec/grape/dsl/routing_spec.rb @@ -25,7 +25,7 @@ class << self version = 'v1' expect(subject.version(version)).to eq(version) expect(subject.inheritable_setting.namespace_inheritable[:version]).to eq([version]) - expect(subject.inheritable_setting.namespace_inheritable[:version_options]).to eq(using: :path) + expect(subject.inheritable_setting.namespace_inheritable[:version_options]).to eq(Grape::DSL::VersionOptions.new) end end diff --git a/spec/grape/exceptions/invalid_accept_header_spec.rb b/spec/grape/exceptions/invalid_accept_header_spec.rb index a74ced5d4..9aeaedd00 100644 --- a/spec/grape/exceptions/invalid_accept_header_spec.rb +++ b/spec/grape/exceptions/invalid_accept_header_spec.rb @@ -42,7 +42,7 @@ subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: false subject.rescue_from :all do |e| error! 'message was processed', 400, e.headers end @@ -77,7 +77,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: false subject.get '/beer' do 'beer received' end @@ -112,7 +112,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: false subject.rescue_from :all do |e| error! 'message was processed', 400, e.headers end @@ -152,7 +152,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: false subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], [404, 'Resource not found'], [406, 'API vendor or version not found'], @@ -192,7 +192,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: true subject.rescue_from :all do |e| error! 'message was processed', 400, e.headers end @@ -236,7 +236,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: true subject.get '/beer' do 'beer received' end @@ -271,7 +271,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: true subject.rescue_from :all do |e| error! 'message was processed', 400, e.headers end @@ -320,7 +320,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: true subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], [404, 'Resource not found'], [406, 'API vendor or version not found'], diff --git a/spec/grape/middleware/exception_spec.rb b/spec/grape/middleware/exception_spec.rb index 6a34bfe4a..4c675a7e8 100644 --- a/spec/grape/middleware/exception_spec.rb +++ b/spec/grape/middleware/exception_spec.rb @@ -190,9 +190,7 @@ def call(_env) rescue_all: true, format: :custom, error_formatters: { - custom: lambda do |message, _backtrace, _options, _env, _original_exception| - { custom_formatter: message }.inspect - end + custom: ->(error:, **) { { custom_formatter: error.message }.inspect } } } end @@ -231,7 +229,7 @@ def call(_env) { rescue_all: true, format: :json, - rescue_options: { backtrace: true, original_exception: true } + rescue_options: Grape::DSL::RescueOptions.new(backtrace: true, original_exception: true) } end @@ -247,7 +245,7 @@ def call(_env) { rescue_all: true, format: :xml, - rescue_options: { backtrace: true, original_exception: true } + rescue_options: Grape::DSL::RescueOptions.new(backtrace: true, original_exception: true) } end @@ -263,7 +261,7 @@ def call(_env) { rescue_all: true, format: :txt, - rescue_options: { backtrace: true, original_exception: true } + rescue_options: Grape::DSL::RescueOptions.new(backtrace: true, original_exception: true) } end diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb index 9cf382a50..1e8df01fd 100644 --- a/spec/grape/middleware/formatter_spec.rb +++ b/spec/grape/middleware/formatter_spec.rb @@ -467,7 +467,6 @@ def self.call(_, _) it 'adds the backtrace and original_exception to the error output' do subject = described_class.new( app, - rescue_options: { backtrace: true, original_exception: true }, parsers: { json: ->(_object, _env) { raise StandardError, 'fail' } } ) io = StringIO.new('{invalid}') diff --git a/spec/grape/middleware/versioner/accept_version_header_spec.rb b/spec/grape/middleware/versioner/accept_version_header_spec.rb index 3c8624dd0..8bef08e8c 100644 --- a/spec/grape/middleware/versioner/accept_version_header_spec.rb +++ b/spec/grape/middleware/versioner/accept_version_header_spec.rb @@ -7,9 +7,7 @@ before do @options = { - version_options: { - using: :accept_version_header - } + version_options: Grape::DSL::VersionOptions.new(using: :accept_version_header) } end @@ -59,7 +57,7 @@ end it 'succeeds if :strict is set to false' do - @options[:version_options][:strict] = false + @options[:version_options] = @options[:version_options].with(strict: false) expect(subject.call('HTTP_ACCEPT_VERSION' => '').first).to eq(200) expect(subject.call({}).first).to eq(200) end @@ -67,7 +65,7 @@ context 'when :strict is set' do before do @options[:versions] = ['v1'] - @options[:version_options][:strict] = true + @options[:version_options] = @options[:version_options].with(strict: true) end it 'fails with 406 Not Acceptable if header is not set' do @@ -94,8 +92,7 @@ context 'when :strict and cascade: false' do before do @options[:versions] = ['v1'] - @options[:version_options][:strict] = true - @options[:version_options][:cascade] = false + @options[:version_options] = @options[:version_options].with(strict: true, cascade: false) end it 'fails with 406 Not Acceptable if header is not set' do diff --git a/spec/grape/middleware/versioner/header_spec.rb b/spec/grape/middleware/versioner/header_spec.rb index d673e08b9..2ed015625 100644 --- a/spec/grape/middleware/versioner/header_spec.rb +++ b/spec/grape/middleware/versioner/header_spec.rb @@ -7,10 +7,7 @@ before do @options = { - version_options: { - using: :header, - vendor: 'vendor' - } + version_options: Grape::DSL::VersionOptions.new(using: :header, vendor: 'vendor') } end @@ -156,13 +153,13 @@ end it 'succeeds if :strict is set to false' do - @options[:version_options][:strict] = false + @options[:version_options] = @options[:version_options].with(strict: false) expect(subject.call('HTTP_ACCEPT' => '').first).to eq(200) expect(subject.call({}).first).to eq(200) end it 'succeeds if :strict is set to false and given an invalid header' do - @options[:version_options][:strict] = false + @options[:version_options] = @options[:version_options].with(strict: false) expect(subject.call('HTTP_ACCEPT' => 'yaml').first).to eq(200) expect(subject.call({}).first).to eq(200) end @@ -170,7 +167,7 @@ context 'when :strict is set' do before do @options[:versions] = ['v1'] - @options[:version_options][:strict] = true + @options[:version_options] = @options[:version_options].with(strict: true) end it 'fails with 406 Not Acceptable if header is not set' do @@ -199,8 +196,7 @@ context 'when :strict and cascade: false' do before do @options[:versions] = ['v1'] - @options[:version_options][:strict] = true - @options[:version_options][:cascade] = false + @options[:version_options] = @options[:version_options].with(strict: true, cascade: false) end it 'fails with 406 Not Acceptable if header is not set' do diff --git a/spec/grape/middleware/versioner/param_spec.rb b/spec/grape/middleware/versioner/param_spec.rb index d772c610b..8f0fbab4b 100644 --- a/spec/grape/middleware/versioner/param_spec.rb +++ b/spec/grape/middleware/versioner/param_spec.rb @@ -24,7 +24,9 @@ end context 'with specified parameter name' do - let(:options) { { version_options: { parameter: 'v' } } } + let(:options) do + { version_options: Grape::DSL::VersionOptions.new(using: :param, parameter: 'v') } + end it 'sets the API version based on the custom parameter name' do env = Rack::MockRequest.env_for('/awesome', params: { 'v' => 'v1' }) @@ -55,7 +57,7 @@ let(:options) do { versions: ['v1'], - version_options: { using: :header } + version_options: Grape::DSL::VersionOptions.new(using: :header) } end diff --git a/spec/grape/path_spec.rb b/spec/grape/path_spec.rb index 9b9b2ed26..9b84756ba 100644 --- a/spec/grape/path_spec.rb +++ b/spec/grape/path_spec.rb @@ -65,7 +65,7 @@ context 'when path versioning is used' do it "includes a '/'" do - path = described_class.new(nil, nil, version: :v1, version_options: { using: :path }) + path = described_class.new(nil, nil, version: :v1, version_options: Grape::DSL::VersionOptions.new) expect(path.suffix).to eql('(/.:format)') end end @@ -77,12 +77,12 @@ end it "does not include a '/' when the path has a path" do - path = described_class.new('/path', nil, version: :v1, version_options: { using: :path }) + path = described_class.new('/path', nil, version: :v1, version_options: Grape::DSL::VersionOptions.new) expect(path.suffix).to eql('(.:format)') end it "includes a '/' otherwise" do - path = described_class.new(nil, nil, version: :v1, version_options: { using: :path }) + path = described_class.new(nil, nil, version: :v1, version_options: Grape::DSL::VersionOptions.new) expect(path.suffix).to eql('(/.:format)') end end diff --git a/spec/shared/versioning_examples.rb b/spec/shared/versioning_examples.rb index 60e622e44..f5c354064 100644 --- a/spec/shared/versioning_examples.rb +++ b/spec/shared/versioning_examples.rb @@ -3,7 +3,7 @@ shared_examples_for 'versioning' do it 'sets the API version' do subject.format :txt - subject.version 'v1', **macro_options + subject.version 'v1', **macro_options.except(:format) subject.get :hello do "Version: #{request.env[Grape::Env::API_VERSION]}" end @@ -14,7 +14,7 @@ it 'adds the prefix before the API version' do subject.format :txt subject.prefix 'api' - subject.version 'v1', **macro_options + subject.version 'v1', **macro_options.except(:format) subject.get :hello do "Version: #{request.env[Grape::Env::API_VERSION]}" end @@ -23,12 +23,12 @@ end it 'is able to specify version as a nesting' do - subject.version 'v2', **macro_options + subject.version 'v2', **macro_options.except(:format) subject.get '/awesome' do 'Radical' end - subject.version 'v1', **macro_options do + subject.version 'v1', **macro_options.except(:format) do get '/legacy' do 'Totally' end @@ -46,7 +46,7 @@ end it 'is able to specify multiple versions' do - subject.version 'v1', 'v2', **macro_options + subject.version 'v1', 'v2', **macro_options.except(:format) subject.get 'awesome' do 'I exist' end @@ -63,12 +63,12 @@ context 'without a prefix' do it 'allows the same endpoint to be implemented' do subject.format :txt - subject.version 'v2', **macro_options + subject.version 'v2', **macro_options.except(:format) subject.get 'version' do request.env[Grape::Env::API_VERSION] end - subject.version 'v1', **macro_options do + subject.version 'v1', **macro_options.except(:format) do get 'version' do "version #{request.env[Grape::Env::API_VERSION]}" end @@ -87,12 +87,12 @@ it 'allows the same endpoint to be implemented' do subject.format :txt subject.prefix 'api' - subject.version 'v2', **macro_options + subject.version 'v2', **macro_options.except(:format) subject.get 'version' do request.env[Grape::Env::API_VERSION] end - subject.version 'v1', **macro_options do + subject.version 'v1', **macro_options.except(:format) do get 'version' do "version #{request.env[Grape::Env::API_VERSION]}" end @@ -113,7 +113,7 @@ it 'calls before block that is defined within the version block' do subject.format :txt subject.prefix 'api' - subject.version 'v2', **macro_options do + subject.version 'v2', **macro_options.except(:format) do before do @output ||= 'v2-' end @@ -123,7 +123,7 @@ end end - subject.version 'v1', **macro_options do + subject.version 'v1', **macro_options.except(:format) do before do @output ||= 'v1-' end @@ -145,7 +145,7 @@ it 'does not overwrite version parameter with API version' do subject.format :txt - subject.version 'v1', **macro_options + subject.version 'v1', **macro_options.except(:format) subject.params { requires :version } subject.get :api_version_with_version_param do params[:version] @@ -158,7 +158,7 @@ let(:options) { macro_options } let(:v1) do klass = Class.new(Grape::API) - klass.version 'v1', **options + klass.version 'v1', **options.except(:format) klass.get 'version' do 'v1' end @@ -166,7 +166,7 @@ end let(:v2) do klass = Class.new(Grape::API) - klass.version 'v2', **options + klass.version 'v2', **options.except(:format) klass.get 'version' do 'v2' end