Add WithMinLength option to control when responses are gzipped #106
Conversation
|
@appleboy hi, can you please approve the workflows to run tests on this PR? |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #106 +/- ##
==========================================
- Coverage 81.35% 78.50% -2.86%
==========================================
Files 3 3
Lines 118 214 +96
==========================================
+ Hits 96 168 +72
- Misses 19 39 +20
- Partials 3 7 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new WithMinLength option to defer gzip compression until the response body reaches a configurable byte threshold. It updates the core handler and writer to buffer small responses and only apply gzip once the limit is met.
- Added
minLengthfield andWithMinLengthoption inoptions.go - Extended
gzipWriteringzip.gowith buffering and length-check logic - Updated
handler.goto conditionally flush buffered data when threshold isn’t reached - Added tests in
gzip_test.gocovering short, long, and multi-write responses - Expanded
README.mdwith usage examples and notes forWithMinLength
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| options.go | Added minLength config and WithMinLength Option |
| gzip.go | Buffers response data, checks length before gzip |
| handler.go | Defers final write/decompression decision |
| gzip_test.go | New tests for min-length-based compression logic |
| README.md | Documentation and example for WithMinLength |
Comments suppressed due to low confidence (1)
gzip.go:44
- [nitpick] The
Writemethod has nested compression and buffering logic that can be hard to follow. Refactoring length checks and buffer handling into helper methods would improve readability and testability.
func (g *gzipWriter) Write(data []byte) (int, error) {
gzip.go
Outdated
| g.Header().Del("Content-Length") | ||
| // If a Content-Length header is set, use that to decide whether to compress the response. | ||
| if g.Header().Get("Content-Length") != "" { | ||
| contentLen, _ := strconv.Atoi(g.Header().Get("Content-Length")) |
There was a problem hiding this comment.
Ignoring the strconv.Atoi error may treat invalid headers as length zero. Consider handling the parse error or validating the header value before using it.
| contentLen, _ := strconv.Atoi(g.Header().Get("Content-Length")) | |
| contentLenStr := g.Header().Get("Content-Length") | |
| contentLen, err := strconv.Atoi(contentLenStr) | |
| if err != nil { | |
| // If Content-Length is invalid, skip compression and write directly | |
| return g.ResponseWriter.Write(data) | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is intentionally ignored. Invalid headers will result in no compression being used since it's undefined behavior
|
@appleboy pushed an update to fix coverage and comments. Request approval to run PR workflows |
gzip.go
Outdated
| // minLength is the minimum length of the response body (in bytes) to enable compression | ||
| minLength int | ||
| // wasMinLengthMetForCompression indicates whether the minimum length for compression has been met | ||
| wasMinLengthMetForCompression bool |
There was a problem hiding this comment.
Is shouldCompress a more appropriate name?
gzip.go
Outdated
| // If a Content-Length header is set, use that to decide whether to compress the response. | ||
| if g.Header().Get("Content-Length") != "" { | ||
| contentLen, _ := strconv.Atoi(g.Header().Get("Content-Length")) // err intentionally ignored for invalid headers | ||
| if contentLen >= g.minLength { |
There was a problem hiding this comment.
Don't use the if-else pattern.
|
Also fix the lint error |
Enables conditional compression based on response size. The response is stored in a temporary buffer within the gzipWriter if it does not meet the provided limit (required in case the server executes additional writes to the response later in its handler control flow). When the handler finally returns, we check whether the limit was met. If not, the buffer contents are written directly to the underlying gin Writer with no gzip compression.
Credit to #20 for the main implementation details
Closes #18
Closes #20
Closes #34
Closes #81
Resolves part of #70