Skip to content

Preserve path metadata inside nested body parameter groups - #146

Merged
numbata merged 9 commits into
mainfrom
fix/143-nested-path-metadata
Sep 10, 2026
Merged

numbata merged 9 commits into
mainfrom
fix/143-nested-path-metadata

Conversation

@numbata

@numbata numbata commented Sep 10, 2026

Copy link
Copy Markdown
Owner

Problem

A declared path parameter inherits param_type: "body" from an outer Grape parameter group and is skipped when the request has nested fields. The fallback then recreates it as a string, losing its description, type, and format. Closes #143.

Fix

Skip Hash parents while allowing the location resolver to classify scalar route captures ahead of inherited body metadata. Path parameters are always required, even when their Grape declaration is optional. Keep fallback generation for undeclared captures and keep declared captures out of the body schema.

Example

class API < Grape::API
  format :json
  params do
    with(documentation: { param_type: "body" }) do
      requires :id, type: Integer, desc: "The resource ID", documentation: { format: "int64" }
      requires :payload, type: Hash do
        requires :name, type: String
      end
    end
  end
  post(":id") { {} }
end

Schema before / after

OAS 2.0 path parameter excerpt:

# Before
- name: id
  in: path
  required: true
  type: string

# After
- name: id
  in: path
  required: true
  description: The resource ID
  type: integer
  format: int64

OAS 3.0 and 3.1 preserve the same description on the parameter and emit type: integer and format: int64 inside its schema. The nested payload remains in the request body.

Backward compatibility

No public API changes. Optional declared path captures now emit required: true, as required by OpenAPI. Hash-typed captures retain the existing string fallback. Generated clients may now use the declared path type instead of the fallback string. Undeclared captures retain the required string fallback. No new dependencies.

@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown

Memory Profile Report

Summary

Metric Value
Iterations 5
API Endpoints 18
Avg Allocated 1.79 MB
Avg Retained 888.00 B
Retained Trend 0.0%
Memory Stable Yes
Potential Leak No

Iteration Details

# Allocated Retained Objects
1 1.79 MB 888.00 B 25033
2 1.79 MB 888.00 B 25033
3 1.79 MB 888.00 B 25033
4 1.79 MB 888.00 B 25033
5 1.79 MB 888.00 B 25033

Top Memory Allocations (by location)

# Avg Memory Location
1 208.63 KB lib/grape_oas/doc_key_normalizer.rb:11
2 121.21 KB lib/grape_oas/api_model_builders/concerns/oas_utilities.rb:18
3 89.69 KB lib/grape_oas/type_resolvers/primitive_resolver.rb:44
4 82.03 KB lib/grape_oas/api_model/schema.rb:26
5 82.03 KB lib/grape_oas/api_model/schema.rb:32
6 62.03 KB lib/grape_oas/schema_constraints.rb:10
7 59.53 KB lib/grape_oas/exporter/concerns/schema_indexer.rb:63
8 51.80 KB lib/grape_oas/api_model/schema.rb:43
9 48.91 KB vendor/bundle/ruby/4.0.0/gems/securerandom-0.4.1/lib/securerandom.rb:71

Generated by Memory Profile workflow • Commit: 2bd2476

@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown

Danger Report

No issues found.

View run

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The behavior change is narrowly scoped, matches the stated issue/expected schema, and is covered by both unit and e2e regression tests.

Pull request overview

This PR fixes a request-parameter edge case in the API model builder where a declared route capture (path parameter) could be misclassified as part of a nested request body group and then recreated via the path-parameter fallback as a generic string, losing its declared description/type/format.

Changes:

  • Adjust RequestParams#extract_non_body_params to not skip route captures when nested params are present, allowing ParamLocationResolver to correctly classify them as "path" and preserve declared metadata.
  • Add a unit regression test around the nested-param builder output for a path capture inside an outer param_type: "body" group.
  • Add an end-to-end regression test across OAS 2.0 / 3.0 / 3.1 that validates both (a) preserved declared path metadata and (b) continued fallback generation for an undeclared capture; document the fix in CHANGELOG.md.

Review Checklist Findings (schema correctness, test coverage, boundaries, compatibility, changelog/PR body, code quality)

  • Nit (lib/grape_oas/api_model_builders/request_params.rb:99-101): local variable is_hash_param is actually derived from body_param? and the comment refers to “Hash type params”, which is misleading now that route-capture exceptions are involved.
File summaries
File Description
lib/grape_oas/api_model_builders/request_params.rb Prevents nested-param extraction from skipping declared route captures so path params keep their declared schema/description.
test/grape_oas/api_model_builders/request_params_nested_test.rb Adds a focused unit regression test verifying the API model builder preserves path capture metadata within an outer body group.
test/e2e/generate_body_params_test.rb Adds cross-OAS-version coverage ensuring declared path metadata is preserved while undeclared captures still fall back to required string.
CHANGELOG.md Records the user-visible bug fix under Unreleased.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/grape_oas/api_model_builders/request_params.rb Outdated
@cursor

cursor Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Automated review (3-reviewer panel)

Summary: The nested-params skip now lets route captures through so ParamLocationResolver.resolve can classify them as path instead of dropping them for ensure_path_parameters to recreate as required strings. That matches issue #143: Integer/int64/desc survive on the path parameter, the nested payload stays in the body, and undeclared captures still fall back to required string across OAS 2.0 / 3.0 / 3.1. The remaining risk is that body_param? also matches type: Hash, so the same hole can emit an object path parameter; the changelog line also sits in dated 1.5.1 while that gem is still untagged.

Medium

lib/grape_oas/api_model_builders/request_params.rb:100 — Hash-typed route captures become invalid OAS path parameters
body_param? is true for type: Hash / "Hash" as well as inherited param_type/in: "body". The old nested skip dropped every body_param?, so a capture whose name matches a Hash parent was omitted and ensure_path_parameters emitted a required string. After next if is_body_param && !route_params.include?(name), that capture is built as in: path with type: object. OAS path parameters cannot be objects; NestedParamsBuilder still excludes the same name from the body, so nested children disappear from the body too. Concrete input: requires :payload, type: Hash { requires :name, type: String } on post(":payload"). This is not the #143 case and is not called out as an emitted-shape change. The nested skip exists to omit Hash parents; documented-body scalars are already dropped later by next if location == "body", and resolve already prefers path over inherited body docs. Punching a path-name hole into body_param? keeps two classifiers in disagreement with build_flat_params.
Suggestion: Skip Hash types unconditionally and leave location to resolve:

next if [Hash, "Hash"].include?(spec[:type])
next if is_body_param && !route_params.include?(name)

Add a nested Hash-capture regression either way. If object path params are intended, lock that shape in a test instead of leaving it implicit.
(Flagged by: Bug Hunter, Architect, Guardian — unanimous)

lib/grape_oas/api_model_builders/request_params.rb:111 — Optional path captures can emit required: false
Declared captures now go through build_parameter(..., spec[:required] || false, ...). An optional :id inside the nested with body group used to be omitted here and reinserted by ensure_path_parameters as required: true. After this PR they keep required: false. OAS path parameters must be required; the 2.0 and 3.x exporters copy param.required through. Tests only use requires :id. The flat (non-nested) path already had this behavior; this change regresses the nested path toward that same invalid shape.
Suggestion: When location is "path", pass required: true. Add a unit case with optional :id plus a nested Hash inside with(documentation: { param_type: "body" }) and assert id.required.
(Flagged by: Guardian)

CHANGELOG.md:14 — Entry written into dated ## [1.5.1], not ## [Unreleased]
The #146 line sits in ## [1.5.1] - 2026-09-09 ### Fixed. Project rule is to add entries only under ## [Unreleased] and not edit released sections. main currently has no Unreleased heading because the 1.5.1 prepare commit already dated that section. 1.5.1 is not published yet (RubyGems and GitHub latest are 1.5.0; there is no v1.5.1 tag), and commit 62f18a3 explicitly puts this fix in that release train. If 1.5.1 is still open and this PR will merge before the tag, the placement is consistent. If 1.5.1 will be tagged from current main first, this line documents a fix that gem will not contain.
Suggestion: Confirm whether 1.5.1 is still open for this commit. If yes, keep the line and tag only after merge. If 1.5.1 should stay frozen, restore that block to match main, add ## [Unreleased] / ### Fixed, and move the #146 entry there. Do not bump VERSION.
(Flagged by: Bug Hunter, Architect, Guardian)

Low

lib/grape_oas/api_model_builders/request_params.rb:99 — Comment does not match the predicate
“Route captures take precedence over inherited body documentation” is a good why, but the condition also exempts Hash-typed captures with no body docs. If the Hash skip stays as-is, say so; if it narrows to Hash types, the old parent-skip comment is enough.
(Flagged by: Bug Hunter, Architect)

test/e2e/generate_body_params_test.rb:28 — E2E duplicates the unit fixture
The unit test already proves the builder contract (path location, description, integer + int64, id absent from body, payload kept). The e2e is the only coverage of exporter placement (OAS2 type/format on the parameter vs OAS3/3.1 schema) and of ensure_path_parameters still synthesizing the undeclared capture. Keep those exporter/fallback asserts if published JSON is the contract; drop the duplicated metadata checks, or move the method out of GenerateBodyParamsTest (that file is about flat POST fields not being dropped when a sibling Hash trips the nested path). Extra unit cases for GET/DELETE, documentation: { in: "body" }, and a sibling body scalar staying out of parameters would pin the skip more tightly than another e2e.
(Flagged by: Architect, Guardian)


4 files changed · 0 High · 3 Medium · 2 Low · Reviewed by: Bug Hunter, Architect, Guardian

Comment thread lib/grape_oas/api_model_builders/request_params.rb
Comment thread CHANGELOG.md Outdated
Comment thread test/e2e/generate_body_params_test.rb
Comment thread lib/grape_oas/api_model_builders/request_params.rb Outdated
Comment thread lib/grape_oas/api_model_builders/request_params.rb Outdated
@numbata
numbata force-pushed the fix/143-nested-path-metadata branch from 4177389 to 6f612fe Compare September 10, 2026 21:27
Merging #145 removed the only caller of ParamLocationResolver.body_param?
(and its sole dependent, body_annotation?), leaving both unreferenced.
Rename the surviving Hash-type check to hash_param? and call it directly
from request_params.rb, so the Hash-type literal has one home instead of
being duplicated across both files.

Closes #148.
build_parameter already forces required: true for any "path" location,
but no test asserted this for a flat (non-nested) route where the Grape
declaration itself is optional. Without coverage, a future change to
build_parameter could silently reintroduce invalid specs where an
optional Grape path capture is documented as an optional OAS parameter.
Two entries for one PR (one under Fixed, one under Changed) duplicated
attribution for the same change; fold the required-flag correction into
the existing path-metadata entry, and keep it under the still-unreleased
1.5.1 section alongside #145.
@numbata

numbata commented Sep 10, 2026

Copy link
Copy Markdown
Owner Author

Addressed review follow-ups:

Full suite green (1634 runs, 0 failures) and RuboCop clean.

@numbata
numbata merged commit bcf811c into main Sep 10, 2026
17 checks passed
@numbata
numbata deleted the fix/143-nested-path-metadata branch September 10, 2026 22:12
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.

Nested body params cause declared path parameters to lose metadata

2 participants