From 30aa29fd648e1bbf912c7d7f561e5de7a1cd3eb4 Mon Sep 17 00:00:00 2001 From: Kai Tamkun Date: Mon, 7 Apr 2025 19:12:06 -0700 Subject: [PATCH] Fix ERR_INVALID_ARG_TYPE phrasing --- src/bun.js/bindings/ErrorCode.cpp | 152 ++++++++++++++---- src/js/node/http.ts | 2 +- ...est-http-client-reject-unexpected-agent.js | 69 ++++++++ 3 files changed, 195 insertions(+), 28 deletions(-) create mode 100644 test/js/node/test/parallel/test-http-client-reject-unexpected-agent.js diff --git a/src/bun.js/bindings/ErrorCode.cpp b/src/bun.js/bindings/ErrorCode.cpp index 15777ecbac..9f93253514 100644 --- a/src/bun.js/bindings/ErrorCode.cpp +++ b/src/bun.js/bindings/ErrorCode.cpp @@ -537,33 +537,131 @@ WTF::String ERR_INVALID_ARG_TYPE(JSC::ThrowScope& scope, JSC::JSGlobalObject* gl result.append("The "_s); addParameter(result, arg_name); result.append(" must be "_s); - result.append("of type "_s); - unsigned length = expected_types.size(); - if (length == 1) { - auto* str = expected_types.at(0).toString(globalObject); - RETURN_IF_EXCEPTION(scope, {}); - result.append(str->view(globalObject)); - } else if (length == 2) { - auto* str1 = expected_types.at(0).toString(globalObject); - RETURN_IF_EXCEPTION(scope, {}); - result.append(str1->view(globalObject)); - result.append(" or "_s); - auto* str2 = expected_types.at(1).toString(globalObject); - RETURN_IF_EXCEPTION(scope, {}); - result.append(str2->view(globalObject)); - } else { - for (unsigned i = 0, end = length - 1; i < end; i++) { - JSValue expected_type = expected_types.at(i); - auto* str = expected_type.toString(globalObject); - RETURN_IF_EXCEPTION(scope, {}); - result.append(str->view(globalObject)); - result.append(", "_s); + auto isCommon = [](WTF::String type) -> bool { + constexpr static std::array commonTypes = { + "string"_s, + "function"_s, + "number"_s, + "object"_s, + "Function"_s, + "Object"_s, + "boolean"_s, + "bigint"_s, + "symbol"_s, + }; + + for (ASCIILiteral common : commonTypes) { + if (type == common) { + return true; + } } - result.append("or "_s); - auto* str = expected_types.at(length - 1).toString(globalObject); - RETURN_IF_EXCEPTION(scope, {}); - result.append(str->view(globalObject)); + + return false; + }; + + auto isClass = [](WTF::String type) -> bool { + const std::locale& locale = std::locale::classic(); + + if (!type.containsOnlyASCII()) { + return false; + } + + const auto span = type.span8(); + + if (span.empty() || !std::isupper(span[0], locale)) { + return false; + } + + for (unsigned i = 1; i < span.size(); ++i) { + if (!std::isalnum(span[i], locale)) { + return false; + } + } + + return true; + }; + + auto formatList = [](WTF::Vector& types, WTF::StringBuilder& builder, WTF::ASCIILiteral spacedConjunction) -> void { + switch (types.size()) { + case 0: + break; + case 1: + builder.append(types[0]); + case 2: + builder.append(types[0]); + builder.append(spacedConjunction); + builder.append(types[1]); + break; + default: + for (unsigned i = 0; i < types.size() - 2; i++) { + builder.append(types[i]); + builder.append(", "_s); + } + builder.append(types[types.size() - 2]); + builder.append(","_s); + builder.append(spacedConjunction); + builder.append(types[types.size() - 1]); + break; + } + }; + + WTF::Vector types, instances, other; + + for (unsigned i = 0; i < expected_types.size(); i++) { + JSValue value = expected_types.at(i); + + if (!value.isString()) { + scope.throwException(globalObject, createError(globalObject, ErrorCode::ERR_INTERNAL_ASSERTION, "All expected entries have to be of type string"_s)); + return {}; + } + + WTF::String valueString = value.getString(globalObject); + if (isCommon(valueString)) { + types.append(valueString.convertToASCIILowercase()); + } else if (isClass(valueString)) { + instances.append(WTFMove(valueString)); + } else { + if (valueString == "object") { + scope.throwException(globalObject, createError(globalObject, ErrorCode::ERR_INTERNAL_ASSERTION, "The value \"object\" should be written as \"Object\""_s)); + return {}; + } + other.append(WTFMove(valueString)); + } + } + + if (!instances.isEmpty() && types.removeFirst("object"_s)) { + instances.append("Object"_s); + } + + if (!types.isEmpty()) { + if (types.size() > 1) { + result.append("one of type "_s); + } else { + result.append("of type "_s); + } + formatList(types, result, " or "); + if (!instances.isEmpty() || !other.isEmpty()) { + result.append(" or "_s); + } + } + + if (!instances.isEmpty()) { + result.append("an instance of "_s); + formatList(instances, result, " or "); + if (!other.isEmpty()) { + result.append(" or "_s); + } + } + + if (other.size() > 1) { + result.append("one of "_s); + formatList(other, result, " or "); + } else if (!other.isEmpty()) { + if (other[0].convertToASCIILowercase() != other[0]) { + result.append("an "_s); + } + result.append(other[0]); } result.append(". Received "_s); @@ -573,7 +671,8 @@ WTF::String ERR_INVALID_ARG_TYPE(JSC::ThrowScope& scope, JSC::JSGlobalObject* gl return result.toString(); } -WTF::String ERR_INVALID_ARG_TYPE(JSC::ThrowScope& scope, JSC::JSGlobalObject* globalObject, const ZigString* arg_name_string, const ZigString* expected_type_string, JSValue actual_value) +WTF::String +ERR_INVALID_ARG_TYPE(JSC::ThrowScope& scope, JSC::JSGlobalObject* globalObject, const ZigString* arg_name_string, const ZigString* expected_type_string, JSValue actual_value) { auto arg_name = std::span(arg_name_string->ptr, arg_name_string->len); ASSERT(WTF::charactersAreAllASCII(arg_name)); @@ -622,7 +721,6 @@ WTF::String ERR_OUT_OF_RANGE(JSC::ThrowScope& scope, JSC::JSGlobalObject* global return builder.toString(); } - } namespace ERR { diff --git a/src/js/node/http.ts b/src/js/node/http.ts index eb3b15b914..275471f0db 100644 --- a/src/js/node/http.ts +++ b/src/js/node/http.ts @@ -3054,7 +3054,7 @@ function ClientRequest(input, options, cb) { } else if (agent == null) { agent = defaultAgent; } else if (typeof agent.addRequest !== "function") { - throw $ERR_INVALID_ARG_TYPE("options.agent", "Agent-like Object, undefined, or false", agent); + throw $ERR_INVALID_ARG_TYPE("options.agent", ["Agent-like Object", "undefined", "false"], agent); } this[kAgent] = agent; this.destroyed = false; diff --git a/test/js/node/test/parallel/test-http-client-reject-unexpected-agent.js b/test/js/node/test/parallel/test-http-client-reject-unexpected-agent.js new file mode 100644 index 0000000000..8ec6506888 --- /dev/null +++ b/test/js/node/test/parallel/test-http-client-reject-unexpected-agent.js @@ -0,0 +1,69 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const baseOptions = { + method: 'GET', + port: undefined, + host: common.localhostIPv4, +}; + +const failingAgentOptions = [ + true, + 'agent', + {}, + 1, + () => null, + Symbol(), +]; + +const acceptableAgentOptions = [ + false, + undefined, + null, + new http.Agent(), +]; + +const server = http.createServer((req, res) => { + res.end('hello'); +}); + +let numberOfResponses = 0; + +function createRequest(agent) { + const options = Object.assign(baseOptions, { agent }); + const request = http.request(options); + request.end(); + request.on('response', common.mustCall(() => { + numberOfResponses++; + if (numberOfResponses === acceptableAgentOptions.length) { + server.close(); + } + })); +} + +server.listen(0, baseOptions.host, common.mustCall(function() { + baseOptions.port = this.address().port; + + failingAgentOptions.forEach((agent) => { + assert.throws( + () => createRequest(agent), + { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "options.agent" property must be one of Agent-like ' + + 'Object, undefined, or false.' + + common.invalidArgTypeHelper(agent) + } + ); + }); + + acceptableAgentOptions.forEach((agent) => { + createRequest(agent); + }); +})); + +process.on('exit', () => { + assert.strictEqual(numberOfResponses, acceptableAgentOptions.length); +});