From b27804ba00409da2555153d8795bdac1092ccb9d Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Wed, 18 May 2022 20:33:22 +0300 Subject: [PATCH 1/7] Improve coverage --- docs/api/Dispatcher.md | 3 ++- lib/api/api-request.js | 15 ++++++++--- lib/core/errors.js | 11 ++++++++ lib/core/request.js | 5 +++- test/client.js | 58 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 87 insertions(+), 5 deletions(-) diff --git a/docs/api/Dispatcher.md b/docs/api/Dispatcher.md index 56b34275209..9a4c63dabe8 100644 --- a/docs/api/Dispatcher.md +++ b/docs/api/Dispatcher.md @@ -199,13 +199,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. * **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. diff --git a/lib/api/api-request.js b/lib/api/api-request.js index bcfe483ebef..efa2c96b2be 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -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') @@ -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') { @@ -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) => { @@ -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) { @@ -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) { + this.runInAsyncScope(callback, null, + new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`) + ) + return + } + this.runInAsyncScope(callback, null, null, { statusCode, headers, diff --git a/lib/core/errors.js b/lib/core/errors.js index f480f31a176..4017525714b 100644 --- a/lib/core/errors.js +++ b/lib/core/errors.js @@ -56,6 +56,16 @@ class BodyTimeoutError extends UndiciError { } } +class ResponseStatusCodeError extends UndiciError { + constructor (message) { + super(message) + Error.captureStackTrace(this, ResponseStatusCodeError) + this.name = 'ResponseStatusCodeError' + this.message = message || 'ResponseStatusCode Error' + this.code = 'UND_ERR_RESPONSE_STATUS_CODE' + } +} + class InvalidArgumentError extends UndiciError { constructor (message) { super(message) @@ -186,6 +196,7 @@ module.exports = { BodyTimeoutError, RequestContentLengthMismatchError, ConnectTimeoutError, + ResponseStatusCodeError, InvalidArgumentError, InvalidReturnValueError, RequestAbortedError, diff --git a/lib/core/request.js b/lib/core/request.js index f04fe4fab21..92e98935cf4 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -42,7 +42,8 @@ class Request { blocking, upgrade, headersTimeout, - bodyTimeout + bodyTimeout, + throwOnError }, handler) { if (typeof path !== 'string') { throw new InvalidArgumentError('path must be a string') @@ -70,6 +71,8 @@ class Request { this.bodyTimeout = bodyTimeout + this.throwOnError = throwOnError !== undefined ? throwOnError : false + this.method = method if (body == null) { diff --git a/test/client.js b/test/client.js index 6469ce65165..48d41336a4b 100644 --- a/test/client.js +++ b/test/client.js @@ -80,6 +80,64 @@ test('basic get', (t) => { }) }) +test('basic get returns 400 when configured to throw on errors (callback)', (t) => { + t.plan(2) + + 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, + }, (err) => { + t.equal(err.message, 'Response status code 400: Bad Request') + }) + t.equal(signal.listenerCount('abort'), 1) + }) +}) + +test('basic get returns 400 when configured to throw on errors (promise)', (t) => { + t.plan(1) + + 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') + } + }) +}) + test('basic head', (t) => { t.plan(14) From 01cbde5fa7085587cfbaaf6487a7eaf205228b1a Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Wed, 18 May 2022 20:35:40 +0300 Subject: [PATCH 2/7] Update TS types --- types/dispatcher.d.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/types/dispatcher.d.ts b/types/dispatcher.d.ts index 9b2af26e6d2..6e4a60c438f 100644 --- a/types/dispatcher.d.ts +++ b/types/dispatcher.d.ts @@ -55,6 +55,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; } export interface ConnectOptions { path: string; From 8f57b26541bab3655b1fc38dabad68aeaf6c85ea Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Wed, 18 May 2022 20:38:52 +0300 Subject: [PATCH 3/7] Fix linting --- test/client.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/client.js b/test/client.js index 48d41336a4b..7cceb7b0fcd 100644 --- a/test/client.js +++ b/test/client.js @@ -100,7 +100,7 @@ test('basic get returns 400 when configured to throw on errors (callback)', (t) signal, path: '/', method: 'GET', - throwOnError: true, + throwOnError: true }, (err) => { t.equal(err.message, 'Response status code 400: Bad Request') }) @@ -112,7 +112,7 @@ test('basic get returns 400 when configured to throw on errors (promise)', (t) = t.plan(1) const server = createServer((req, res) => { - res.writeHead( 400, 'Invalid params', {'content-type' : 'text/plain'}); + res.writeHead(400, 'Invalid params', { 'content-type': 'text/plain' }) res.end('Invalid params') }) t.teardown(server.close.bind(server)) From e5f7c2bdc8a549e1583a7971f7f6cb23a793556f Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Thu, 19 May 2022 22:36:54 +0300 Subject: [PATCH 4/7] Improve naming, add type tests --- lib/core/errors.js | 2 +- test/types/dispatcher.test-d.ts | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/core/errors.js b/lib/core/errors.js index 4017525714b..1c44c6e4501 100644 --- a/lib/core/errors.js +++ b/lib/core/errors.js @@ -61,7 +61,7 @@ class ResponseStatusCodeError extends UndiciError { super(message) Error.captureStackTrace(this, ResponseStatusCodeError) this.name = 'ResponseStatusCodeError' - this.message = message || 'ResponseStatusCode Error' + this.message = message || 'Response Status Code Error' this.code = 'UND_ERR_RESPONSE_STATUS_CODE' } } diff --git a/test/types/dispatcher.test-d.ts b/test/types/dispatcher.test-d.ts index 5c47cfb14a6..783fe85c2bb 100644 --- a/test/types/dispatcher.test-d.ts +++ b/test/types/dispatcher.test-d.ts @@ -26,6 +26,9 @@ expectAssignable(new Dispatcher()) // request expectAssignable>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0 })) + expectAssignable>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0, query: {} })) + expectAssignable>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0, query: { pageNum: 1, id: 'abc' } })) + expectAssignable>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0, throwOnError: true })) expectAssignable>(dispatcher.request({ origin: new URL('http://localhost'), path: '', method: 'GET' })) expectAssignable(dispatcher.request({ origin: '', path: '', method: 'GET' }, (err, data) => { expectAssignable(err) From 2b4d087b57a48b7b7b6d9268a0f8d179f6c0fbdf Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Thu, 19 May 2022 22:45:40 +0300 Subject: [PATCH 5/7] Address code review comments --- lib/api/api-request.js | 2 +- lib/core/errors.js | 5 ++++- test/client.js | 12 ++++++++++-- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/api/api-request.js b/lib/api/api-request.js index efa2c96b2be..2261a62f9ac 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -93,7 +93,7 @@ class RequestHandler extends AsyncResource { if (callback !== null) { if (this.throwOnError && statusCode > 399) { this.runInAsyncScope(callback, null, - new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`) + new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers) ) return } diff --git a/lib/core/errors.js b/lib/core/errors.js index 1c44c6e4501..a36fd067c9f 100644 --- a/lib/core/errors.js +++ b/lib/core/errors.js @@ -57,12 +57,15 @@ class BodyTimeoutError extends UndiciError { } class ResponseStatusCodeError extends UndiciError { - constructor (message) { + 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 } } diff --git a/test/client.js b/test/client.js index 913de1dd361..e953f6d3729 100644 --- a/test/client.js +++ b/test/client.js @@ -316,7 +316,7 @@ 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(2) + t.plan(6) const server = createServer((req, res) => { res.statusCode = 400 @@ -338,13 +338,17 @@ test('basic get returns 400 when configured to throw on errors (callback)', (t) throwOnError: true }, (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(1) + t.plan(5) const server = createServer((req, res) => { res.writeHead(400, 'Invalid params', { 'content-type': 'text/plain' }) @@ -369,6 +373,10 @@ test('basic get returns 400 when configured to throw on errors (promise)', (t) = 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') } }) }) From b36f767c967dee21afb1643811f22b0b10fd192d Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Fri, 20 May 2022 10:12:48 +0300 Subject: [PATCH 6/7] make check explicit Co-authored-by: Robert Nagy --- lib/core/request.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/core/request.js b/lib/core/request.js index 448f50ee979..2a13b0549a8 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -72,7 +72,7 @@ class Request { this.bodyTimeout = bodyTimeout - this.throwOnError = throwOnError !== undefined ? throwOnError : false + this.throwOnError = throwOnError === true this.method = method From d3d6aa0df1cc508936f4872589e8fc2b03d3e5f9 Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Fri, 20 May 2022 10:13:53 +0300 Subject: [PATCH 7/7] make condition more explicit Co-authored-by: Robert Nagy --- lib/api/api-request.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/api-request.js b/lib/api/api-request.js index 2261a62f9ac..2fc5afa991c 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -91,7 +91,7 @@ class RequestHandler extends AsyncResource { const headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders) if (callback !== null) { - if (this.throwOnError && statusCode > 399) { + if (this.throwOnError && statusCode >= 400) { this.runInAsyncScope(callback, null, new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers) )