Fix wildcard path patterns in OpenAPI spec output for known routers #934
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical issue where router-specific wildcard path patterns in the internal API representation were being directly included in the OpenAPI specification output, which violates the OpenAPI spec and breaks tooling like Swagger UI, Restish, and TypeScript code generators.
Key Changes:
- Added
FixWildcardPathsfunction to normalize router-specific wildcard patterns to OpenAPI-compatible path parameters during JSON marshaling - Implemented support for wildcard patterns from ServeMux, Gorilla Mux, Gin, HttpRouter, BunRouter, Chi, Echo, and Fiber routers
- Integration is transparent - only affects the OpenAPI spec output via
MarshalJSON(), leaving internal path handling unchanged
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openapi.go | Adds regex-based path transformation logic with fixWildcardPath and FixWildcardPaths functions, integrated into MarshalJSON to normalize wildcards only in OpenAPI spec output |
| openapi_test.go | Comprehensive test coverage for all supported router wildcard patterns, verifying correct transformation and nil input handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Simplified regex matching and replacement. Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Should never happen as this would require multiple routers on purpose creating the collision with different syntaxes. Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Chi, Echo, Fiber: trailing /* or /+ -> /{path} | ||
| if strings.HasSuffix(path, "/*") { | ||
| return strings.TrimSuffix(path, "/*") + "/{path}" | ||
| } | ||
| if strings.HasSuffix(path, "/+") { | ||
| return strings.TrimSuffix(path, "/+") + "/{path}" | ||
| } |
There was a problem hiding this comment.
The hardcoded parameter name "path" used for Chi, Echo, and Fiber wildcards (lines 1712 and 1715) might not be semantically meaningful in all contexts. For example, a route like "/assets/*" would become "/assets/{path}", but "path" might not be the best descriptor for asset files. Consider documenting this naming choice or explaining why a generic name was chosen over attempting to infer context from the route structure.
There was a problem hiding this comment.
Needs concensus, path makes the most sense, because wildcards are meant to capture the rest of the path. Left open for opinions.
| // path parameter syntax for compatibility with OpenAPI tools and clients. | ||
| // | ||
| // The PathItem values are preserved (same pointer references), only the map keys | ||
| // are transformed. |
There was a problem hiding this comment.
The FixWildcardPaths function is exported (public API) primarily to enable testing from the external test package. However, this makes it part of the public API surface, which means it could be called by external users and needs to be maintained for backward compatibility. Consider either: (1) moving the test to an internal test file (package huma) where it can test the unexported version, or (2) if this function is intentionally part of the public API, ensure the documentation clearly indicates its intended use case and that users typically don't need to call it directly.
| // are transformed. | |
| // are transformed. | |
| // | |
| // This helper is primarily used internally by huma during OpenAPI generation | |
| // and by tests that validate path normalization behavior. Typical users of the | |
| // huma package do not need to call FixWildcardPaths directly; instead, it is | |
| // applied automatically when serializing the OpenAPI definition. |
There was a problem hiding this comment.
Needs direction, testing on a different package is weird. What would make more sense is to review implementation with the current test, and once validated remove it and un-export the function.
1 participant
I did not see any CONTRIBUTION guidelines, but here is first draft at a fix for wildcard path parameters that is ONLY applied to the OpenAPI spec output (MarshalJSON()), everything else is kept intact.
This has been discussed as an issue at:
The problem is that although wildcard patterns are supported by multiple routers, everyone does it differently, and they are not supported by the OpenAPI spec, which breaks the spec, the swagger ui, restish, typescript-codegen, etc...
The proposed solution is to normalize the wildcards only at the spec level as a simple path parameter. I found the testing weird because this is internal, but the tests are in a different package, so I had to export the function. Let me know how to proceed if this is an issue.
Any comments are appreciated and I will fix the code to match repo ergonomics and best practices.
Cheers!