Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit f8910e3

Browse filesBrowse 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 <yagiz@nizipli.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent 3b305f2 commit f8910e3
Copy full SHA for f8910e3

File tree

Expand file treeCollapse file tree

3 files changed

+94
-104
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+94
-104
lines changed
Open diff view settings
Collapse file

‎src/node_url_pattern.cc‎

Copy file name to clipboardExpand all lines: src/node_url_pattern.cc
+64-102Lines 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");
Collapse file

‎src/node_url_pattern.h‎

Copy file name to clipboardExpand all lines: src/node_url_pattern.h
+2-2Lines 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
};
Collapse file

‎test/parallel/test-urlpattern.js‎

Copy file name to clipboard
+28Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.