sanitize param names for http.ServeMux#2279
sanitize param names for http.ServeMux#2279mromaszewicz wants to merge 1 commit intooapi-codegen:mainfrom
Conversation
e8cbb61 to
cb9c19d
Compare
Go's net/http ServeMux requires wildcard segment names to be valid Go
identifiers. OpenAPI specs can use path parameter names containing
dashes (e.g. "addressing-identifier"), which causes a panic when
registering routes with ServeMux.
Fix by sanitizing parameter names in the stdhttp code path:
- SwaggerUriToStdHttpUri now sanitizes param names via SanitizeGoIdentity
so route patterns use valid Go identifiers (e.g. {addressing_identifier})
- stdhttp middleware template uses new SanitizedParamName for r.PathValue()
calls to match the sanitized route pattern, while keeping the original
ParamName for error messages
- Add SanitizedParamName() method to ParameterDefinition for use by
templates that need the sanitized form
Add server-specific test directory with per-router integration tests
exercising dashed path parameter names. Right now, only stdhttp has
a test in this directory, but we'll do router specific tests in there
in the future.
Fixes oapi-codegen#2278
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cb9c19d to
730c94f
Compare
Greptile SummaryThis PR fixes a runtime panic caused by OpenAPI path parameters with dashes (e.g. Key changes:
The core fix is correct and well-structured. Two things worth attention:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| pkg/codegen/utils.go | Introduces SanitizeGoIdentifier (no keyword-prefixing) extracted from SanitizeGoIdentity, and updates SwaggerUriToStdHttpUri to sanitize wildcard names via a double-regex pattern; logic is correct but an edge-case name collision risk exists. |
| pkg/codegen/operations.go | Adds SanitizedParamName() method to ParameterDefinition, correctly delegating to SanitizeGoIdentifier; straightforward and safe. |
| pkg/codegen/templates/stdhttp/std-http-middleware.tmpl | Updates the three path-param extraction paths (PassThrough, Json, Styled) to call r.PathValue with .SanitizedParamName while keeping .ParamName in error messages; correct. |
| pkg/codegen/utils_test.go | New assertions cover the dashed-param sanitization and the Go-keyword pass-through case for SwaggerUriToStdHttpUri; SanitizeGoIdentifier itself has no direct unit tests. |
| internal/test/server-specific/stdhttp/server_test.go | Integration test exercises the dashed-param scenario end-to-end; correctly asserts HTTP 200 and proper param extraction. |
Last reviewed commit: 730c94f
| return pathParamRE.ReplaceAllStringFunc(uri, func(match string) string { | ||
| sub := pathParamRE.FindStringSubmatch(match) | ||
| return "{" + SanitizeGoIdentifier(sub[1]) + "}" | ||
| }) |
There was a problem hiding this comment.
Sanitization collision: two distinct parameter names can map to the same wildcard
SanitizeGoIdentifier replaces every illegal rune with _, so two different OpenAPI parameter names can produce the same sanitized output. For example, my-param and my_param both produce my_param.
If a spec defines both in the same path (or if two separate parameters accidentally collide), the route registered with ServeMux would contain a duplicate wildcard segment, e.g.:
GET /resources/{my_param}/{my_param}
Go's net/http ServeMux panics when it encounters a pattern with two wildcards that have the same name (see Go source). This would cause a runtime panic at server startup rather than a compile-time error.
Consider detecting this collision during code generation (e.g. in SwaggerUriToStdHttpUri or the calling site) and returning an error, or at minimum documenting the limitation.
There was a problem hiding this comment.
I think I am ok with the possibility to rename something into a new conflict in this very limited case. This is at such a lower level than our name conflict resolution code, that combining the two would be incredibly messy.
There was a problem hiding this comment.
No, PR #2220 handles a different scenario. It addresses intentional duplicates in the spec (like /clients/{client-uuid}/roles/{role-name}/composites/clients/{client-uuid} where client-uuid genuinely appears twice), deduplicating them at the ParameterDefinition struct level.
The collision risk here is about accidental duplicates created by sanitization: two originally distinct parameter names (e.g., {my-param} and {my_param}) both becoming {my_param} in the route pattern string that ServeMux sees, causing a panic when registering the route.
Since #2220's deduplication operates on parameter definitions (before URI transformation), it wouldn't prevent ServeMux from seeing duplicate wildcards in the pattern string after sanitization.
Adding a test that documents "we don't support specs where sanitization creates collisions" would be valuable - it makes the limitation explicit and prevents confusion if someone hits this edge case. Something like:
// Test that specs with parameters that collide after sanitization are not supported
// (e.g., {my-param} and {my_param} both become {my_param})This also helps future maintainers understand the behavior is intentional, not an overlooked bug.
Go's net/http ServeMux requires wildcard segment names to be valid Go identifiers. OpenAPI specs can use path parameter names containing dashes (e.g. "addressing-identifier"), which causes a panic when registering routes with ServeMux.
Fix by sanitizing parameter names in the stdhttp code path:
Add server-specific test directory with per-router integration tests exercising dashed path parameter names. Right now, only stdhttp has a test in this directory, but we'll do router specific tests in there in the future.
Fixes #2278