From 429db406c6e55b0deac254004682cdddabfcddad Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 26 Apr 2020 16:03:57 +0200 Subject: [PATCH] Fix `options.searchParams` duplicates Fixes #1188 --- source/core/index.ts | 82 +++++++++++++++++++++++++++----------------- test/arguments.ts | 12 +++++++ test/error.ts | 2 +- 3 files changed, 63 insertions(+), 33 deletions(-) diff --git a/source/core/index.ts b/source/core/index.ts index 55549c07e..d860931a9 100644 --- a/source/core/index.ts +++ b/source/core/index.ts @@ -560,21 +560,37 @@ export default class Request extends Duplex implements RequestEvents { static normalizeArguments(url?: string | URL, options?: Options, defaults?: Defaults): NormalizedOptions { const rawOptions = options; + const searchParameters = options?.searchParams; + const hooks = options?.hooks; if (is.object(url) && !is.urlInstance(url)) { - options = {...defaults as NormalizedOptions, ...(url as Options), ...options}; + options = {...defaults, ...(url as Options), ...options}; } else { if (url && options && options.url) { throw new TypeError('The `url` option is mutually exclusive with the `input` argument'); } - options = {...defaults as NormalizedOptions, ...options}; + options = {...defaults, ...options}; if (url) { options.url = url; } } + // Prevent duplicating default search params & hooks + if (searchParameters === undefined) { + delete options.searchParams; + } else { + options.searchParams = searchParameters; + } + + if (hooks === undefined) { + delete options.hooks; + } else { + options.hooks = hooks; + } + + // Setting options to `undefined` turns off its functionalities if (rawOptions && defaults) { for (const key in rawOptions) { // @ts-ignore Dear TypeScript, all object keys are strings (or symbols which are NOT enumerable). @@ -637,6 +653,28 @@ export default class Request extends Duplex implements RequestEvents { throw new TypeError('Parameter `auth` is deprecated. Use `username` / `password` instead.'); } + // `options.searchParams` + if (options.searchParams) { + if (!is.string(options.searchParams) && !(options.searchParams instanceof URLSearchParams)) { + validateSearchParameters(options.searchParams); + } + + options.searchParams = new URLSearchParams(options.searchParams as Record); + + // `normalizeArguments()` is also used to merge options + if (defaults?.searchParams) { + defaults.searchParams.forEach((value, key) => { + (options!.searchParams as URLSearchParams).append(key, value); + }); + } + } else { + options.searchParams = defaults?.searchParams; + } + + // `options.username` & `options.password` + options.username = options.username ?? ''; + options.password = options.password ?? ''; + // `options.prefixUrl` & `options.url` if (options.prefixUrl) { options.prefixUrl = options.prefixUrl.toString(); @@ -675,7 +713,7 @@ export default class Request extends Duplex implements RequestEvents { get: () => prefixUrl }); - // Protocol check + // Support UNIX sockets let {protocol} = options.url; if (protocol === 'unix:') { @@ -684,6 +722,12 @@ export default class Request extends Duplex implements RequestEvents { options.url = new URL(`http://unix${options.url.pathname}${options.url.search}`); } + // Set search params + if (options.searchParams) { + options.url.search = options.searchParams.toString(); + } + + // Trigger search params normalization if (options.url.search) { const triggerSearchParameters = '_GOT_INTERNAL_TRIGGER_NORMALIZATION'; @@ -691,16 +735,12 @@ export default class Request extends Duplex implements RequestEvents { options.url.searchParams.delete(triggerSearchParameters); } + // Protocol check if (protocol !== 'http:' && protocol !== 'https:') { throw new UnsupportedProtocolError(options as NormalizedOptions); } - } - // `options.username` & `options.password` - options.username = options.username ?? ''; - options.password = options.password ?? ''; - - if (options.url) { + // Update `username` & `password` options.url.username = options.username; options.url.password = options.password; } @@ -725,27 +765,6 @@ export default class Request extends Duplex implements RequestEvents { } } - // `options.searchParams` - if (options.searchParams) { - if (!is.string(options.searchParams) && !(options.searchParams instanceof URLSearchParams)) { - validateSearchParameters(options.searchParams); - } - - options.searchParams = new URLSearchParams(options.searchParams as Record); - - // `normalizeArguments()` is also used to merge options - const defaultsAsOptions = defaults as Options | undefined; - if (defaultsAsOptions && defaultsAsOptions.searchParams instanceof URLSearchParams) { - defaultsAsOptions.searchParams.forEach((value, key) => { - (options!.searchParams as URLSearchParams).append(key, value); - }); - } - - if (options.url) { - options.url.search = options.searchParams.toString(); - } - } - // `options.cache` const {cache} = options; if (cache) { @@ -782,7 +801,6 @@ export default class Request extends Duplex implements RequestEvents { } // `options.hooks` - const areHooksUserDefined = options.hooks !== defaults?.hooks; options.hooks = {...options.hooks}; for (const event of knownHookEvents) { @@ -798,7 +816,7 @@ export default class Request extends Duplex implements RequestEvents { } } - if (defaults && areHooksUserDefined) { + if (defaults) { for (const event of knownHookEvents) { const defaultHooks = defaults.hooks[event]; diff --git a/test/arguments.ts b/test/arguments.ts index 8bcaf5231..c45803832 100644 --- a/test/arguments.ts +++ b/test/arguments.ts @@ -138,6 +138,18 @@ test('overrides `searchParams` from options', withServer, async (t, server, got) t.is(body, '/?test=wow'); }); +test('does not duplicate `searchParams`', withServer, async (t, server, got) => { + server.get('/', echoUrl); + + const instance = got.extend({ + searchParams: new URLSearchParams({foo: '123'}) + }); + + const body = await instance('?bar=456').text(); + + t.is(body, '/?foo=123'); +}); + test('escapes `searchParams` parameter values', withServer, async (t, server, got) => { server.get('/', echoUrl); diff --git a/test/error.ts b/test/error.ts index a134f9098..a9b5fc0a9 100644 --- a/test/error.ts +++ b/test/error.ts @@ -178,7 +178,7 @@ test('`http.request` error through CacheableRequest', async t => { }); }); -test('errors are thrown directly when options.stream is true', t => { +test('errors are thrown directly when options.isStream is true', t => { t.throws(() => { // @ts-ignore Error tests got('https://example.com', {isStream: true, hooks: false});