url: simplify and improve url formatting

anonrig · danielleadams · commit a39dd376538e · 2023-04-11T22:55:03.000-04:00
PR-URL: #46736 Backport-PR-URL: #47435 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/lib/internal/url.js b/lib/internal/url.js
@@ -79,7 +79,6 @@ const path = require('path');
 
 const {
   validateFunction,
-  validateObject,
 } = require('internal/validators');
 
 const querystring = require('querystring');
@@ -152,8 +151,6 @@ class URLContext {
   password = '';
   port = '';
   hash = '';
-  hasHost = false;
-  hasOpaquePath = false;
 }
 
 function isURLSearchParams(self) {
@@ -290,7 +287,9 @@ class URLSearchParams {
     name = toUSVString(name);
     value = toUSVString(value);
     ArrayPrototypePush(this[searchParams], name, value);
-    update(this[context], this);
+    if (this[context]) {
+      this[context].search = this.toString();
+    }
   }
 
   delete(name) {
@@ -311,7 +310,9 @@ class URLSearchParams {
         i += 2;
       }
     }
-    update(this[context], this);
+    if (this[context]) {
+      this[context].search = this.toString();
+    }
   }
 
   get(name) {
@@ -406,7 +407,9 @@ class URLSearchParams {
       ArrayPrototypePush(list, name, value);
     }
 
-    update(this[context], this);
+    if (this[context]) {
+      this[context].search = this.toString();
+    }
   }
 
   sort() {
@@ -450,7 +453,9 @@ class URLSearchParams {
       }
     }
 
-    update(this[context], this);
+    if (this[context]) {
+      this[context].search = this.toString();
+    }
   }
 
   // https://heycam.github.io/webidl/#es-iterators
@@ -536,46 +541,6 @@ function isURLThis(self) {
   return self != null && ObjectPrototypeHasOwnProperty(self, context);
 }
 
-function constructHref(ctx, options) {
-  if (options)
-    validateObject(options, 'options');
-
-  options = {
-    fragment: true,
-    unicode: false,
-    search: true,
-    auth: true,
-    ...options,
-  };
-
-  // https://url.spec.whatwg.org/#url-serializing
-  let ret = ctx.protocol;
-  if (ctx.hasHost) {
-    ret += '//';
-    const hasUsername = ctx.username !== '';
-    const hasPassword = ctx.password !== '';
-    if (options.auth && (hasUsername || hasPassword)) {
-      if (hasUsername)
-        ret += ctx.username;
-      if (hasPassword)
-        ret += `:${ctx.password}`;
-      ret += '@';
-    }
-    ret += options.unicode ?
-      domainToUnicode(ctx.hostname) : ctx.hostname;
-    if (ctx.port !== '')
-      ret += `:${ctx.port}`;
-  } else if (!ctx.hasOpaquePath && ctx.pathname.lastIndexOf('/') !== 0 && ctx.pathname.startsWith('//')) {
-    ret += '/.';
-  }
-  ret += ctx.pathname;
-  if (options.search)
-    ret += ctx.search;
-  if (options.fragment)
-    ret += ctx.hash;
-  return ret;
-}
-
 class URL {
   constructor(input, base = undefined) {
     // toUSVString is not needed.
@@ -628,14 +593,8 @@ class URL {
     return `${constructor.name} ${inspect(obj, opts)}`;
   }
 
-  [kFormat](options) {
-    // TODO(@anonrig): Replace kFormat with actually calling setters.
-    return constructHref(this[context], options);
-  }
-
   #onParseComplete = (href, origin, protocol, hostname, pathname,
-                      search, username, password, port, hash, hasHost,
-                      hasOpaquePath) => {
+                      search, username, password, port, hash) => {
     const ctx = this[context];
     ctx.href = href;
     ctx.origin = origin;
@@ -647,9 +606,6 @@ class URL {
     ctx.password = password;
     ctx.port = port;
     ctx.hash = hash;
-    // TODO(@anonrig): Remove hasHost and hasOpaquePath when kFormat is removed.
-    ctx.hasHost = hasHost;
-    ctx.hasOpaquePath = hasOpaquePath;
     if (!this[searchParams]) { // Invoked from URL constructor
       this[searchParams] = new URLSearchParams();
       this[searchParams][context] = this;
@@ -862,33 +818,6 @@ ObjectDefineProperties(URL, {
   revokeObjectURL: kEnumerableProperty,
 });
 
-function update(url, params) {
-  if (!url)
-    return;
-
-  const ctx = url[context];
-  const serializedParams = params.toString();
-  if (serializedParams.length > 0) {
-    ctx.search = '?' + serializedParams;
-  } else {
-    ctx.search = '';
-
-    // Potentially strip trailing spaces from an opaque path
-    if (ctx.hasOpaquePath && ctx.hash.length === 0) {
-      let length = ctx.pathname.length;
-      while (length > 0 && ctx.pathname.charCodeAt(length - 1) === 32) {
-        length--;
-      }
-
-      // No need to copy the whole string if there is no space at the end
-      if (length !== ctx.pathname.length) {
-        ctx.pathname = ctx.pathname.slice(0, length);
-      }
-    }
-  }
-  ctx.href = constructHref(ctx);
-}
-
 function initSearchParams(url, init) {
   if (!init) {
     url[searchParams] = [];
@@ -1387,7 +1316,6 @@ module.exports = {
   domainToASCII,
   domainToUnicode,
   urlToHttpOptions,
-  formatSymbol: kFormat,
   searchParamsSymbol: searchParams,
   encodeStr,
 };
diff --git a/lib/url.js b/lib/url.js
@@ -22,6 +22,7 @@
 'use strict';
 
 const {
+  Boolean,
   Int8Array,
   ObjectCreate,
   ObjectKeys,
@@ -38,7 +39,10 @@ const {
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_URL,
 } = require('internal/errors').codes;
-const { validateString } = require('internal/validators');
+const {
+  validateString,
+  validateObject,
+} = require('internal/validators');
 
 // This ensures setURLConstructor() is called before the native
 // URL::ToObject() method is used.
@@ -51,11 +55,14 @@ const {
   domainToASCII,
   domainToUnicode,
   fileURLToPath,
-  formatSymbol,
   pathToFileURL,
   urlToHttpOptions,
 } = require('internal/url');
 
+const {
+  formatUrl,
+} = internalBinding('url');
+
 // Original url.parse() API
 
 function Url() {
@@ -579,13 +586,36 @@ function urlFormat(urlObject, options) {
   } else if (typeof urlObject !== 'object' || urlObject === null) {
     throw new ERR_INVALID_ARG_TYPE('urlObject',
                                    ['Object', 'string'], urlObject);
-  } else if (!(urlObject instanceof Url)) {
-    const format = urlObject[formatSymbol];
-    return format ?
-      format.call(urlObject, options) :
-      Url.prototype.format.call(urlObject);
+  } else if (urlObject instanceof URL) {
+    let fragment = true;
+    let unicode = false;
+    let search = true;
+    let auth = true;
+
+    if (options) {
+      validateObject(options, 'options');
+
+      if (options.fragment != null) {
+        fragment = Boolean(options.fragment);
+      }
+
+      if (options.unicode != null) {
+        unicode = Boolean(options.unicode);
+      }
+
+      if (options.search != null) {
+        search = Boolean(options.search);
+      }
+
+      if (options.auth != null) {
+        auth = Boolean(options.auth);
+      }
+    }
+
+    return formatUrl(urlObject.href, fragment, unicode, search, auth);
   }
-  return urlObject.format();
+
+  return Url.prototype.format.call(urlObject);
 }
 
 // These characters do not need escaping:
diff --git a/src/node_url.cc b/src/node_url.cc
@@ -11,7 +11,6 @@
 
 namespace node {
 
-using v8::Boolean;
 using v8::Context;
 using v8::Function;
 using v8::FunctionCallbackInfo;
@@ -47,7 +46,7 @@ enum url_update_action {
   kHref = 9,
 };
 
-void SetArgs(Environment* env, Local<Value> argv[12], const ada::result& url) {
+void SetArgs(Environment* env, Local<Value> argv[10], const ada::result& url) {
   Isolate* isolate = env->isolate();
   argv[0] = Utf8String(isolate, url->get_href());
   argv[1] = Utf8String(isolate, url->get_origin());
@@ -59,8 +58,6 @@ void SetArgs(Environment* env, Local<Value> argv[12], const ada::result& url) {
   argv[7] = Utf8String(isolate, url->get_password());
   argv[8] = Utf8String(isolate, url->get_port());
   argv[9] = Utf8String(isolate, url->get_hash());
-  argv[10] = Boolean::New(isolate, url->host.has_value());
-  argv[11] = Boolean::New(isolate, url->has_opaque_path);
 }
 
 void Parse(const FunctionCallbackInfo<Value>& args) {
@@ -86,8 +83,7 @@ void Parse(const FunctionCallbackInfo<Value>& args) {
     }
     base_pointer = &base.value();
   }
-  ada::result out =
-      ada::parse(std::string_view(input.out(), input.length()), base_pointer);
+  ada::result out = ada::parse(input.ToStringView(), base_pointer);
 
   if (!out) {
     return args.GetReturnValue().Set(false);
@@ -105,8 +101,6 @@ void Parse(const FunctionCallbackInfo<Value>& args) {
       undef,
       undef,
       undef,
-      undef,
-      undef,
   };
   SetArgs(env, argv, out);
   USE(success_callback_->Call(
@@ -192,10 +186,8 @@ void UpdateUrl(const FunctionCallbackInfo<Value>& args) {
   Utf8Value new_value(isolate, args[2].As<String>());
   Local<Function> success_callback_ = args[3].As<Function>();
 
-  std::string_view new_value_view =
-      std::string_view(new_value.out(), new_value.length());
-  std::string_view input_view = std::string_view(input.out(), input.length());
-  ada::result out = ada::parse(input_view);
+  std::string_view new_value_view = new_value.ToStringView();
+  ada::result out = ada::parse(input.ToStringView());
   CHECK(out);
 
   bool result{true};
@@ -255,21 +247,73 @@ void UpdateUrl(const FunctionCallbackInfo<Value>& args) {
       undef,
       undef,
       undef,
-      undef,
-      undef,
   };
   SetArgs(env, argv, out);
   USE(success_callback_->Call(
       env->context(), args.This(), arraysize(argv), argv));
   args.GetReturnValue().Set(result);
 }
 
+void FormatUrl(const FunctionCallbackInfo<Value>& args) {
+  CHECK_GT(args.Length(), 4);
+  CHECK(args[0]->IsString());  // url href
+
+  Environment* env = Environment::GetCurrent(args);
+  Isolate* isolate = env->isolate();
+
+  Utf8Value href(isolate, args[0].As<String>());
+  const bool fragment = args[1]->IsTrue();
+  const bool unicode = args[2]->IsTrue();
+  const bool search = args[3]->IsTrue();
+  const bool auth = args[4]->IsTrue();
+
+  ada::result out = ada::parse(href.ToStringView());
+  CHECK(out);
+
+  if (!fragment) {
+    out->fragment = std::nullopt;
+  }
+
+  if (unicode) {
+#if defined(NODE_HAVE_I18N_SUPPORT)
+    std::string hostname = out->get_hostname();
+    MaybeStackBuffer<char> buf;
+    int32_t len = i18n::ToUnicode(&buf, hostname.data(), hostname.length());
+
+    if (len < 0) {
+      out->host = "";
+    } else {
+      out->host = buf.ToString();
+    }
+#else
+    out->host = "";
+#endif
+  }
+
+  if (!search) {
+    out->query = std::nullopt;
+  }
+
+  if (!auth) {
+    out->username = "";
+    out->password = "";
+  }
+
+  std::string result = out->get_href();
+  args.GetReturnValue().Set(String::NewFromUtf8(env->isolate(),
+                                                result.data(),
+                                                NewStringType::kNormal,
+                                                result.length())
+                                .ToLocalChecked());
+}
+
 void Initialize(Local<Object> target,
                 Local<Value> unused,
                 Local<Context> context,
                 void* priv) {
   SetMethod(context, target, "parse", Parse);
   SetMethod(context, target, "updateUrl", UpdateUrl);
+  SetMethod(context, target, "formatUrl", FormatUrl);
 
   SetMethodNoSideEffect(context, target, "domainToASCII", DomainToASCII);
   SetMethodNoSideEffect(context, target, "domainToUnicode", DomainToUnicode);
@@ -279,6 +323,7 @@ void Initialize(Local<Object> target,
 void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(Parse);
   registry->Register(UpdateUrl);
+  registry->Register(FormatUrl);
 
   registry->Register(DomainToASCII);
   registry->Register(DomainToUnicode);
diff --git a/test/parallel/test-whatwg-url-custom-inspect.js b/test/parallel/test-whatwg-url-custom-inspect.js
