fix(api-stream): Add throwOnError support in undici.stream

[assalielmehdi]

#1753

[mdashlw]

This doesn't handle body properly, see differences between api-request.js and api-stream.js in onData and onComplete. Whatever stream is doing requires .write() and .end(), but new Readable requires .push() and .push(null)

await undici.request('https://api.github.com/notfound', {
  throwOnError: true
})

ResponseStatusCodeError: Response status code 403: Forbidden {
  code: 'UND_ERR_RESPONSE_STATUS_CODE',
  body: '\r\n' +
    'Request forbidden by administrative rules. Please make sure your request has a User-Agent header (https://docs.github.com/en/rest/overview/resources-in-the-rest-api#user-agent-required). Check https://developer.github.com for other possible causes.\r\n',
  status: 403,
  statusCode: 403,
  headers: {
    'cache-control': 'no-cache',
    'content-type': 'text/html; charset=utf-8',
    'strict-transport-security': 'max-age=31536000',
    'x-content-type-options': 'nosniff',
    'x-frame-options': 'deny',
    'x-xss-protection': '0',
    'content-security-policy': "default-src 'none'; style-src 'unsafe-inline'",
    connection: 'close'
  }
}

await undici.stream(
  "https://api.github.com/notfound",
  { throwOnError: true },
  () => fs.createWriteStream("response.txt")
);

ResponseStatusCodeError: Response status code 403: Forbidden {
  code: 'UND_ERR_RESPONSE_STATUS_CODE',
  body: undefined,
  status: 403,
  statusCode: 403,
  headers: {
    'cache-control': 'no-cache',
    'content-type': 'text/html; charset=utf-8',
    'strict-transport-security': 'max-age=31536000',
    'x-content-type-options': 'nosniff',
    'x-frame-options': 'deny',
    'x-xss-protection': '0',
    'content-security-policy': "default-src 'none'; style-src 'unsafe-inline'",
    connection: 'close'
  }
}

[assalielmehdi]

Hi @mdashlw, thank you for your comment. I think that it's pretty normal for the body to be undefined in the case of undici.stream() since the HTTP body is written to the writable stream created by the factory. However, I had to make sure that the writable in finished before throwing an error. We can retrieve the body from a PassThrough opaque if needed (see unit test).

[assalielmehdi]

Hi @mcollina, thank you 🙏🏼. In docs, options is already of type RequestOptions that extends DispatchOptions, and throwOnError is already explained in DispatchOptions. I see nothing more to add in docs. WDYT?

[mdashlw]

https://github.com/nodejs/undici/pull/1767/commits/8f7a6d79ff2d43af6997770a710e0086057e9ba3 kind of re-introduces the entire issue. This will create response.txt with the actual response and still throw ResponseStatusCodeError.

await undici.stream(
  "https://api.github.com",
  { throwOnError: true },
  () => fs.createWriteStream("response.txt")
);

ResponseStatusCodeError: Response status code 403: Forbidden {
  code: 'UND_ERR_RESPONSE_STATUS_CODE',
  body: undefined
  ...
}

response.txt
Request forbidden by administrative rules. Please make sure your request has a User-Agent header (https://docs.github.com/en/rest/overview/resources-in-the-rest-api#user-agent-required). Check https://developer.github.com for other possible causes.

IMO the most intuitive and convenient way would be to throw an error with special body handling, as in .request(), and not to call the factory.

[assalielmehdi]

Since in undici.request(), the body is consumed before throwing the error, I figured we want the same from undici.stream(). I fixed the code to throw an error before even calling the factory, and updated the unit test accordingly.

@mdashlw for the body handling in undici.request(), the body is consumed before throwing, and we can't do so in undici.stream() unless we call the factory. So body will always be undefined in the error object.

[mcollina]

You can consume the body without calling the factory.

[assalielmehdi]

I updated the logic, to throw before calling the factory, and I handled the body so the thrown error contains it. I updated the tests accordingly. I hope that is the behavior we expect.

[codecov-commenter]

Codecov Report

Base: 90.28% // Head: 90.24% // Decreases project coverage by -0.03% :warning:

Coverage data is based on head (94dbebd) compared to base (93fd794). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1767      +/-   ##
==========================================
- Coverage   90.28%   90.24%   -0.04%     
==========================================
  Files          58       58              
  Lines        5188     5199      +11     
==========================================
+ Hits         4684     4692       +8     
- Misses        504      507       +3     

Impacted Files	Coverage Δ
lib/core/util.js	97.93% <ø> (ø)
lib/api/api-stream.js	100.00% <100.00%> (ø)
lib/fetch/body.js	95.87% <0.00%> (-0.46%)	:arrow_down:
lib/fetch/index.js	83.27% <0.00%> (-0.38%)	:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[assalielmehdi]

Hi @mcollina, can you review the changes please? 😊

[KhafraDev]

there are ci failures caused by a node-fetch test (npm run test:node-fetch)

[ronag]

This will be fixed once we move throwOnError to an interceptor.

https://github.com/nodejs/undici/issues/2835