-
Notifications
You must be signed in to change notification settings - Fork 8.2k
fix render: write content length in Data.Render #4206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Hi, @appleboy Could you check this pull request? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4206 +/- ##
==========================================
- Coverage 99.21% 99.00% -0.22%
==========================================
Files 42 44 +2
Lines 3182 3405 +223
==========================================
+ Hits 3157 3371 +214
- Misses 17 24 +7
- Partials 8 10 +2
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:
|
I will take it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue with the Content-Length header in Data.Render to ensure that it is set correctly for both large and empty data responses.
- Updated Data.Render to explicitly set the Content-Length header using strconv.Itoa(len(r.Data)).
- Added tests in render_test.go to verify the header for various data sizes, including zero-length.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
render/render_test.go | Added tests to validate the Content-Length header for different data sizes. |
render/data.go | Modified Render to set the Content-Length header based on the data length. |
if len(r.Data) > 0 { | ||
w.Header().Set("Content-Length", strconv.Itoa(len(r.Data))) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition 'if len(r.Data) > 0' prevents setting the Content-Length header when the data length is 0, causing test failures for zero-length responses. Consider removing the condition so the header is always set to strconv.Itoa(len(r.Data)).
if len(r.Data) > 0 { | |
w.Header().Set("Content-Length", strconv.Itoa(len(r.Data))) | |
} | |
w.Header().Set("Content-Length", strconv.Itoa(len(r.Data))) |
Copilot uses AI. Check for mistakes.
This PR fixes an issue where the
Content-Length
header is not set correctly for large data written by ResponseWriter.Write. According to the Go documentation, theContent-Length
header is automatically added only for small data sizes. For larger data, it is necessary to explicitly set theContent-Length
header to ensure proper behavior.