From 76d976027d72f919bc031b980b9111de4b9f5baf Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Wed, 11 Jan 2023 23:49:07 +0530 Subject: [PATCH 1/4] lib: refactor to use validators in http2 Refs: https://github.com/nodejs/node/pull/46101 --- lib/internal/http2/core.js | 28 ++++++++++--------- ...est-http2-client-request-options-errors.js | 25 +++++++++++++++-- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 48b73ae54ce9f6..836dec0b29589d 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -126,7 +126,8 @@ const { validateNumber, validateString, validateUint32, - validateAbortSignal + validateAbortSignal, + validateBoolean, } = require('internal/validators'); const fsPromisesInternal = require('internal/fs/promises'); const { utcDate } = require('internal/http'); @@ -761,27 +762,27 @@ function requestOnConnect(headers, options) { const setAndValidatePriorityOptions = hideStackFrames((options) => { if (options.weight === undefined) { options.weight = NGHTTP2_DEFAULT_WEIGHT; - } else if (typeof options.weight !== 'number') { - throw new ERR_INVALID_ARG_VALUE('options.weight', options.weight); - } + } + + validateNumber(options.weight, 'options.weight'); if (options.parent === undefined) { options.parent = 0; - } else if (typeof options.parent !== 'number' || options.parent < 0) { - throw new ERR_INVALID_ARG_VALUE('options.parent', options.parent); - } + } + + validateNumber(options.parent, 'options.parent', 0); if (options.exclusive === undefined) { options.exclusive = false; - } else if (typeof options.exclusive !== 'boolean') { - throw new ERR_INVALID_ARG_VALUE('options.exclusive', options.exclusive); } + validateBoolean(options.exclusive, 'options.exclusive'); + if (options.silent === undefined) { options.silent = false; - } else if (typeof options.silent !== 'boolean') { - throw new ERR_INVALID_ARG_VALUE('options.silent', options.silent); } + + validateBoolean(options.silent, 'options.silent'); }); // When an error occurs internally at the binding level, immediately @@ -1784,10 +1785,11 @@ class ClientHttp2Session extends Http2Session { // stream by default if the user has not specifically indicated a // preference. options.endStream = isPayloadMeaningless(headers[HTTP2_HEADER_METHOD]); - } else if (typeof options.endStream !== 'boolean') { - throw new ERR_INVALID_ARG_VALUE('options.endStream', options.endStream); } + validateBoolean(options.endStream, 'options.endStream'); + + const headersList = mapToHeaders(headers); const stream = new ClientHttp2Stream(this, undefined, undefined, {}); diff --git a/test/parallel/test-http2-client-request-options-errors.js b/test/parallel/test-http2-client-request-options-errors.js index f2950fe4d287b4..9d7cec2abfa479 100644 --- a/test/parallel/test-http2-client-request-options-errors.js +++ b/test/parallel/test-http2-client-request-options-errors.js @@ -28,6 +28,25 @@ const types = { symbol: Symbol('test') }; +function determineSpecificType(value) { + if (value == null) { + return '' + value; + } + if (typeof value === 'function' && value.name) { + return `function ${value.name}`; + } + if (typeof value === 'object') { + if (value.constructor?.name) { + return `an instance of ${value.constructor.name}`; + } + return `${inspect(value, { depth: -1 })}`; + } + let inspected = inspect(value, { colors: false }); + if (inspected.length > 28) { inspected = `${StringPrototypeSlice(inspected, 0, 25)}...`; } + + return `type ${typeof value} (${inspected})`; +} + const server = http2.createServer(common.mustNotCall()); server.listen(0, common.mustCall(() => { @@ -48,9 +67,9 @@ server.listen(0, common.mustCall(() => { [option]: types[type] }), { name: 'TypeError', - code: 'ERR_INVALID_ARG_VALUE', - message: `The property 'options.${option}' is invalid. ` + - `Received ${inspect(types[type])}` + code: 'ERR_INVALID_ARG_TYPE', + message: `The "options.${option}" property must be of type ${optionsToTest[option]}. ` + + `Received ${determineSpecificType(types[type])}` }); }); }); From aa1bc5c423ae3cd567b0436c50caafee45eee70a Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Thu, 12 Jan 2023 00:16:20 +0530 Subject: [PATCH 2/4] fixup! lint fix --- lib/internal/http2/core.js | 5 ++--- test/parallel/test-http2-client-request-options-errors.js | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 836dec0b29589d..fc146fdab7ff99 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -762,13 +762,13 @@ function requestOnConnect(headers, options) { const setAndValidatePriorityOptions = hideStackFrames((options) => { if (options.weight === undefined) { options.weight = NGHTTP2_DEFAULT_WEIGHT; - } + } validateNumber(options.weight, 'options.weight'); if (options.parent === undefined) { options.parent = 0; - } + } validateNumber(options.parent, 'options.parent', 0); @@ -1788,7 +1788,6 @@ class ClientHttp2Session extends Http2Session { } validateBoolean(options.endStream, 'options.endStream'); - const headersList = mapToHeaders(headers); diff --git a/test/parallel/test-http2-client-request-options-errors.js b/test/parallel/test-http2-client-request-options-errors.js index 9d7cec2abfa479..fe9e246f25389d 100644 --- a/test/parallel/test-http2-client-request-options-errors.js +++ b/test/parallel/test-http2-client-request-options-errors.js @@ -42,7 +42,7 @@ function determineSpecificType(value) { return `${inspect(value, { depth: -1 })}`; } let inspected = inspect(value, { colors: false }); - if (inspected.length > 28) { inspected = `${StringPrototypeSlice(inspected, 0, 25)}...`; } + if (inspected.length > 28) { inspected = `${inspected.slice(0, 25)}...`; } return `type ${typeof value} (${inspected})`; } From b6ab0d9ed604f11b33967c9b55f5450644ced475 Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Thu, 12 Jan 2023 13:33:07 +0530 Subject: [PATCH 3/4] fixup! prevent unnecessary checking --- lib/internal/http2/core.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index fc146fdab7ff99..5c2959d0f26ef6 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -762,27 +762,27 @@ function requestOnConnect(headers, options) { const setAndValidatePriorityOptions = hideStackFrames((options) => { if (options.weight === undefined) { options.weight = NGHTTP2_DEFAULT_WEIGHT; + } else { + validateNumber(options.weight, 'options.weight'); } - validateNumber(options.weight, 'options.weight'); - if (options.parent === undefined) { options.parent = 0; + } else { + validateNumber(options.parent, 'options.parent', 0); } - validateNumber(options.parent, 'options.parent', 0); - if (options.exclusive === undefined) { options.exclusive = false; + } else { + validateBoolean(options.exclusive, 'options.exclusive'); } - validateBoolean(options.exclusive, 'options.exclusive'); - if (options.silent === undefined) { options.silent = false; + } else { + validateBoolean(options.silent, 'options.silent'); } - - validateBoolean(options.silent, 'options.silent'); }); // When an error occurs internally at the binding level, immediately @@ -1785,10 +1785,10 @@ class ClientHttp2Session extends Http2Session { // stream by default if the user has not specifically indicated a // preference. options.endStream = isPayloadMeaningless(headers[HTTP2_HEADER_METHOD]); + } else { + validateBoolean(options.endStream, 'options.endStream'); } - validateBoolean(options.endStream, 'options.endStream'); - const headersList = mapToHeaders(headers); const stream = new ClientHttp2Stream(this, undefined, undefined, {}); From 293ccd7d12da44a61fe41ce62c7bc106a40e786a Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Thu, 12 Jan 2023 13:42:53 +0530 Subject: [PATCH 4/4] fixup! remove message checking in test! --- ...est-http2-client-request-options-errors.js | 22 ------------------- 1 file changed, 22 deletions(-) diff --git a/test/parallel/test-http2-client-request-options-errors.js b/test/parallel/test-http2-client-request-options-errors.js index fe9e246f25389d..f3c0c57965cf97 100644 --- a/test/parallel/test-http2-client-request-options-errors.js +++ b/test/parallel/test-http2-client-request-options-errors.js @@ -5,7 +5,6 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const http2 = require('http2'); -const { inspect } = require('util'); // Check if correct errors are emitted when wrong type of data is passed // to certain options of ClientHttp2Session request method @@ -28,25 +27,6 @@ const types = { symbol: Symbol('test') }; -function determineSpecificType(value) { - if (value == null) { - return '' + value; - } - if (typeof value === 'function' && value.name) { - return `function ${value.name}`; - } - if (typeof value === 'object') { - if (value.constructor?.name) { - return `an instance of ${value.constructor.name}`; - } - return `${inspect(value, { depth: -1 })}`; - } - let inspected = inspect(value, { colors: false }); - if (inspected.length > 28) { inspected = `${inspected.slice(0, 25)}...`; } - - return `type ${typeof value} (${inspected})`; -} - const server = http2.createServer(common.mustNotCall()); server.listen(0, common.mustCall(() => { @@ -68,8 +48,6 @@ server.listen(0, common.mustCall(() => { }), { name: 'TypeError', code: 'ERR_INVALID_ARG_TYPE', - message: `The "options.${option}" property must be of type ${optionsToTest[option]}. ` + - `Received ${determineSpecificType(types[type])}` }); }); });