Refactor swagger documentation modules into GrapeSwagger namespace with routing fixes#980
Merged
numbata merged 36 commits intoJun 26, 2026
Conversation
…atibility table in README
Master's commit 978 added a variant_types parameter to fulfill_params; update the test to pass an empty hash for that argument.
The CI rows for grape 2.1.3 and 2.2.0 were dropped when the minimum was raised to 2.4.0, but 2.1.x and 2.2.x users should not be broken by this release. Revert the gemspec floor to >= 2.1 and restore the CI entries so those combinations continue to be verified. Update the README compatibility table to match.
SwaggerRouting and SwaggerDocumentationAdder have been namespaced under GrapeSwagger::, but downstream code that references the old top-level constants by name would get a NameError without any migration path. Assign the old constant names as aliases pointing to the new namespaced modules and document the change in UPGRADING.md so consumers know to migrate to GrapeSwagger::SwaggerRouting / GrapeSwagger::SwaggerDocumentationAdder.
build_path_params walks the StackableValues chain from innermost to outermost and was calling merge! at each step, which let the outer (less specific) scope overwrite the inner one for params with the same name. Reverse the merge so the accumulated inner result takes precedence over each successive outer level, and add a test that asserts the innermost :id definition wins when two namespace levels both declare options for it.
…rays
- Escape route.prefix before interpolating it into the split regex.
A prefix like 'v1.0' made the dot a wildcard, silently mis-bucketing
routes; a prefix with '(' raised RegexpError and crashed doc generation.
- Hoist standalone_namespaces computation above the each_key loop in
combine_namespace_routes. The result never changes between iterations,
so recomputing it O(N) times was wasted work.
- Replace Enumerator.new yielder blocks with plain array literals in
route_namespace_equals? and route_path_start_with?. The two forms are
semantically identical for a fixed two-element set; arrays are clearer.
- Regexp.escape doc_klass.mount_path in combine_routes, mirroring the
same fix already applied to route.prefix on the line above. A mount
path like '/api/v1.0/docs' would otherwise treat the dot as a wildcard,
causing the hide_documentation_path guard to suppress unintended routes
or raise RegexpError on paths containing '(' or '+'.
- Replace app.endpoints.clone with .dup in combine_namespaces. clone
preserves the frozen state of the original array, so if Grape ever
freezes its endpoint list the subsequent shift would raise FrozenError.
dup always returns a mutable copy, which is the intended semantics here.
When the same path param is declared at multiple namespace levels, a plain Hash#merge meant the inner declaration silently dropped every key from the outer one, even keys the inner didn't set. Replace the flat merge with a recursive deep merge so inner values win on conflict while outer-only metadata (e.g. description or example set at the parent level) is preserved. Add regression cases: one for partial override (inner type wins, outer description preserved) and one for nested documentation hash merging.
…ion tests
Replace the triple-negation reject block with a plain select using dig,
which directly expresses the intent: keep only namespaces that opt out
of nesting via swagger: { nested: false }.
Add spec/lib/swagger_routing_spec.rb covering the two Regexp.escape
fixes: route.prefix and mount_path. Uses '+' and '(' as metacharacters
since those cause a RegexpError rather than silent mismatches, making
the test fail clearly if escaping is ever removed.
…ated Use Ruby's built-in deprecate_constant so callers get a warning when they reference the old top-level names, instead of silently inheriting aliases that would otherwise become permanent. Document planned removal in 3.0 in UPGRADING.md and add a CHANGELOG entry.
- swagger_routing.rb: tighten the standalone_namespaces parent check from
name.start_with?(ns_name) to name == ns_name || name.start_with?("#{ns_name}/")
so sibling namespaces with a shared prefix (e.g. 'user' and 'users') are
not mistaken for a parent-child pair. Add a regression spec.
- grape-swagger.rb: guard the top-level compat aliases with unless defined?
to avoid 'already initialized constant' noise in reload-ish environments.
- route.rb: replace gsub(':', '') with delete_prefix(':') to express the
intent precisely — only a leading colon should be stripped.
- route_spec.rb: rename misleading context title from "namespace options use
string keys" to "route.params contains only symbol-keyed params", which
accurately reflects the fixture.
sub(/\/{2,}/, '/') only collapses the first run of consecutive slashes.
A mount_path + namespace combination that produces multiple doubled-slash
runs (e.g. //foo//bar) would produce /foo//bar after sub, leaving the
second run intact and generating a malformed namespace key.
gsub collapses every run in one pass.
- swagger_routing_spec.rb: add a test proving that multiple double-slash runs in a joined mount path are all collapsed (covers the gsub fix). - swagger_routing.rb: add a one-line comment on route_path_start_with? explaining that Regexp.escape is intentionally absent because String#start_with? is a literal prefix check, not a regex match. - swagger_documentation_adder.rb: add require_relative 'swagger_routing' to make the load dependency explicit rather than relying on grape-swagger.rb always loading it first.
…n specs
The unless defined?(SwaggerRouting) guard only checked one of the two
top-level aliases, leaving SwaggerDocumentationAdder potentially undefined
if a downstream gem had already defined SwaggerRouting. Removing the guard
ensures both aliases are always assigned together unconditionally. Normal
require semantics already prevent double-loading.
Adds three regression specs to route_spec.rb:
- Explicit ':account_id' -> :account_id symbol-key contract, pinning the
delete_prefix + to_sym chain end-to-end
- Multi-leading-colon edge case ('::id') confirming delete_prefix strips
exactly one colon
- Two-level-deep nested documentation hash merge, proving merge_path_param_options
recurses past the first frame
Danger ReportNo issues found. |
Contributor
Author
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
2 participants
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
This PR builds on top of #976 by @moskvin, who deserves primary credit for the work here. The core refactor — extracting
SwaggerRoutingandSwaggerDocumentationAdderout of the top-level namespace intoGrapeSwagger::, the sibling-namespace prefix boundary fix, the regex injection hardening, and the path-param deep-merge rewrite — was all done by @moskvin. This PR carries that work forward with additional spec coverage, edge-case fixes, and cleanup that emerged from review.Specific additions on top of #976: remove the conditional guard that could leave
SwaggerDocumentationAdderunaliased ifSwaggerRoutingwas pre-defined elsewhere; fix empty-string prefix generating//-prefixed route patterns; add regression specs pinning thedelete_prefixcolon-stripping contract, the symbol-key lookup chain, two-level-deep hash merge, and intentional array-replace behavior inmerge_path_param_options; remove redundant inline comments that narrated obvious code.Changes
SwaggerRoutingandSwaggerDocumentationAdderextracted tolib/grape-swagger/swagger_routing.rbandlib/grape-swagger/swagger_documentation_adder.rbunder theGrapeSwagger::namespace; top-level constants kept as deprecated aliases for backward compatibilityRegexp.escapeapplied toroute.prefixanddoc_klass.mount_pathincombine_routesto close regex injectionstart_with?(ns_name)replaced withname == ns_name || name.start_with?("#{ns_name}/")sousersis no longer a phantom child ofuserstandalone_namespaceshoisted out of the per-namespace loop (was O(n) recomputation)route_path_start_with?to avoid//namepatternsgsub(/\/{2,}/, '/')replacessubso all repeated slash runs infull_namespaceare normalized, not just the firstrequest_param_parsers/route.rbrewritten to deep-merge inherited namespace options and normalize keys to symbolsunless defined?guard)Specrenamed toIssue537Spec/Issue579Specto avoid cross-file constant leaksRelated
Builds on #976 by @moskvin.