Fix: Preserve HTTP Response in URL Errors #3369
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
… errors - Move newResponse call earlier to avoid code duplication - Properly handle nil responses with var declaration - Return response object with URL errors to preserve HTTP information - Add test case using client.Do to verify behavior
devin-ai-integration
bot
force-pushed
the
devin/1733470068-preserve-response-in-url-errors
branch
from
a4f97fc to
d341400
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3369 +/- ##
==========================================
- Coverage 97.72% 92.30% -5.42%
==========================================
Files 153 176 +23
Lines 13390 15031 +1641
==========================================
+ Hits 13085 13874 +789
- Misses 215 1064 +849
- Partials 90 93 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you, @silasalberti !
This looks like a great start. I think the unit test can be improved, however.
Additionally, on line 854 I think it is also a safe location to return the new response instead of nil.
github/github_test.go
Outdated
| @@ -1106,6 +1106,27 @@ func TestDo_redirectLoop(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestDo_preservesResponseInURLError(t *testing.T) { | |||
There was a problem hiding this comment.
I'm not convinced that this test actually creates a url.Error.
For examples, please see: https://cs.opensource.google/go/go/+/refs/tags/go1.23.4:src/net/url/url_test.go;l=1763-1821
There was a problem hiding this comment.
That's a good point. I guess the original issue was about a 404 error. Renamed the test to HTTPError and made the mock more in line with the other mocks in the file. Also, now it's returning response on line 854 too. Does that look better?
There was a problem hiding this comment.
Ideally, tests would cover each of the return paths through BareDo but I'm thinking that this may be much more effort to achieve... so I think I'm fine with this level of testing.
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
|
Thank you, @tomfeigin ! |
Fix: Preserve HTTP Response in URL Errors
Fixes #3362
This PR improves error handling in the BareDo method to properly preserve HTTP response information when encountering *url.Error. Changes include:
The changes allow users to access HTTP status codes (like 404) even when encountering URL errors.
This PR was created with help from Devin: https://preview.devin.ai/sessions/1b2f7ce6e3b44942b3ac1f518eac7c22