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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #106 +/- ##
==========================================
- Coverage 81.35% 80.58% -0.78%
==========================================
Files 3 3
Lines 118 206 +88
==========================================
+ Hits 96 166 +70
- Misses 19 31 +12
- Partials 3 9 +6
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) {
|
@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 |
|
@appleboy Hi, request your review on this PR to add length-based gzipping for gin? |
takanuva15
force-pushed
the
master
branch
3 times, most recently
from
5d6022e to
acff587
Compare
|
@appleboy Hi, I fixed merge conflicts. Request your approval on this PR to add length-based gzipping for gin? |
|
I will take it. |
|
@takanuva15 Please help resolve the conflicts from #47 issue. See the PR #124 |
fixed conflicts |
3 participants
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