Skip to content

Commit f8910e3

Browse files
jasnelltargos
jasnell
authored and
targos
committed
src: make multiple improvements to node_url_pattern
* remove USE from node_url_pattern to handle error correctly * simplify if block in node_url_pattern * improve error handling in node_url_pattern * fix error propagation on URLPattern init * use ToV8Value where more convenient in node_url_pattern * simplify URLPatternInit::ToJsObject by reducing boilerplate PR-URL: #56871 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
1 parent 3b305f2 commit f8910e3

File tree

3 files changed

+94
-104
lines changed

3 files changed

+94
-104
lines changed

‎src/node_url_pattern.cc‎

Lines changed: 64 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,10 @@ void URLPattern::New(const FunctionCallbackInfo<Value>& args) {
165165
input = input_buffer.ToString();
166166
} else if (args[0]->IsObject()) {
167167
init = URLPatternInit::FromJsObject(env, args[0].As<Object>());
168+
// If init does not have a value here, the implication is that an
169+
// error was thrown. Let's allow that to be handled now by returning
170+
// early. If we don't, the error thrown will be swallowed.
171+
if (!init) return;
168172
} else {
169173
THROW_ERR_INVALID_ARG_TYPE(env, "Input must be an object or a string");
170174
return;
@@ -180,7 +184,9 @@ void URLPattern::New(const FunctionCallbackInfo<Value>& args) {
180184
CHECK(!options.has_value());
181185
options = URLPatternOptions::FromJsObject(env, args[1].As<Object>());
182186
if (!options) {
183-
THROW_ERR_INVALID_ARG_TYPE(env, "options.ignoreCase must be a boolean");
187+
// If options does not have a value, we assume an error was
188+
// thrown and scheduled on the isolate. Return early to
189+
// propagate it.
184190
return;
185191
}
186192
} else {
@@ -197,7 +203,9 @@ void URLPattern::New(const FunctionCallbackInfo<Value>& args) {
197203
CHECK(!options.has_value());
198204
options = URLPatternOptions::FromJsObject(env, args[2].As<Object>());
199205
if (!options) {
200-
THROW_ERR_INVALID_ARG_TYPE(env, "options.ignoreCase must be a boolean");
206+
// If options does not have a value, we assume an error was
207+
// thrown and scheduled on the isolate. Return early to
208+
// propagate it.
201209
return;
202210
}
203211
}
@@ -234,73 +242,29 @@ MaybeLocal<Value> URLPattern::URLPatternInit::ToJsObject(
234242
auto isolate = env->isolate();
235243
auto context = env->context();
236244
auto result = Object::New(isolate);
237-
if (init.protocol) {
238-
Local<Value> protocol;
239-
if (!ToV8Value(context, *init.protocol).ToLocal(&protocol) ||
240-
result->Set(context, env->protocol_string(), protocol).IsNothing()) {
241-
return {};
242-
}
243-
}
244-
if (init.username) {
245-
Local<Value> username;
246-
if (!ToV8Value(context, *init.username).ToLocal(&username) ||
247-
result->Set(context, env->username_string(), username).IsNothing()) {
248-
return {};
249-
}
250-
}
251-
if (init.password) {
252-
Local<Value> password;
253-
if (!ToV8Value(context, *init.password).ToLocal(&password) ||
254-
result->Set(context, env->password_string(), password).IsNothing()) {
255-
return {};
256-
}
257-
}
258-
if (init.hostname) {
259-
Local<Value> hostname;
260-
if (!ToV8Value(context, *init.hostname).ToLocal(&hostname) ||
261-
result->Set(context, env->hostname_string(), hostname).IsNothing()) {
262-
return {};
263-
}
264-
}
265-
if (init.port) {
266-
Local<Value> port;
267-
if (!ToV8Value(context, *init.port).ToLocal(&port) ||
268-
result->Set(context, env->port_string(), port).IsNothing()) {
269-
return {};
270-
}
271-
}
272-
if (init.pathname) {
273-
Local<Value> pathname;
274-
if (!ToV8Value(context, *init.pathname).ToLocal(&pathname) ||
275-
result->Set(context, env->pathname_string(), pathname).IsNothing()) {
276-
return {};
277-
}
278-
}
279-
if (init.search) {
280-
Local<Value> search;
281-
if (!ToV8Value(context, *init.search).ToLocal(&search) ||
282-
result->Set(context, env->search_string(), search).IsNothing()) {
283-
return {};
284-
}
285-
}
286-
if (init.hash) {
287-
Local<Value> hash;
288-
if (!ToV8Value(context, *init.hash).ToLocal(&hash) ||
289-
result->Set(context, env->hash_string(), hash).IsNothing()) {
290-
return {};
291-
}
292-
}
293-
if (init.base_url) {
294-
Local<Value> base_url;
295-
if (!ToV8Value(context, *init.base_url).ToLocal(&base_url) ||
296-
result->Set(context, env->base_url_string(), base_url).IsNothing()) {
297-
return {};
298-
}
245+
246+
const auto trySet = [&](auto name, const std::optional<std::string>& val) {
247+
if (!val) return true;
248+
Local<Value> temp;
249+
return ToV8Value(context, *val).ToLocal(&temp) &&
250+
result->Set(context, name, temp).IsJust();
251+
};
252+
253+
if (!trySet(env->protocol_string(), init.protocol) ||
254+
!trySet(env->username_string(), init.username) ||
255+
!trySet(env->password_string(), init.password) ||
256+
!trySet(env->hostname_string(), init.hostname) ||
257+
!trySet(env->port_string(), init.port) ||
258+
!trySet(env->pathname_string(), init.pathname) ||
259+
!trySet(env->search_string(), init.search) ||
260+
!trySet(env->hash_string(), init.hash) ||
261+
!trySet(env->base_url_string(), init.base_url)) {
262+
return {};
299263
}
300264
return result;
301265
}
302266

303-
ada::url_pattern_init URLPattern::URLPatternInit::FromJsObject(
267+
std::optional<ada::url_pattern_init> URLPattern::URLPatternInit::FromJsObject(
304268
Environment* env, Local<Object> obj) {
305269
ada::url_pattern_init init{};
306270
Local<String> components[] = {
@@ -344,6 +308,10 @@ ada::url_pattern_init URLPattern::URLPatternInit::FromJsObject(
344308
Utf8Value utf8_value(isolate, value);
345309
set_parameter(key.ToStringView(), utf8_value.ToStringView());
346310
}
311+
} else {
312+
// If ToLocal failed then we assume an error occurred,
313+
// bail out early to propagate the error.
314+
return std::nullopt;
347315
}
348316
}
349317
return init;
@@ -355,12 +323,8 @@ MaybeLocal<Object> URLPattern::URLPatternComponentResult::ToJSObject(
355323
auto context = env->context();
356324
auto parsed_group = Object::New(isolate);
357325
for (const auto& [group_key, group_value] : result.groups) {
358-
Local<String> key;
359-
if (!String::NewFromUtf8(isolate,
360-
group_key.c_str(),
361-
NewStringType::kNormal,
362-
group_key.size())
363-
.ToLocal(&key)) {
326+
Local<Value> key;
327+
if (!ToV8Value(context, group_key).ToLocal(&key)) {
364328
return {};
365329
}
366330
Local<Value> value;
@@ -371,7 +335,9 @@ MaybeLocal<Object> URLPattern::URLPatternComponentResult::ToJSObject(
371335
} else {
372336
value = Undefined(isolate);
373337
}
374-
USE(parsed_group->Set(context, key, value));
338+
if (parsed_group->Set(context, key, value).IsNothing()) {
339+
return {};
340+
}
375341
}
376342
Local<Value> input;
377343
if (!ToV8Value(env->context(), result.input).ToLocal(&input)) {
@@ -418,34 +384,20 @@ MaybeLocal<Value> URLPattern::URLPatternResult::ToJSValue(
418384
LocalVector<Value> values(isolate, arraysize(names));
419385
values[0] = Array::New(isolate, inputs.data(), inputs.size());
420386
if (!URLPatternComponentResult::ToJSObject(env, result.protocol)
421-
.ToLocal(&values[1])) {
422-
return {};
423-
}
424-
if (!URLPatternComponentResult::ToJSObject(env, result.username)
425-
.ToLocal(&values[2])) {
426-
return {};
427-
}
428-
if (!URLPatternComponentResult::ToJSObject(env, result.password)
429-
.ToLocal(&values[3])) {
430-
return {};
431-
}
432-
if (!URLPatternComponentResult::ToJSObject(env, result.hostname)
433-
.ToLocal(&values[4])) {
434-
return {};
435-
}
436-
if (!URLPatternComponentResult::ToJSObject(env, result.port)
437-
.ToLocal(&values[5])) {
438-
return {};
439-
}
440-
if (!URLPatternComponentResult::ToJSObject(env, result.pathname)
441-
.ToLocal(&values[6])) {
442-
return {};
443-
}
444-
if (!URLPatternComponentResult::ToJSObject(env, result.search)
445-
.ToLocal(&values[7])) {
446-
return {};
447-
}
448-
if (!URLPatternComponentResult::ToJSObject(env, result.hash)
387+
.ToLocal(&values[1]) ||
388+
!URLPatternComponentResult::ToJSObject(env, result.username)
389+
.ToLocal(&values[2]) ||
390+
!URLPatternComponentResult::ToJSObject(env, result.password)
391+
.ToLocal(&values[3]) ||
392+
!URLPatternComponentResult::ToJSObject(env, result.hostname)
393+
.ToLocal(&values[4]) ||
394+
!URLPatternComponentResult::ToJSObject(env, result.port)
395+
.ToLocal(&values[5]) ||
396+
!URLPatternComponentResult::ToJSObject(env, result.pathname)
397+
.ToLocal(&values[6]) ||
398+
!URLPatternComponentResult::ToJSObject(env, result.search)
399+
.ToLocal(&values[7]) ||
400+
!URLPatternComponentResult::ToJSObject(env, result.hash)
449401
.ToLocal(&values[8])) {
450402
return {};
451403
}
@@ -461,9 +413,15 @@ URLPattern::URLPatternOptions::FromJsObject(Environment* env,
461413
if (obj->Get(env->context(), env->ignore_case_string())
462414
.ToLocal(&ignore_case)) {
463415
if (!ignore_case->IsBoolean()) {
416+
THROW_ERR_INVALID_ARG_TYPE(env, "options.ignoreCase must be a boolean");
464417
return std::nullopt;
465418
}
466419
options.ignore_case = ignore_case->IsTrue();
420+
} else {
421+
// If ToLocal returns false, the assumption is that getting the
422+
// ignore_case_string threw an error, let's propagate that now
423+
// by returning std::nullopt.
424+
return std::nullopt;
467425
}
468426
return options;
469427
}
@@ -553,7 +511,9 @@ void URLPattern::Exec(const FunctionCallbackInfo<Value>& args) {
553511
input_base = input_value.ToString();
554512
input = std::string_view(input_base);
555513
} else if (args[0]->IsObject()) {
556-
input = URLPatternInit::FromJsObject(env, args[0].As<Object>());
514+
auto maybeInput = URLPatternInit::FromJsObject(env, args[0].As<Object>());
515+
if (!maybeInput.has_value()) return;
516+
input = std::move(*maybeInput);
557517
} else {
558518
THROW_ERR_INVALID_ARG_TYPE(
559519
env, "URLPattern input needs to be a string or an object");
@@ -594,7 +554,9 @@ void URLPattern::Test(const FunctionCallbackInfo<Value>& args) {
594554
input_base = input_value.ToString();
595555
input = std::string_view(input_base);
596556
} else if (args[0]->IsObject()) {
597-
input = URLPatternInit::FromJsObject(env, args[0].As<Object>());
557+
auto maybeInput = URLPatternInit::FromJsObject(env, args[0].As<Object>());
558+
if (!maybeInput.has_value()) return;
559+
input = std::move(*maybeInput);
598560
} else {
599561
THROW_ERR_INVALID_ARG_TYPE(
600562
env, "URLPattern input needs to be a string or an object");

‎src/node_url_pattern.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ class URLPattern : public BaseObject {
5959

6060
class URLPatternInit {
6161
public:
62-
static ada::url_pattern_init FromJsObject(Environment* env,
63-
v8::Local<v8::Object> obj);
62+
static std::optional<ada::url_pattern_init> FromJsObject(
63+
Environment* env, v8::Local<v8::Object> obj);
6464
static v8::MaybeLocal<v8::Value> ToJsObject(
6565
Environment* env, const ada::url_pattern_init& init);
6666
};

‎test/parallel/test-urlpattern.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
'use strict';
2+
3+
require('../common');
4+
5+
const { throws } = require('assert');
6+
const { URLPattern } = require('url');
7+
8+
// Verify that if an error is thrown while accessing any of the
9+
// init options, the error is appropriately propagated.
10+
throws(() => {
11+
new URLPattern({
12+
get protocol() {
13+
throw new Error('boom');
14+
}
15+
});
16+
}, {
17+
message: 'boom',
18+
});
19+
20+
// Verify that if an error is thrown while accessing the ignoreCase
21+
// option, the error is appropriately propagated.
22+
throws(() => {
23+
new URLPattern({}, { get ignoreCase() {
24+
throw new Error('boom');
25+
} });
26+
}, {
27+
message: 'boom'
28+
});

0 commit comments

Comments
 (0)