From bc636c553461c8cac86f6b4835fac64f47e33ee2 Mon Sep 17 00:00:00 2001
From: Khafra <42794878+KhafraDev@users.noreply.github.com>
Date: Mon, 3 Oct 2022 11:24:53 -0400
Subject: [PATCH] feat(util): use node:querystring for buildURL (#1677)

* feat(util): use node:querystring for buildURL

* fix: skip test on v12

* fix: handle case for empty object
---
 lib/core/util.js | 38 +++-----------------
 test/client.js   | 93 ++----------------------------------------------
 test/util.js     | 25 +++++++++++++
 3 files changed, 32 insertions(+), 124 deletions(-)

diff --git a/lib/core/util.js b/lib/core/util.js
index e9a8384ced8..c2dcf79fb80 100644
--- a/lib/core/util.js
+++ b/lib/core/util.js
@@ -8,6 +8,7 @@ const net = require('net')
 const { InvalidArgumentError } = require('./errors')
 const { Blob } = require('buffer')
 const nodeUtil = require('util')
+const { stringify } = require('querystring')
 
 function nop () {}
 
@@ -26,46 +27,15 @@ function isBlobLike (object) {
   )
 }
 
-function isObject (val) {
-  return val !== null && typeof val === 'object'
-}
-
-// this escapes all non-uri friendly characters
-function encode (val) {
-  return encodeURIComponent(val)
-}
-
-// based on https://github.com/axios/axios/blob/63e559fa609c40a0a460ae5d5a18c3470ffc6c9e/lib/helpers/buildURL.js (MIT license)
 function buildURL (url, queryParams) {
   if (url.includes('?') || url.includes('#')) {
     throw new Error('Query params cannot be passed when url already contains "?" or "#".')
   }
-  if (!isObject(queryParams)) {
-    throw new Error('Query params must be an object')
-  }
-
-  const parts = []
-  for (let [key, val] of Object.entries(queryParams)) {
-    if (val === null || typeof val === 'undefined') {
-      continue
-    }
-
-    if (!Array.isArray(val)) {
-      val = [val]
-    }
-
-    for (const v of val) {
-      if (isObject(v)) {
-        throw new Error('Passing object as a query param is not supported, please serialize to string up-front')
-      }
-      parts.push(encode(key) + '=' + encode(v))
-    }
-  }
 
-  const serializedParams = parts.join('&')
+  const stringified = stringify(queryParams)
 
-  if (serializedParams) {
-    url += '?' + serializedParams
+  if (stringified) {
+    url += '?' + stringified
   }
 
   return url
diff --git a/test/client.js b/test/client.js
index 645ff50450f..1a97775595e 100644
--- a/test/client.js
+++ b/test/client.js
@@ -98,7 +98,9 @@ test('basic get with query params', (t) => {
       foo: '1',
       bar: 'bar',
       '%60~%3A%24%2C%2B%5B%5D%40%5E*()-': '%60~%3A%24%2C%2B%5B%5D%40%5E*()-',
-      multi: ['1', '2']
+      multi: ['1', '2'],
+      nullVal: '',
+      undefinedVal: ''
     })
 
     res.statusCode = 200
@@ -137,95 +139,6 @@ test('basic get with query params', (t) => {
   })
 })
 
-test('basic get with query params with object throws an error', (t) => {
-  t.plan(1)
-
-  const server = createServer((req, res) => {
-    t.fail()
-  })
-  t.teardown(server.close.bind(server))
-
-  const query = {
-    obj: { id: 1 }
-  }
-
-  server.listen(0, () => {
-    const client = new Client(`http://localhost:${server.address().port}`, {
-      keepAliveTimeout: 300e3
-    })
-    t.teardown(client.close.bind(client))
-
-    const signal = new EE()
-    client.request({
-      signal,
-      path: '/',
-      method: 'GET',
-      query
-    }, (err, data) => {
-      t.equal(err.message, 'Passing object as a query param is not supported, please serialize to string up-front')
-    })
-  })
-})
-
-test('basic get with non-object query params throws an error', (t) => {
-  t.plan(1)
-
-  const server = createServer((req, res) => {
-    t.fail()
-  })
-  t.teardown(server.close.bind(server))
-
-  const query = '{ obj: { id: 1 } }'
-
-  server.listen(0, () => {
-    const client = new Client(`http://localhost:${server.address().port}`, {
-      keepAliveTimeout: 300e3
-    })
-    t.teardown(client.close.bind(client))
-
-    const signal = new EE()
-    client.request({
-      signal,
-      path: '/',
-      method: 'GET',
-      query
-    }, (err, data) => {
-      t.equal(err.message, 'Query params must be an object')
-    })
-  })
-})
-
-test('basic get with query params with date throws an error', (t) => {
-  t.plan(1)
-
-  const date = new Date()
-  const server = createServer((req, res) => {
-    t.fail()
-  })
-  t.teardown(server.close.bind(server))
-
-  const query = {
-    dateObj: date
-  }
-
-  server.listen(0, () => {
-    const client = new Client(`http://localhost:${server.address().port}`, {
-      keepAliveTimeout: 300e3
-    })
-    t.teardown(client.close.bind(client))
-
-    const signal = new EE()
-    client.request({
-      signal,
-      path: '/',
-      method: 'GET',
-      query
-    }, (err, data) => {
-      t.equal(err.message, 'Passing object as a query param is not supported, please serialize to string up-front')
-    })
-  })
-})
-
 test('basic get with query params fails if url includes hashmark', (t) => {
   t.plan(1)
 
diff --git a/test/util.js b/test/util.js
index 2b9dd703b1f..cbc94d63ba0 100644
--- a/test/util.js
+++ b/test/util.js
@@ -95,3 +95,28 @@ test('parseRawHeaders', (t) => {
   t.plan(1)
   t.same(util.parseRawHeaders(['key', 'value', Buffer.from('key'), Buffer.from('value')]), ['key', 'value', 'key', 'value'])
 })
+
+test('buildURL', { skip: process.version.startsWith('v12.') }, (t) => {
+  const tests = [
+    [{ id: BigInt(123456) }, 'id=123456'],
+    [{ date: new Date() }, 'date='],
+    [{ obj: { id: 1 } }, 'obj='],
+    [{ params: ['a', 'b', 'c'] }, 'params=a&params=b&params=c'],
+    [{ bool: true }, 'bool=true'],
+    [{ number: 123456 }, 'number=123456'],
+    [{ string: 'hello' }, 'string=hello'],
+    [{ null: null }, 'null='],
+    [{ void: undefined }, 'void='],
+    [{ fn: function () {} }, 'fn='],
+    [{}, '']
+  ]
+
+  const base = 'https://www.google.com'
+
+  for (const [input, output] of tests) {
+    const expected = `${base}${output ? `?${output}` : output}`
+    t.equal(util.buildURL(base, input), expected)
+  }
+
+  t.end()
+})