Document the definition-time rejection of uncoercible collection types - #2837
Open
ericproulx wants to merge 1 commit into
Open
Document the definition-time rejection of uncoercible collection types#2837ericproulx wants to merge 1 commit into
ericproulx wants to merge 1 commit into
Conversation
#2817 made ArrayCoercer build its element coercer eagerly in the constructor rather than memoizing it on first use, so that coercers -- shared across requests -- create no state at request time. A side effect went unrecorded: an Array/Set whose element type Grape cannot coerce is now rejected while the params block is evaluated, so an application that boots today can fail to boot on 4.0 with ArgumentError: type Foo should support coercion via `[]` Previously the element coercer was built on the first request that supplied the parameter, so a declaration never exercised by a request -- a documentation-only type, which is what this breaks in practice -- never raised at all; one that was exercised got the same ArgumentError swallowed by the coercion validator into a generic 400 "is invalid". The tightening is right: the non-collection form (type: Foo) has always raised at definition time, and the collection form's leniency was an accident of the lazy build rather than a supported behavior. But it is a load-time break, so it needs an UPGRADING entry, which this adds -- with the three fixes that actually work (a one-argument parse, coerce_with, or dropping type: in favour of documentation:), and a warning that defining self.[] only silences the load-time error while leaving the parameter failing per request. Also pin the behavior: nothing asserted it anywhere, so a future refactor could revert it silently. The three "raises" examples fail against the pre-#2817 lazy build and pass against the eager one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Danger ReportNo issues found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#2817 has an unrecorded breaking side effect. Declaring
type: Array[Foo]/type: Set[Foo]where Grape cannot coerceFoonow raises while the API class is being loaded:An application that boots today can fail to boot on 4.0, without any request being made. This adds the UPGRADING entry and pins the behavior with specs.
Found while investigating grape-swagger's suite failing against grape
master— 12 of its failures are this, from fixtures declaringtype: Array[Entities::ApiError]on anOpenStructsubclass used purely for documentation.What actually changed
Nothing about which types are supported — only when the element coercer is built. #2817 made
ArrayCoercerbuild it eagerly in the constructor instead of memoizing it on first use, so that coercers (shared across requests, and cached inCoercerCache) create no state at request time. Building it eagerly means an unsupported element type is discovered at definition.Before, it was built on the first request that supplied the parameter. So a declaration never exercised by a request never raised at all; one that was exercised got the same
ArgumentErrorswallowed by the coercion validator into a generic400 {"error":"foo is invalid"}.This is a correct tightening
type: FooArgumentErrorwhen definedtype: Array[Foo]400 "is invalid"per requestArgumentErrorwhen definedThe non-collection form has always raised at definition time. The collection form's leniency was an accident of the lazy build, not a supported behavior — and it wasn't a working feature but a landmine, since the parameter could never actually be coerced. The two are now consistent. I'm documenting it rather than reverting it.
Verified across 3.2.0 and 3.3.4 (both accept) and bisected on
mastertoe209dbe7(#2817) as the first commit that rejects.The UPGRADING entry
Covers what raises, why, the before/after table, and the three fixes that actually work — each verified end-to-end, not just at load:
self.parse(routes toCustomTypeCollectionCoercer,200)coerce_with:supplying the whole collection (no element coercer is built,200)type:fordocumentation: { type: Foo }when the class only describes the parameter — the case behind this breakIt also warns that defining
self.[]silences the load-time error without fixing anything: that's the escape hatch fordry-typesobjects, and a plain class taking it still fails400 "is invalid"on every request. I nearly listedcoerce_withas a non-fix for the same reason before testing it properly — the distinction is easy to get wrong, hence the explicit note.Specs
Nothing asserted this anywhere —
should support coercion viaappeared in no spec — so a future refactor could revert it as silently as #2829 reverted #2824. Five examples added under::build_coercer:Array/Set/params-block rejection, plus theparseandcoerce_withacceptances.The three "raises" examples fail against the pre-#2817 lazy build and pass against the eager one — verified in both directions.
Notes
bundle exec rubocopandbundle exec rspec(2521 examples) pass.🤖 Generated with Claude Code