Skip to content

Remove request-time mutable state from Array and custom-type coercers#2817

Open
ericproulx wants to merge 1 commit into
masterfrom
fix-coercer-request-time-state
Open

Remove request-time mutable state from Array and custom-type coercers#2817
ericproulx wants to merge 1 commit into
masterfrom
fix-coercer-request-time-state

Conversation

@ericproulx

Copy link
Copy Markdown
Contributor

Summary

Two coercers in the types layer created or mutated state at request time on instances that are shared across requests — coercers are built once at definition time and cached in Types::CoercerCache. Found during the same validation-subsystem audit as #2816. Neither is a hard crash on MRI, but both violate the layer's "no request-time mutable state" contract and one produces a user-visible wrong result.

1. ArrayCoercer — lazy element coercer built at call time

def elem_coercer
  @elem_coercer ||= DryTypeCoercer.coercer_instance_for(subtype, strict:)
end

coerce_elements calls this per element, so the first request to reach a shared coercer instance writes a new ivar, and that first call can also seed DryTypes::ParamsCache — a Grape::Util::Cache backed by an unsynchronized default-block Hash. Same shared-mutable-default-hash shape flagged elsewhere in the codebase. SetCoercer subclasses ArrayCoercer and inherits the lazy path.

Fix: build the element coercer eagerly in initialize (renaming the now-redundant @subtype ivar to @elem_coercer). SetCoercer gets the eager build for free via super.

2. CustomTypeCoercermap! on the user's returned collection

def collection_symbolizer(method)
  ->(val) { method.call(val).map! { |item| symbolize_if_hash(item) } }
end

method is the user's coerce_with (or a custom type's parse). Calling map! on its return value mutates an object Grape doesn't own. If that value is frozen, map! raises FrozenError, which CoerceValidator catches under its blanket rescue StandardError and reports as a spurious "<param> is invalid" 400 — even though coercion succeeded.

Fix: dup before map!. dup returns an unfrozen shallow copy, so the user's object is never mutated, and — unlike switching to map — it preserves the class, keeping a Set a Set (Set#map returns an Array). The symbolized hashes are fresh (deep_symbolize_keys), so nothing shared is touched.

Note: the first draft of this fix used map instead of dup.map! and was caught by the existing CustomTypeCoercer Set spec — Set#map silently downgrades the result to an Array. The regression spec added here locks that in.

Tests

  • array_coercer_spec.rb: a frozen coercer instance still coerces — a deterministic proof that #call creates no lazy ivar (a frozen instance with a lazy ||= would raise FrozenError).
  • custom_type_coercer_spec.rb: a coercion method returning a frozen array is symbolized without raising; the existing Set spec continues to assert the Set return type is preserved.

No behavioral change for the ordinary (unfrozen, non-shared-contention) paths. Full suite: 2369 examples, 0 failures. RuboCop clean.

🤖 Generated with Claude Code

Two coercers created or mutated state at request time on instances that
are shared across requests (and cached in Types::CoercerCache):

- ArrayCoercer memoized @elem_coercer with ||= inside #call, so the
  first request to hit a shared instance wrote a new ivar (and could
  seed the unsynchronized DryTypes cache). Build it eagerly in
  initialize instead; rename the now-unused @subtype accordingly.
  SetCoercer inherits the eager build via super.

- CustomTypeCoercer#collection_symbolizer called map! on whatever the
  user's coerce_with method returned. A frozen return raised FrozenError,
  which CoerceValidator swallowed as a spurious "is invalid" 400; the
  input could also be the user's own object. dup before map! avoids the
  mutation and, unlike map, keeps a Set a Set.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ericproulx
ericproulx force-pushed the fix-coercer-request-time-state branch from f9c273f to a8b4b85 Compare July 25, 2026 18:50
@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Danger Report

No issues found.

View run

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