Repository navigation
Add helpers to instrument existing chi router - #243
Merged
Merged
Conversation
Lines Of Code
|
Go API Changes# github.com/swaggest/rest/chirouter ## compatible changes NewOpenAPICollector: added OpenAPICollector: added OpenAPIPreparer: added # github.com/swaggest/rest/gorillamux ## compatible changes (*OpenAPICollector).Describe: added # github.com/swaggest/rest/gzip ## compatible changes JSONContainer.GzipETag: added # github.com/swaggest/rest/nethttp ## compatible changes CollectRouteOperation: added Describe: added Preparer: added RoutePreparer: added RoutePreparerFunc: added # summary Inferred base version: v0.2.75 Suggested version: v0.3.0 |
Benchmark ResultBenchmark diff with base branchBenchmark result |
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
LlamaPReview — Blocking issues found
Shared func pointers silently attach another route's documentation
Exact-head CI remains unresolved (3 failed); no CI-dependent merge-safety claim is made.
Owner action: State the constraint in the exported docs of RouteDocs and Describe (each documented handler must be a distinct func value; repeated Describe for one key is last-write-wins), or make the registration key unique/detectable, and add a test with two routes whose handlers come from the same literal.
1 further item in details.
Change flow
Registration-time identity keying decides which OpenAPI setup each walked route receives — pointer collisions are the risk point.
sequenceDiagram
participant App as Application
participant Col as OpenAPICollector
participant Map as RouteDocs map
participant Walk as chi.Walk / mux.Walk
participant Prep as RouteDocs.Preparer
participant Doc as OpenAPI document
App->>Col: Describe(h, setup) at registration
Col->>Map: byHandler[ValueOf(h).Pointer()] = setup
note over Col,Map: PR change — identity key is the func code pointer, last write wins
App->>Walk: Walk(router, c.Walker)
Walk->>Prep: Preparer(handler, OperationExtractor)
alt Implements SetupOpenAPIOperation
Prep->>Doc: Use interface setup
else HandlerFunc pointer present in map
Prep->>Doc: Use stored setup for that pointer
else No preparer and no annotation
Prep->>Doc: Emit best-effort Incomplete operation
end
note over Map,Doc: Impact — colliding func values silently attach another route's documentation
Review details and evidence
| Priority | File | Finding | Evidence |
|---|---|---|---|
| P2 | nethttp/openapi_router.go |
Shared func pointers silently attach another route's documentation | confirmed |
| P2 | .github/workflows/golangci-lint.yml |
Lint gate lowered to oldstable and modernize disabled alongside feature | needs verification |
Finding details
P2 · Lint gate lowered to oldstable and modernize disabled alongside feature
.github/workflows/golangci-lint.yml
The PR lowers the lint workflow from go-version: stable to oldstable and adds modernize to the disabled linters (with action/linter version bumps and a Makefile comment update). Consequence: the lint gate no longer runs on the newest Go release and no longer surfaces modernization findings; if either change was needed to make the new code pass, the findings are suppressed rather than fixed. No product-behavior risk, and the golangci-lint check passes at head — worth splitting out or justifying, not worth blocking.
Owner action: Split the lint-policy loosening into its own PR or justify why oldstable and disabling modernize are needed for this change.
Verification boundary: needs verification; scope: changed region.
LlamaPReview checks
- Reviewed changed regions in
nethttp/openapi_router.go. - Reviewed changed regions in
chirouter/collector.go. - Reviewed changed regions in
gorillamux/collector.go. - Reviewed changed regions in
chirouter/example_openapi_collector_test.go. - Read bounded PR-head context from
nethttp/openapi_router.go. - Reviewed changed regions in
.github/workflows/golangci-lint.yml.
LlamaPReview is an open-source pull request reviewer. Tell the maintainers where this review got it wrong.
Unit Test Coveragetotal: (statements) 81.3% Coverage of changed lines
Coverage diff with base branchMissing base coverage file |
Examples Benchmark ResultBenchmark diff with base branch |
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the critical gzip ETag representation and Accept-Encoding caching issue.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds reusable OpenAPI instrumentation helpers for existing Chi and Gorilla/Mux routers, alongside gzip ETag, dependency, and tooling updates.
Changes:
- Adds shared route description and collection helpers.
- Adds Chi and Gorilla/Mux integrations and examples.
- Updates gzip ETag handling and expectations.
- Updates dependencies, linting, CI, and generated fixtures.
- Critical: gzip and identity responses may share strong ETags without
Vary: Accept-Encoding.
File summaries
| File | Summary |
|---|---|
README.md |
Documents router integrations. |
nethttp/openapi_router.go |
Adds shared OpenAPI helpers. |
Makefile |
Updates lint tooling. |
gzip/container.go |
Changes gzip ETag hashing. |
gzip/container_test.go |
Updates ETag tests. |
gorillamux/example_openapi_collector_test.go |
Adds Gorilla/Mux usage example. |
gorillamux/collector.go |
Uses shared route helpers. |
go.sum |
Updates dependency checksums. |
go.mod |
Updates dependencies. |
chirouter/example_openapi_collector_test.go |
Adds Chi usage example. |
chirouter/collector.go |
Adds Chi route collection. |
.golangci.yml |
Updates lint configuration. |
.github/workflows/golangci-lint.yml |
Updates lint CI configuration. |
_examples/go.sum |
Updates example checksums. |
_examples/go.mod |
Updates example dependencies. |
_examples/advanced/gzip_pass_through_test.go |
Updates gzip expectations. |
_examples/advanced-generic-openapi31/gzip_pass_through_test.go |
Updates gzip expectations. |
_examples/advanced-generic-openapi31/_testdata/openapi.json |
Updates generated OpenAPI fixture. |
Review details
- Files reviewed: 16/18 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.
There was a problem hiding this comment.
🟡 Changes recommended
Gzip ETag handling needs correction for cache variation, exact compressed bytes, application validators, and actual compression state.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
gzip/container.go:106
GzipETagis not a validator for the gzip representation: it hashes the uncompressed JSON plus a fixed salt, while the bytes served byWriteCompressedBytesare the gzip output. A change in flate settings or Go's gzip implementation can therefore leave this strong ETag unchanged while changing the response bytes. Compute the validator from the exact compressed bytes (or mark it weak/content-equivalence and adjust conditional-cache semantics) instead of salting the identity digest.
// GzipETag returns a hash of uncompressed JSON content salted for the gzip-compressed
// representation, so it differs from ETag.
//
// The gzip and identity representations are byte-distinct, so gzip-compression middleware
// (see response/gzip) uses this instead of ETag when serving the gzip body, to avoid one
// strong validator being shared between two different representations.
gzip/container.go:180
- This replaces any pre-existing ETag whenever the response writer supports direct gzip output, even when that ETag was deliberately supplied by the application rather than generated from
JSONContainer.ETag(). That silently changes the resource's validator only for gzip-capable requests. Restrict the swap to the identity ETag generated for this container (or otherwise preserve application-provided validators).
if rw, ok := w.(http.ResponseWriter); ok {
if _, ok := w.(Writer); ok && rw.Header().Get("Etag") != "" {
rw.Header().Set("Etag", jc.gzipHash)
}
gzip/container.go:179
JSONWriteToselectsgzipHashsolely from the writer implementingWriter, butgzipResponseWriterimplementsWritereven whenwriteHeaderlater disables compression for a non-compressibleContent-Type(for exampletext/csvorapplication/octet-stream). In that case the body is identity-encoded while it carries the gzip representation's ETag. Select the validator only after the middleware has decided the actual representation, or otherwise gate this swap on the same compression decision.
if rw, ok := w.(http.ResponseWriter); ok {
if _, ok := w.(Writer); ok && rw.Header().Get("Etag") != "" {
rw.Header().Set("Etag", jc.gzipHash)
- Files reviewed: 16/18 changed files
- Comments generated: 1
- Review effort level: Lite
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.
No description provided.