Preserve explicitly located nested parameters on write methods - #149
Merged
Merged
Conversation
numbata
added a commit
that referenced
this pull request
Sep 10, 2026
extract_non_body_params only flattened bracket-notation nested params (e.g. "filter[kind]") into non-body parameters when the HTTP method was GET/HEAD/DELETE. On POST/PUT/PATCH, a nested Hash explicitly documented as query, header, or path silently vanished from the generated spec entirely: the bracket-flattening branch rejected it outright, and its Hash-typed root was separately dropped by the unconditional Hash-type skip in the same method. The HTTP-method gate was redundant for read methods in the first place: the branch already resolved the root's own location, and GET/HEAD/DELETE default unlocated params to "query" — so the gate never changed the outcome there. Replacing it with a direct check against the root's resolved location (skip only if it resolves to "body") makes every HTTP method behave the same way, and additionally fixes header/path overrides and childless Hash parameters, which dropped silently even on read methods before this change. Also removes ParamLocationResolver#explicit_non_body_param? and its one caller, a guard in NestedParamsBuilder#build that can never fire: nested_body_params only ever selects roots that already resolved to "body", so a root explicitly marked query/header/path is filtered out one step earlier and never reaches this guard. Closes #147.
numbata
force-pushed
the
fix/147-query-nested-write-methods
branch
from
September 10, 2026 22:24
2aad906 to
46c627a
Compare
Memory Profile ReportSummary
Iteration Details
Top Memory Allocations (by location)
Generated by Memory Profile workflow • Commit: a742313 |
Danger ReportNo issues found. |
extract_non_body_params's nested-param branch stamps a nested Hash root's resolved location straight onto the emitted Parameter. Since explicit_location returned whatever string the user typed in `param_type:`/`in:` verbatim, a typo (`in: "quer"`) or an unsupported grape-swagger location (`param_type: "formData"`, which this library never resolves parameters to) was emitted into the generated document unvalidated, producing an invalid OAS output instead of the previous silent drop. ParamLocationResolver now only honors param_type/in values from a known set (body, query, header, path, cookie); anything else is treated as unset and falls through to the default location, same as if it had never been specified. This also protects the flat-param path, which had the same unvalidated pass-through before this PR. Does not address a separate, pre-existing (unrelated to this PR) issue: an explicit `in: "path"` is honored even when the parameter name has no matching route capture, since path resolution never checks the location string against the route template. Reproduces identically for flat params on main; needs a route-template-aware fix, not a location allowlist. Closes #150.
This was referenced Sep 10, 2026
An explicit `in: "path"` / `param_type: "path"` was honored even when the name (or, for a nested Hash, its root) had no corresponding route capture, producing a path parameter that could never appear in the URL template — an invalid OAS document. `resolve` already returns "path" for actual route captures before reaching the explicit-location branch, so a "path" value reaching that branch is always a mismatch; treat it the same as an unrecognized location and fall through to the default instead of emitting it. Also fixes the docs/MIGRATING_FROM_GRAPE_SWAGGER.md parameter table, which listed `formData` as a supported `param_type` value alongside the other locations. grape-oas never places a parameter there directly (the OAS 2.0 exporter derives `formData` automatically from a form route's request body schema), and the location allowlist added earlier in this PR now rejects it if set explicitly, so the table was actively wrong either way.
extract_from_spec's path-capture check threaded name:/route_params: through the resolver just to re-check a condition resolve already guarantees: by the time extract_from_spec runs, route_params.include? (name) is always false, since resolve returns "path" for a genuine capture before ever calling it. Dropping the dead parameters and re-deriving "location = nil if location == 'path'" removes the tautology without changing behavior (verified: replacing the old conditional with the unconditional check produces an identical test suite run). Two more gaps found while re-verifying the path-capture fix: - A Hash root that happens to share its name with an actual route capture (e.g. `optional :filter, type: Hash do ... end` on `post "items/:filter"`) resolves to "path" for the wrong reason — the root name matches the URL template, but that doesn't make its bracket children path segments too. extract_non_body_params now skips nested children when the root resolves to "path", the same as it already does for "body". - `cookie` is a valid explicit location (OAS 3+), but Swagger 2.0 only allows query/header/path/formData/body. ParamLocationResolver resolves per-parameter with no visibility into the target OAS version, so this can't be caught there. The OAS 2.0 parameter exporter now drops cookie-located parameters and logs a warning, matching the existing "OAS2 cannot represent this" pattern used for complex form fields.
The param_type row for request parameters crammed several sentences into a table cell (300+ characters wide) and left formData listed as "supported" with no caveat. Move the explanation below the table and mark the row Partial, matching how the entity-options table already flags partial support. Also document that an explicit path is only honored when it matches a real route capture, and that cookie is OAS 3+ only.
test/e2e/explicit_query_nested_params_test.rb generated specs across all three OAS versions but never validated the result against a metaschema, unlike the sibling e2e tests in this repo (e.g. generate_oas2_complex_test.rb). This is exactly the kind of test that should catch a future "unknown in: value slips through" regression in ParamLocationResolver before it ships, so add the same OASValidator.validate! call the rest of the e2e suite already uses. Also corrects the param_type row in MIGRATING_FROM_GRAPE_SWAGGER.md: grape-oas supporting cookie (which grape-swagger, OAS 2.0-only, cannot) is a strict superset, not partial support - the "Partial" label read as a migration regression that isn't real.
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.
Problem
A parameter's location (path/query/body/header) is resolved per-parameter, but
RequestParams#extract_non_body_paramsonly flattened bracket-notation nested params (e.g.filter[kind]) into non-body parameters when the HTTP method was GET/HEAD/DELETE. On POST/PUT/PATCH, a nested Hash explicitly documented asquery,header, orpathsilently vanished from the generated spec entirely — it was rejected by the method-gated flattening branch, and its Hash-typed root was separately dropped by an unconditional Hash-type skip in the same method. Closes #147.Fix
The HTTP-method gate turns out to be redundant on the read side: the flattening branch already resolves the root's own location, and GET/HEAD/DELETE default unlocated params to
"query"— so the gate never changed the outcome there. Replacing it with a direct check against the root's resolved location (skip only when it resolves to"body") makes every HTTP method behave the same way, and as a side effect also fixes two adjacent gaps that predate this PR and reproduced on read methods too: an explicitin: "header"override on a nested root, and a childless Hash parameter (no nested children of its own) marked non-body.Also removes
ParamLocationResolver#explicit_non_body_param?and its one caller — a guard inNestedParamsBuilder#buildthat can never fire, sincenested_body_paramsonly ever selects roots that already resolved to"body"; a root explicitly marked query/header/path is filtered out one step earlier and never reaches that guard.Example
Schema before / after
OAS 2.0 emits the equivalent
in: queryparameter; OAS 3.1 emits the same parameter shape through its JSON Schema-compatible exporter. NorequestBody/bodyparameter is introduced in any version, sincefilternever resolves to"body".Backward compatibility