[refiner] style(middleware): fix gzip doc comments and use otelkeys.Error (#3116)#3119
Open
amalgamated-bot wants to merge 5 commits into
Open
[refiner] style(middleware): fix gzip doc comments and use otelkeys.Error (#3116)#3119amalgamated-bot wants to merge 5 commits into
amalgamated-bot wants to merge 5 commits into
Conversation
Add GzipMiddleware to the global middleware chain, transparently compressing responses for JSON, XML, ATOM (OPDS), and other text-based content types when the client sends Accept-Encoding: gzip. Binary responses (images, epub/mobi/pdf), SSE streams, and clients that do not advertise gzip support are passed through uncompressed. Key implementation details: - Compression is decided lazily at the first Write/WriteHeader/Flush call, so the handler's Content-Type header is visible at decision time. - gzip.Writer instances are reused via sync.Pool to avoid per-request allocation overhead. - gzip.BestSpeed (level 1) is used to minimise CPU cost while still achieving ~60-80% size reduction for typical JSON payloads. - Unwrap() is implemented so http.ResponseController.SetWriteDeadline (used by the SSE handler) reaches the underlying ResponseWriter. - Vary: Accept-Encoding is set on every response (with or without gzip) so intermediate caches store separate entries per encoding. Energy efficiency impact (Network & I/O focus area): Typical book list (50 books): ~35 KB JSON → ~5 KB gzipped (~85% less) OPDS feed: ~20 KB XML → ~3 KB gzipped (~85% less) Network transfer energy is proportional to bytes transmitted across all hops (client NIC, switches, server NIC, DRAM buffers), so reducing payload size directly reduces energy consumption per request. GSF principle: Demand Shaping — less data transferred reduces load on network infrastructure. No new dependencies: uses only compress/gzip from the Go standard library.
- Remove dead `decided` field (sync.Once already provides idempotency) - Replace strings.Contains with proper acceptsGzip parser that handles q=0 - Use Header().Add for Vary instead of Set to preserve existing values - Log gz.Close() errors at debug level instead of silently discarding - Reset gzip.Writer to nil before returning to pool to avoid retaining refs - Skip compression for Range requests to preserve Content-Range semantics - Assert write errors in tests instead of discarding them - Add tests for acceptsGzip and Range request handling
- Move acceptsGzip before GzipMiddleware so each function has its own doc comment; previously the GzipMiddleware comment block bled into acceptsGzip leaving GzipMiddleware undocumented - Replace raw string key "error" with otelkeys.Error constant in slog.DebugContext call per project logging conventions
There was a problem hiding this comment.
Pull request overview
This PR introduces and wires up a gzip response-compression middleware, along with unit tests, and aligns middleware logging field keys with the otelkeys constants.
Changes:
- Register
middleware.GzipMiddlewarein the server’s global middleware chain. - Add
internal/handlers/middleware/gzip.goimplementing lazy gzip compression for selected text-based content types. - Add
internal/handlers/middleware/gzip_test.gocovering compression and bypass scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/server/server.go | Adds gzip middleware to the global alice chain. |
| internal/handlers/middleware/gzip.go | Implements gzip compression middleware and response-writer wrapper (lazy decision, pooling, vary header, range bypass). |
| internal/handlers/middleware/gzip_test.go | Adds tests for compression behavior and bypass conditions. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…xisting Content-Encoding - Fix acceptsGzip to correctly reject q=0.0, q=0.00, q=0.000 as zero quality per RFC 7231 §5.3.1 - Add support for the "*" wildcard token in Accept-Encoding per RFC 7231 §5.3.4 - Add guard in decide() to skip compression when Content-Encoding is already set by the handler, preventing double-encoding - Update tests to match corrected RFC semantics and add coverage for new cases
Rewrite acceptsGzip to do a full pass over all tokens before returning, so that an explicit gzip;q=0 rejection always takes precedence over a wildcard (*) match regardless of token order. Per RFC 7231 §5.3.4.
Comment on lines
+130
to
+144
| for _, token := range strings.Split(header, ",") { | ||
| token = strings.TrimSpace(token) | ||
| lower := strings.ToLower(token) | ||
|
|
||
| if strings.HasPrefix(lower, "gzip") { | ||
| if hasZeroQuality(lower) { | ||
| gzipRejected = true | ||
| } else { | ||
| gzipExplicit = true | ||
| } | ||
| } else if strings.HasPrefix(lower, "*") { | ||
| if !hasZeroQuality(lower) { | ||
| wildcardMatch = true | ||
| } | ||
| } |
Comment on lines
+162
to
+167
| // Trim trailing whitespace/parameters. | ||
| if end := strings.IndexAny(val, " \t;,"); end >= 0 { | ||
| val = val[:end] | ||
| } | ||
| // Must be "0" optionally followed by a dot and up to three zeros. | ||
| if val == "0" { |
Comment on lines
+33
to
+37
| gr, err := gzip.NewReader(rec.Body) | ||
| require.NoError(t, err) | ||
| got, err := io.ReadAll(gr) | ||
| require.NoError(t, err) | ||
| require.Equal(t, body, string(got)) |
Comment on lines
+125
to
+129
| gr, err := gzip.NewReader(rec.Body) | ||
| require.NoError(t, err) | ||
| got, err := io.ReadAll(gr) | ||
| require.NoError(t, err) | ||
| require.Equal(t, body, string(got)) |
Comment on lines
+148
to
+152
| gr, err := gzip.NewReader(rec.Body) | ||
| require.NoError(t, err) | ||
| got, err := io.ReadAll(gr) | ||
| require.NoError(t, err) | ||
| require.Equal(t, `{"id":"123"}`, string(got)) |
Comment on lines
220
to
226
| chain := alice.New( | ||
| middleware.RequestIDHandler, | ||
| otel.TraceMiddleware, | ||
| middleware.LoggingMiddleware, | ||
| middleware.NewSecurityHeadersMiddleware(middleware.SecurityHeadersConfig{SecureCookies: s.secureCookies}), | ||
| middleware.GzipMiddleware, | ||
| ).Then(s.mux) |
Comment on lines
+182
to
+190
| func TestAcceptsGzip(t *testing.T) { | ||
| tests := []struct { | ||
| header string | ||
| want bool | ||
| }{ | ||
| {"gzip", true}, | ||
| {"gzip, deflate, br", true}, | ||
| {"br, gzip", true}, | ||
| {"deflate", false}, |
This was referenced May 30, 2026
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.
Refinement Summary
This PR contains automated code style improvements for PR #3116 (feat(middleware): add gzip HTTP response compression).
Changes Made
Style Alignment
Fixed doc comment ordering in
internal/handlers/middleware/gzip.go: TheGzipMiddlewaredoc comment block was attached to theacceptsGzipfunction because no blank line separated them, leavingGzipMiddlewareitself undocumented. Fixed by movingacceptsGzip(with its own doc comment) before theGzipMiddlewaredoc comment + function, so each declaration is correctly documented per Go conventions.Use
otelkeys.Errorconstant: Replaced the raw string key"error"withotelkeys.Errorin theslog.DebugContextcall insideGzipMiddleware. Per project conventions, all structured log field keys must use the predefined constants frominternal/otelkeys/logger_keys.go.Security Analysis
Test Coverage
shouldGzipandacceptsGzipFiles Modified
internal/handlers/middleware/gzip.goGenerated by refiner workflow | Targets PR #3116
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
localhostproxy.golang.orgSee Network Configuration for more information.
Add this agentic workflows to your repo
To install this agentic workflow, run
Greptile Summary
This automated refinement PR introduces two style fixes to the new
gzip.gomiddleware: it reordersacceptsGzipbefore theGzipMiddlewarefunction so each declaration's doc comment attaches to the correct symbol, and replaces the raw"error"log key with theotelkeys.Errorconstant per project conventions.server.gois updated to wireGzipMiddlewareinto the alice middleware chain.gzip.go):acceptsGzipis moved aboveGzipMiddlewareso Go tooling (godoc, LSP) associates each comment block with the right function.otelkeys.Erroradoption (gzip.goline 205):slog.Any("error", err)becomesslog.Any(otelkeys.Error, err), aligning with the project's structured-log key constants.server.go):middleware.GzipMiddlewareis appended to the alice chain as the innermost wrapper, so compression applies after tracing, logging, and security-header decoration.Confidence Score: 5/5
Safe to merge — the two changes are purely stylistic (doc comment reordering and otelkeys constant adoption) with no behavioral impact on the middleware logic.
Both modifications are mechanical: moving a function declaration so its doc comment attaches to the right symbol, and swapping a raw string literal for a named constant. Neither touches the compression decision path, pool lifecycle, header manipulation, or request routing. The slog.DebugContext call correctly carries r.Context() per project convention.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming Request] --> B[Add Vary: Accept-Encoding] B --> C{Range header present?} C -- Yes --> D[Pass through uncompressed] C -- No --> E{acceptsGzip?} E -- No --> D E -- Yes --> F[Wrap w with gzipResponseWriter] F --> G[next.ServeHTTP] G --> H{decide - on first Write/WriteHeader/Flush} H --> I{Content-Encoding already set?} I -- Yes --> J[Pass through: skip compression] I -- No --> K{shouldGzip Content-Type?} K -- No --> J K -- Yes --> L[Get gzip.Writer from pool] L --> M[Set Content-Encoding: gzip, Del Content-Length] M --> N[Write compressed body] N --> O{defer: gz.Close} O -- error --> P[slog.DebugContext close error] O -- ok/after log --> Q[gz.Reset nil + return to pool]Reviews (3): Last reviewed commit: "fix(middleware): explicit gzip;q=0 overr..." | Re-trigger Greptile