-
Notifications
You must be signed in to change notification settings - Fork 679
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
Handle EOF from multipart form file readers #438
Conversation
Bump @jeevatkm |
@NathanBaulch Thank you for your PR appreciated, I'm sorry for the delayed response. Travis CI build link was broken. I have migrated travis-ci.org to travis-ci.com Can you please edit the PR to trigger the CI build? |
Codecov Report
@@ Coverage Diff @@
## master #438 +/- ##
==========================================
+ Coverage 96.66% 96.81% +0.15%
==========================================
Files 10 10
Lines 1320 1320
==========================================
+ Hits 1276 1278 +2
+ Misses 25 24 -1
+ Partials 19 18 -1
Continue to review full report at Codecov.
|
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.
@NathanBaulch It seems the test case is not covered for this PR update, can you please take a look at it?
https://app.codecov.io/gh/go-resty/resty/compare/438/diff#diff-dXRpbC5nbw==
The test case doesn't increase coverage because my fix is all about avoiding a false error. There is no new code path that the test could hit. The test fails without the fix, passes with the fix and doesn't impact coverage, as expected. Hopefully that makes sense. |
@NathanBaulch Yes, it makes sense. |
Not really relevant to my fix but sure, I've added a test that borrows |
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.
@NathanBaulch Thanks for your PR.
I stumbled on a bug when trying to send a multipart file that happened to be a gzip reader downloaded from elsewhere. Looks like the multipart content type detection logic doesn't handle
EOF
correctly. This can also be easily reproduced by sending an empty file (bytes.NewReader(nil)
).Here's a self contained example that reproduces the issue with a gzipped string:
Closes #431