Fix double wrap on json errors

[MattHall]

When an error is presented via an entity, ErrorFormatter::Base#present returns:

presenter.represent(message, embeds).serializable_hash

Grape::Entity#serializable_hash does not return a plain Hash - it returns a Grape::Entity::Exposure::NestingExposure::OutputBuilder, which is a SimpleDelegator wrapping a Hash (not a Hash subclass).

3.3.0 refactored ErrorFormatter::Json#wrap_message from an is_a? check to case/when:

3.2.1

return message if message.is_a?(Hash)   # delegated to the wrapped Hash → true → returned unwrapped

3.3.0

case message
when Hash then message                  # Hash === message → C-level real-class check → false
else { error: ensure_utf8(message) }    # OutputBuilder falls here → wrapped
end

obj.is_a?(Hash) is an ordinary method call, so SimpleDelegator forwards it to the wrapped Hash (true). But case/when Hash matches via Module#===, a C-level ancestry check that ignores delegation (false) — so the delegator falls through to the else branch and gets wrapped.

Fix

Restore is_a? based matching in ErrorFormatter::Json#wrap_message, bringing it back in line with the base class and the pre-3.3.0 behaviour. A comment documents the Module#=== vs is_a? delegation pitfall so it isn't refactored back.

[github-actions]

Danger Report

No issues found.

View run

[ericproulx]

The fix is correct and the regression is real — happy to see this restored. One correction on the explanation, though, since the new code comment is meant to prevent a future refactor and currently documents the wrong cause.

is_a? is forwarded by the delegator to the wrapped Hash, so it matches.

A plain SimpleDelegator does not forward is_a?/kind_of? to the wrapped object:

SimpleDelegator.new({}).is_a?(Hash)   # => false   (not forwarded)

If forwarding were the mechanism, this would be true. The reason OutputBuilder.is_a?(Hash) returns true is that Grape::Entity's OutputBuilder explicitly overrides the check (grape_entity/.../output_builder.rb):

def kind_of?(klass)
  klass == output.class || super   # Hash == @output_hash.class => Hash == Hash => true
end
alias is_a? kind_of?

So it matches because of the class's own kind_of?/is_a? override (a real-class == comparison it opts into), not because of generic delegation.

The case/when half of the comment is spot-on: Hash === obj is Module#===, a C-level ancestry check that bypasses the Ruby-level kind_of? override entirely — verified that it calls the override for is_a? but never for ===:

ob.is_a?(Hash)  => true    (override called)
Hash === ob     => false   (override ignored)

Suggested tweak to the comment so it doesn't get "corrected" later by someone who tests a bare SimpleDelegator:

# Use +is_a?+ rather than +case/when+ here. +case/when Hash+ matches via
# +Module#===+, a C-level real-class check that ignores any Ruby-level override.
# +Grape::Entity#serializable_hash+ returns an +OutputBuilder+ (a +SimpleDelegator+)
# that defines its own +kind_of?+/+is_a?+ returning true for the wrapped Hash's class,
# so +is_a?+ matches while +case/when Hash+ would fall through and wrap the payload in
# a spurious +{ error: ... }+ envelope.

Mechanism aside, the patch itself is good to merge.

[ericproulx]

@MattHall thank you, TIL :)

[schinery]

@ericproulx would it be possible to get this fix released as 3.3.1?

[ericproulx]

@schinery of course. If you don't mind I'm gonna wait a little more in case other issues are created. Let's say by the end of the week, I'm gonna release 3.3.1 with that fix.

[schinery]

@schinery of course. If you don't mind I'm gonna wait a little more in case other issues are created. Let's say by the end of the week, I'm gonna release 3.3.1 with that fix.

Magic, thanks @ericproulx 👍🏻