Skip to content

Make ErrorFormatter.formatter_for a lookup and move the fallback to its caller - #2902

Merged
ericproulx merged 1 commit into
error-formatter-nil-guardfrom
error-formatter-registry-lookup
Sep 5, 2026
Merged

Make ErrorFormatter.formatter_for a lookup and move the fallback to its caller#2902
ericproulx merged 1 commit into
error-formatter-nil-guardfrom
error-formatter-registry-lookup

Conversation

@ericproulx

@ericproulx ericproulx commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Stacked on #2901 — review that one first; this PR's base is error-formatter-nil-guard and I'll retarget it to master once #2901 merges.

ErrorFormatter.formatter_for took the API's default_error_formatter as a third argument and ended in default_error_formatter || Grape::ErrorFormatter::Txt, so the global registry implemented the caller's precedence out of the caller's own state. It was the only registry in the family that did:

module signature miss returns
Parser.parser_for (format, parsers = nil) nil — caller decides
ParamsBuilder.params_builder_for (short_name) raises
Formatter.formatter_for (api_format, formatters) DEFAULT_LAMBDA_FORMATTER (its own constant)
ErrorFormatter.formatter_for (format, error_formatters, default_error_formatter) default_error_formatter || Txt

error_formatters is at least a table it can search; default_error_formatter is API policy threaded through a global module. The || Txt alone would be fine — Formatter does the same — but it cannot stay once the other leaves: a caller can only insert its own default between "nothing registered" and Txt if the lookup reports the miss.

So formatter_for takes the Parser.parser_for shape, and Middleware::Error applies the fallback it owns:

# error_formatter.rb
def formatter_for(format, error_formatters = nil)
  return error_formatters[format] if error_formatters&.key?(format)

  registry[format]
end

# middleware/error.rb — Options#initialize, next to the existing rescue_options ||=
default_error_formatter ||= Grape::ErrorFormatter::Txt

# format_message
formatter = Grape::ErrorFormatter.formatter_for(current_format, error_formatters) || default_error_formatter

Options#initialize is the right home for the default because Endpoint#error_middleware_options passes an explicit nil when the API called no format, which bypasses the Data keyword default — the same reason rescue_options ||= is already there.

Resolution is unchanged, precedence included: a formatter registered for the requested format still wins over the API's default_error_formatter, which applies to formats with none of their own (format == nil included — that is what makes the default_error_formatter :json spec pass). Both directions are now pinned by specs in error_spec.rb, and a new spec/grape/error_formatter_spec.rb pins the lookup contract, mirroring parser_spec.rb.

Reporting the miss also lets the DSL see it

default_error_formatter names an error formatter, so a name nothing is registered under is a mistake. It used to store the Txt the lookup substituted, so the typo read back as a working setting and errors quietly rendered as text:

default_error_formatter :jsonn
# => Grape::Exceptions::UnknownErrorFormatter: unknown error formatter: jsonn

format names a format, where having no error formatter of its own is ordinary (format :xls), so it still succeeds and stores nil, with the middleware supplying Txt at render time.

Both are in UPGRADING, along with the signature change: a third argument to formatter_for now raises ArgumentError, and the setting reads back nil instead of Txt for a format with no error formatter — visible only if you read it back, since error responses are unchanged.

🤖 Generated with Claude Code

@ericproulx
ericproulx force-pushed the error-formatter-registry-lookup branch from ffe373c to 72a75de Compare September 4, 2026 21:47
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Danger Report

No issues found.

View run

@ericproulx
ericproulx force-pushed the error-formatter-registry-lookup branch from 72a75de to 71aa77d Compare September 4, 2026 21:52
@ericproulx
ericproulx force-pushed the error-formatter-registry-lookup branch from 71aa77d to c9da551 Compare September 4, 2026 22:09
…ts caller

`formatter_for` took the API's `default_error_formatter` as a third argument
and ended in `default_error_formatter || Grape::ErrorFormatter::Txt`, so the
global registry implemented the caller's precedence with the caller's own
state. It was the only registry that did: `Parser.parser_for` answers nil and
lets the caller decide, `Formatter.formatter_for` falls back to a constant of
its own, and `ParamsBuilder.params_builder_for` raises.

Give it the `Parser.parser_for` shape — API registrations, then the registry,
then nil — and let `Middleware::Error` apply the fallback it owns, defaulting
`default_error_formatter` to `ErrorFormatter::Txt` in its `Options` alongside
`rescue_options` (both arrive as an explicit nil from
`Endpoint#error_middleware_options`, which bypasses the keyword defaults).

Resolution is unchanged, precedence included: a formatter registered for the
requested format still wins over the API's `default_error_formatter`, which
applies to formats that have none of their own.

Reporting the miss also lets the DSL see it. `default_error_formatter` names an
error formatter, so a name nothing is registered under now raises
`Grape::Exceptions::UnknownErrorFormatter` rather than storing the `Txt` the
lookup used to substitute, which read back as a working setting. `format` names
a format, where having no error formatter of its own is ordinary, so it still
succeeds and stores nil, with the middleware supplying `Txt` at render time.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ericproulx
ericproulx force-pushed the error-formatter-registry-lookup branch from c9da551 to 49a3a0d Compare September 5, 2026 09:29
@ericproulx
ericproulx merged commit 17bf345 into master Sep 5, 2026
69 checks passed
@ericproulx
ericproulx deleted the error-formatter-registry-lookup branch September 5, 2026 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant