Skip to content
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

feat: Option to throw on error status codes #1453

Merged
merged 8 commits into from May 20, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/api/Dispatcher.md
Expand Up @@ -200,13 +200,14 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo
* **upgrade** `string | null` (optional) - Default: `null` - Upgrade the request. Should be used to specify the kind of upgrade i.e. `'Websocket'`.
* **bodyTimeout** `number | null` (optional) - The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use `0` to disable it entirely. Defaults to 30 seconds.
* **headersTimeout** `number | null` (optional) - The amount of time the parser will wait to receive the complete HTTP headers. Defaults to 30 seconds.
* **throwOnError** `boolean` (optional) - Default: `false` - Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server.

#### Parameter: `DispatchHandler`

* **onConnect** `(abort: () => void, context: object) => void` - Invoked before request is dispatched on socket. May be invoked multiple times when a request is retried when the request at the head of the pipeline fails.
* **onError** `(error: Error) => void` - Invoked when an error has occurred. May not throw.
* **onUpgrade** `(statusCode: number, headers: Buffer[], socket: Duplex) => void` (optional) - Invoked when request is upgraded. Required if `DispatchOptions.upgrade` is defined or `DispatchOptions.method === 'CONNECT'`.
* **onHeaders** `(statusCode: number, headers: Buffer[], resume: () => void) => boolean` - Invoked when statusCode and headers have been received. May be invoked multiple times due to 1xx informational headers. Not required for `upgrade` requests.
* **onHeaders** `(statusCode: number, headers: Buffer[], resume: () => void, statusText: string) => boolean` - Invoked when statusCode and headers have been received. May be invoked multiple times due to 1xx informational headers. Not required for `upgrade` requests.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was always passed, but neither used nor documented

* **onData** `(chunk: Buffer) => boolean` - Invoked when response payload data is received. Not required for `upgrade` requests.
* **onComplete** `(trailers: Buffer[]) => void` - Invoked when response payload and trailers have been received and the request has completed. Not required for `upgrade` requests.
* **onBodySent** `(chunk: string | Buffer | Uint8Array) => void` - Invoked when a body chunk is sent to the server. Not required. For a stream or iterable body this will be invoked for every chunk. For other body types, it will be invoked once after the body is sent.
Expand Down
15 changes: 12 additions & 3 deletions lib/api/api-request.js
Expand Up @@ -3,7 +3,8 @@
const Readable = require('./readable')
const {
InvalidArgumentError,
RequestAbortedError
RequestAbortedError,
ResponseStatusCodeError
} = require('../core/errors')
const util = require('../core/util')
const { AsyncResource } = require('async_hooks')
Expand All @@ -15,7 +16,7 @@ class RequestHandler extends AsyncResource {
throw new InvalidArgumentError('invalid opts')
}

const { signal, method, opaque, body, onInfo, responseHeaders } = opts
const { signal, method, opaque, body, onInfo, responseHeaders, throwOnError } = opts

try {
if (typeof callback !== 'function') {
Expand Down Expand Up @@ -51,6 +52,7 @@ class RequestHandler extends AsyncResource {
this.trailers = {}
this.context = null
this.onInfo = onInfo || null
this.throwOnError = throwOnError

if (util.isStream(body)) {
body.on('error', (err) => {
Expand All @@ -70,7 +72,7 @@ class RequestHandler extends AsyncResource {
this.context = context
}

onHeaders (statusCode, rawHeaders, resume) {
onHeaders (statusCode, rawHeaders, resume, statusMessage) {
const { callback, opaque, abort, context } = this

if (statusCode < 200) {
Expand All @@ -89,6 +91,13 @@ class RequestHandler extends AsyncResource {
const headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)

if (callback !== null) {
if (this.throwOnError && statusCode > 399) {
kibertoad marked this conversation as resolved.
Show resolved Hide resolved
this.runInAsyncScope(callback, null,
new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers)
)
return
}

this.runInAsyncScope(callback, null, null, {
statusCode,
headers,
Expand Down
14 changes: 14 additions & 0 deletions lib/core/errors.js
Expand Up @@ -56,6 +56,19 @@ class BodyTimeoutError extends UndiciError {
}
}

class ResponseStatusCodeError extends UndiciError {
constructor (message, statusCode, headers) {
super(message)
Error.captureStackTrace(this, ResponseStatusCodeError)
this.name = 'ResponseStatusCodeError'
this.message = message || 'Response Status Code Error'
this.code = 'UND_ERR_RESPONSE_STATUS_CODE'
this.status = statusCode
this.statusCode = statusCode
this.headers = headers
}
}

class InvalidArgumentError extends UndiciError {
constructor (message) {
super(message)
Expand Down Expand Up @@ -186,6 +199,7 @@ module.exports = {
BodyTimeoutError,
RequestContentLengthMismatchError,
ConnectTimeoutError,
ResponseStatusCodeError,
InvalidArgumentError,
InvalidReturnValueError,
RequestAbortedError,
Expand Down
5 changes: 4 additions & 1 deletion lib/core/request.js
Expand Up @@ -43,7 +43,8 @@ class Request {
blocking,
upgrade,
headersTimeout,
bodyTimeout
bodyTimeout,
throwOnError
}, handler) {
if (typeof path !== 'string') {
throw new InvalidArgumentError('path must be a string')
Expand Down Expand Up @@ -71,6 +72,8 @@ class Request {

this.bodyTimeout = bodyTimeout

this.throwOnError = throwOnError !== undefined ? throwOnError : false
kibertoad marked this conversation as resolved.
Show resolved Hide resolved

this.method = method

if (body == null) {
Expand Down
66 changes: 66 additions & 0 deletions test/client.js
Expand Up @@ -315,6 +315,72 @@ test('basic get with query params partially in path', (t) => {
})
})

test('basic get returns 400 when configured to throw on errors (callback)', (t) => {
t.plan(6)

const server = createServer((req, res) => {
res.statusCode = 400
res.end('hello')
})
t.teardown(server.close.bind(server))

server.listen(0, () => {
const client = new Client(`http://localhost:${server.address().port}`, {
keepAliveTimeout: 300e3
})
t.teardown(client.close.bind(client))

const signal = new EE()
client.request({
signal,
path: '/',
method: 'GET',
throwOnError: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a better name for the option, in this example is not throwing at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas? What aspects would you like emphasized? Axios uses validateStatus for similar config, got uses throwHttpErrors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ronag any preference on the name?

}, (err) => {
t.equal(err.message, 'Response status code 400: Bad Request')
t.equal(err.status, 400)
t.equal(err.statusCode, 400)
t.equal(err.headers.connection, 'keep-alive')
t.equal(err.headers['content-length'], '5')
})
t.equal(signal.listenerCount('abort'), 1)
})
})

test('basic get returns 400 when configured to throw on errors (promise)', (t) => {
t.plan(5)

const server = createServer((req, res) => {
res.writeHead(400, 'Invalid params', { 'content-type': 'text/plain' })
res.end('Invalid params')
})
t.teardown(server.close.bind(server))

server.listen(0, async () => {
const client = new Client(`http://localhost:${server.address().port}`, {
keepAliveTimeout: 300e3
})
t.teardown(client.close.bind(client))

const signal = new EE()
try {
await client.request({
signal,
path: '/',
method: 'GET',
throwOnError: true
})
t.fail('Should throw an error')
} catch (err) {
t.equal(err.message, 'Response status code 400: Invalid params')
t.equal(err.status, 400)
t.equal(err.statusCode, 400)
t.equal(err.headers.connection, 'keep-alive')
t.equal(err.headers['content-type'], 'text/plain')
}
})
})

test('basic head', (t) => {
t.plan(14)

Expand Down
3 changes: 3 additions & 0 deletions test/types/dispatcher.test-d.ts
Expand Up @@ -26,6 +26,9 @@ expectAssignable<Dispatcher>(new Dispatcher())

// request
expectAssignable<Promise<Dispatcher.ResponseData>>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0 }))
expectAssignable<Promise<Dispatcher.ResponseData>>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0, query: {} }))
expectAssignable<Promise<Dispatcher.ResponseData>>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0, query: { pageNum: 1, id: 'abc' } }))
expectAssignable<Promise<Dispatcher.ResponseData>>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0, throwOnError: true }))
expectAssignable<Promise<Dispatcher.ResponseData>>(dispatcher.request({ origin: new URL('http://localhost'), path: '', method: 'GET' }))
expectAssignable<void>(dispatcher.request({ origin: '', path: '', method: 'GET' }, (err, data) => {
expectAssignable<Error | null>(err)
Expand Down
2 changes: 2 additions & 0 deletions types/dispatcher.d.ts
Expand Up @@ -57,6 +57,8 @@ declare namespace Dispatcher {
headersTimeout?: number | null;
/** The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use 0 to disable it entirely. Defaults to 30 seconds. */
bodyTimeout?: number | null;
/** Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server. Defaults to false */
throwOnError?: boolean;
kibertoad marked this conversation as resolved.
Show resolved Hide resolved
}
export interface ConnectOptions {
path: string;
Expand Down