fix: allow reading body from non-2xx responses in net.request #21055

kwonoj · 2019-11-08T22:06:51Z

Description of Change

Context: https://cs.chromium.org/chromium/src/services/network/public/cpp/simple_url_loader.h?dr=C&g=0&l=262-276

SimpleURLLoader by default do not allow non-2xx http response as results, raises ERR_HTTP_RESPONSE_CODE_FAILURE and it makes attempt to read response body for those response not possible in net module's clientrequest.

This PR changes urlloader configuration to allow - this behavior matches to previous net module implementation, as well as browser native like fetch.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed net module request raises error when non-2xx response received.

nornagon

Thanks for the PR! I'd love to get a test for this before we merge.

nornagon · 2019-11-13T00:31:44Z

Hm, looks like the tests are consistently failing now. Perhaps this had some unintended consequences?

nornagon

RC until CI is green

kwonoj · 2019-11-16T00:58:26Z

Relevant test case failures are corrected, but each time rebase to master some of tests are failing inconsistently, not sure if it's actually related tests & how to fix.

deepak1556

LGTM, thanks! @nornagon can you review the test code style.

nornagon · 2019-11-25T20:58:22Z

spec-main/api-net-spec.ts

+            return
+          }
+
+          expect(requestResponseEventEmitted).to.equal(true)


Calling expect() from an event / callback will result in the entire test process being aborted, instead of just failing the test. Please refactor similarly to https://github.com/electron/electron/pull/21265/files#diff-c31378831f4597a16ed943c2f7060323R566

this is mostly copy-paste from existing test cases like https://github.com/electron/electron/blob/master/spec-main/api-net-spec.ts#L337 fyi

yep, these changed with #21265

- closes #21046

release-clerk · 2019-11-25T22:34:29Z

Release Notes Persisted

Fixed net module request raises error when non-2xx response received.

trop · 2019-11-25T22:34:40Z

I was unable to backport this PR to "7-1-x" cleanly;
you will need to perform this backport manually.

trop · 2019-11-25T22:34:43Z

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

* fix(urlrequest): allow non-2xx repsponse results - closes #21046 * test(net): add test cases to verify non-2xx body * test(session): update spec to match clientrequest behavior * test(net): update test cases to match clientrequest behavior * spec: clean up async net spec

trop · 2019-11-25T22:43:25Z

@MarshallOfSound has manually backported this PR to "8-x-y", please check out #21285

* fix(urlrequest): allow non-2xx repsponse results - closes #21046 * test(net): add test cases to verify non-2xx body * test(session): update spec to match clientrequest behavior * test(net): update test cases to match clientrequest behavior * spec: clean up async net spec

#21285) * fix(urlrequest): allow non-2xx repsponse results - closes #21046 * test(net): add test cases to verify non-2xx body * test(session): update spec to match clientrequest behavior * test(net): update test cases to match clientrequest behavior * spec: clean up async net spec

* fix(urlrequest): allow non-2xx repsponse results - closes #21046 * test(net): add test cases to verify non-2xx body * test(session): update spec to match clientrequest behavior * test(net): update test cases to match clientrequest behavior * spec: clean up async net spec

#21295) * fix: allow reading body from non-2xx responses in net.request (#21055) * fix(urlrequest): allow non-2xx repsponse results - closes #21046 * test(net): add test cases to verify non-2xx body * test(session): update spec to match clientrequest behavior * test(net): update test cases to match clientrequest behavior * spec: clean up async net spec * Update api-session-spec.js * chore: fixup test as per original PR to master

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 8, 2019

nornagon requested changes Nov 8, 2019

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 9, 2019

miniak requested a review from nornagon November 10, 2019 21:52

nornagon approved these changes Nov 13, 2019

View reviewed changes

nornagon changed the title ~~fix: allow non-2xx repsponse results~~ fix: allow reading body from non-2xx responses in net.request Nov 13, 2019

nornagon requested changes Nov 13, 2019

View reviewed changes

kwonoj mentioned this pull request Nov 15, 2019

net in electron7: impossible to read the body of an HTTP request that reponds with an error #21147

Closed

3 tasks

arantes555 mentioned this pull request Nov 18, 2019

Fix Electron usage sindresorhus/got#899

Closed

deepak1556 approved these changes Nov 25, 2019

View reviewed changes

MarshallOfSound added target/7-1-x labels Nov 25, 2019

nornagon requested changes Nov 25, 2019

View reviewed changes

kwonoj and others added 5 commits November 25, 2019 13:13

fix(urlrequest): allow non-2xx repsponse results

1ab779d

- closes #21046

test(net): add test cases to verify non-2xx body

63bde2b

test(session): update spec to match clientrequest behavior

97ed4cf

test(net): update test cases to match clientrequest behavior

6e4ff6e

spec: clean up async net spec

66f5c9f

nornagon approved these changes Nov 25, 2019

View reviewed changes

MarshallOfSound merged commit ca61d2f into electron:master Nov 25, 2019

trop bot added needs-manual-bp/7-1-x and removed target/7-1-x labels Nov 25, 2019

trop bot added needs-manual-bp/8-x-y and removed target/8-x-y labels Nov 25, 2019

MarshallOfSound mentioned this pull request Nov 25, 2019

fix: allow reading body from non-2xx responses in net.request (#21055) #21285

Merged

trop bot added in-flight/8-x-y and removed needs-manual-bp/8-x-y labels Nov 25, 2019

trop bot added merged/8-x-y and removed in-flight/8-x-y labels Nov 26, 2019

MarshallOfSound removed needs-manual-bp/7-1-x labels Nov 26, 2019

sofianguy added this to Fixed in 8.0.0-beta.4 in 8.2.x Dec 9, 2019

sofianguy added this to Fixed in 7.1.3 in 7.2.x Dec 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow reading body from non-2xx responses in net.request #21055

fix: allow reading body from non-2xx responses in net.request #21055

kwonoj commented Nov 8, 2019 •

edited

nornagon left a comment

nornagon commented Nov 13, 2019

nornagon left a comment

kwonoj commented Nov 16, 2019

deepak1556 left a comment

nornagon Nov 25, 2019

kwonoj Nov 25, 2019

nornagon Nov 25, 2019

release-clerk bot commented Nov 25, 2019

trop bot commented Nov 25, 2019

trop bot commented Nov 25, 2019

trop bot commented Nov 25, 2019

fix: allow reading body from non-2xx responses in net.request #21055

fix: allow reading body from non-2xx responses in net.request #21055

Conversation

kwonoj commented Nov 8, 2019 • edited

Description of Change

Checklist

Release Notes

nornagon left a comment

Choose a reason for hiding this comment

nornagon commented Nov 13, 2019

nornagon left a comment

Choose a reason for hiding this comment

kwonoj commented Nov 16, 2019

deepak1556 left a comment

Choose a reason for hiding this comment

nornagon Nov 25, 2019

Choose a reason for hiding this comment

kwonoj Nov 25, 2019

Choose a reason for hiding this comment

nornagon Nov 25, 2019

Choose a reason for hiding this comment

release-clerk bot commented Nov 25, 2019

trop bot commented Nov 25, 2019

trop bot commented Nov 25, 2019

trop bot commented Nov 25, 2019

kwonoj commented Nov 8, 2019 •

edited