Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Query string params #1449

Merged
merged 37 commits into from May 19, 2022
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
2b56a54
feat: redirect
ronag Mar 22, 2021
7607b90
fix linting
ronag Apr 27, 2022
a4d185e
fix: linting
ronag Apr 27, 2022
176cce2
Implement support for querystring params
kibertoad May 18, 2022
def801f
Merge branch 'main' of https://github.com/nodejs/undici into feat/144…
kibertoad May 18, 2022
bf7433a
Remove leaked change
kibertoad May 18, 2022
433a455
Fix test
kibertoad May 18, 2022
5afcdc8
Address code review comments
kibertoad May 18, 2022
05bc042
Address linting errors
kibertoad May 18, 2022
52089aa
Improve coverage
kibertoad May 18, 2022
a89e7aa
Comment out new tests to see if they are causing problems in CI
kibertoad May 18, 2022
06a0093
Improve coverage
kibertoad May 18, 2022
18744d4
Fix linting
kibertoad May 18, 2022
3fdd223
Adjust two other tests too
kibertoad May 18, 2022
8f6f998
Remove excessive condition
kibertoad May 18, 2022
4c711c6
Fix linting
kibertoad May 18, 2022
d270947
Merge remote-tracking branch 'origin/feat/1448-params' into feat/1448…
kibertoad May 18, 2022
5d35025
Handle additional use-cases
kibertoad May 18, 2022
babcf18
Simplify
kibertoad May 18, 2022
2eb2797
Try to fix tests in CI
kibertoad May 18, 2022
a471788
Make code more consistent
kibertoad May 18, 2022
7bb502a
Address code review comments
kibertoad May 19, 2022
edd2272
Fix linting
kibertoad May 19, 2022
fb9f354
Cleanup
kibertoad May 19, 2022
ad84f02
Cleanup and improve coverage
kibertoad May 19, 2022
86014dd
Restore encoding with explanation
kibertoad May 19, 2022
5e0bf8b
Fix linting
kibertoad May 19, 2022
ffc6db2
Fix docs
kibertoad May 19, 2022
54c97e5
Reduce amount of unescaped characters
kibertoad May 19, 2022
6f3802e
Simplify branch
kibertoad May 19, 2022
c3f71f9
Fix symbol
kibertoad May 19, 2022
63e7f07
Explicitly do not support #
kibertoad May 19, 2022
5dba6b7
Adjust based on latest comments
kibertoad May 19, 2022
8d2c7d4
Fix linting
kibertoad May 19, 2022
ed81bbb
Address code review comments
kibertoad May 19, 2022
a454d00
Fix linting
kibertoad May 19, 2022
79610dd
Clarify documentation
kibertoad May 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/api/Dispatcher.md
Expand Up @@ -194,6 +194,7 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo
* **method** `string`
* **body** `string | Buffer | Uint8Array | stream.Readable | Iterable | AsyncIterable | null` (optional) - Default: `null`
* **headers** `UndiciHeaders | string[]` (optional) - Default: `null`.
* **query** `Record<string, any> | null` (optional) - Default: `null` - Query string params to be embedded in the request URL.
* **idempotent** `boolean` (optional) - Default: `true` if `method` is `'HEAD'` or `'GET'` - Whether the requests can be safely retried or not. If `false` the request won't be sent until all preceding requests in the pipeline has completed.
* **blocking** `boolean` (optional) - Default: `false` - Whether the response is expected to take a long time and would end up blocking the pipeline. When this is set to `true` further pipelining will be avoided on the same connection until headers have been received.
* **upgrade** `string | null` (optional) - Default: `null` - Upgrade the request. Should be used to specify the kind of upgrade i.e. `'Websocket'`.
Expand Down
5 changes: 3 additions & 2 deletions lib/core/request.js
Expand Up @@ -4,8 +4,8 @@ const {
InvalidArgumentError,
NotSupportedError
} = require('./errors')
const util = require('./util')
const assert = require('assert')
const util = require('./util')

const kHandler = Symbol('handler')

Expand Down Expand Up @@ -38,6 +38,7 @@ class Request {
method,
body,
headers,
query,
idempotent,
blocking,
upgrade,
Expand Down Expand Up @@ -97,7 +98,7 @@ class Request {

this.upgrade = upgrade || null

this.path = path
this.path = query ? util.buildURL(path, query) : path

this.origin = origin

Expand Down
56 changes: 55 additions & 1 deletion lib/core/util.js
Expand Up @@ -26,6 +26,59 @@ function isBlobLike (object) {
)
}

function isDate (val) {
return toString.call(val) === '[object Date]'
}

function isObject (val) {
return val !== null && typeof val === 'object'
}

// this escapes all non-uri friendly characters, but restores : which is commonly used for date formatting
function encode (val) {
return encodeURIComponent(val).replace(/%3A/gi, ':')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why restore :? I don't understand the use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escaping : is going to mess up date format and complicate rather frequent use-case.
I'm having second thoughts about escaping anything, TBH, especially since Undici is not supposed to be used from browser. What do you think about removing escaping completely and let user do it if they need to?

Copy link
Member

@ronag ronag May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does got do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we actually need to escape or we might end up with invalid URLs, e.g. if they contain ?, & etc...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think encodeURIComponent is sufficient for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got is using URLSearchParams, and there are already issues with that: sindresorhus/got#1180
If we go with escaping, I would suggest following axios example instead of got - they were around for much longer, and had all the time in the world to work out the kinks of real world usage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the use case. Is it that some server implementations don't decode query parameters at all?

Copy link
Member

@ronag ronag May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if we just keep it simple and use encodeURIComponent. Anything else feels opinionated to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
ronag marked this conversation as resolved.
Show resolved Hide resolved

// based on https://github.com/axios/axios/blob/63e559fa609c40a0a460ae5d5a18c3470ffc6c9e/lib/helpers/buildURL.js (MIT license)
function buildURL (url, queryParams) {
if (url.includes('?')) {
throw new Error('Query params cannot be passed when url contains "?" already.')
}
kibertoad marked this conversation as resolved.
Show resolved Hide resolved

ronag marked this conversation as resolved.
Show resolved Hide resolved
const parts = []
for (let [key, val] of Object.entries(queryParams)) {
if (val === null || typeof val === 'undefined') {
continue
}

if (!Array.isArray(val)) {
val = [val]
}

for (let v of val) {
if (isDate(v)) {
v = v.toISOString()
kibertoad marked this conversation as resolved.
Show resolved Hide resolved
} else 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('&')

if (serializedParams) {
const hashmarkIndex = url.indexOf('#')
if (hashmarkIndex !== -1) {
url = url.slice(0, hashmarkIndex)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit magical...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can drop it, if you like

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throw if #?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not resolved yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, change was done and test was added, but some garbage left behind

}

url += '&' + serializedParams
kibertoad marked this conversation as resolved.
Show resolved Hide resolved
}

return url
}
ronag marked this conversation as resolved.
Show resolved Hide resolved

function parseURL (url) {
if (typeof url === 'string') {
url = new URL(url)
Expand Down Expand Up @@ -357,5 +410,6 @@ module.exports = {
isBuffer,
validateHandler,
getSocketInfo,
isFormDataLike
isFormDataLike,
buildURL
}
219 changes: 216 additions & 3 deletions test/client.js
@@ -1,10 +1,10 @@
'use strict'

const { test } = require('tap')
const { Client, errors } = require('..')
const { createServer } = require('http')
const { readFileSync, createReadStream } = require('fs')
const { createServer } = require('http')
const { Readable } = require('stream')
const { test } = require('tap')
const { Client, errors } = require('..')
const { kSocket } = require('../lib/core/symbols')
const { wrapWithAsyncIterable } = require('./utils/async-iterators')
const EE = require('events')
Expand Down Expand Up @@ -80,6 +80,196 @@ test('basic get', (t) => {
})
})

test('basic get with query params', (t) => {
t.plan(4)

const date = new Date(1995, 11, 17)
const server = createServer((req, res) => {
const searchParamsObject = buildParams(req.url)
t.strictSame(searchParamsObject, {
date: date.toISOString(),
bool: 'true',
foo: '1',
bar: 'bar',
':%24%2C%2B%5B%5D%40%5E*()-': ':%24%2C%2B%5B%5D%40%5E*()-',
multi: ['1', '2']
})

res.statusCode = 200
res.end('hello')
})
t.teardown(server.close.bind(server))

const query = {
date,
bool: true,
foo: 1,
bar: 'bar',
nullVal: null,
undefinedVal: undefined,
':$,+[]@^*()-': ':$,+[]@^*()-',
multi: [1, 2]
}

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.error(err)
const { statusCode } = data
t.equal(statusCode, 200)
})
t.equal(signal.listenerCount('abort'), 1)
})
})

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 query params ignores hashmark', (t) => {
t.plan(4)

const server = createServer((req, res) => {
const searchParamsObject = buildParams(req.url)
t.strictSame(searchParamsObject, {
foo: '1',
bar: 'bar',
multi: ['1', '2']
})

res.statusCode = 200
res.end('hello')
})
t.teardown(server.close.bind(server))

const query = {
foo: 1,
bar: 'bar',
multi: [1, 2]
}

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.error(err)
const { statusCode } = data
t.equal(statusCode, 200)
})
t.equal(signal.listenerCount('abort'), 1)
})
})

test('basic get with empty query params', (t) => {
t.plan(4)

const server = createServer((req, res) => {
const searchParamsObject = buildParams(req.url)
t.strictSame(searchParamsObject, {})

res.statusCode = 200
res.end('hello')
})
t.teardown(server.close.bind(server))

const query = {}

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.error(err)
const { statusCode } = data
t.equal(statusCode, 200)
})
t.equal(signal.listenerCount('abort'), 1)
})
})

test('basic get with query params partially in path', (t) => {
t.plan(1)

const server = createServer((req, res) => {
t.fail()
})
t.teardown(server.close.bind(server))

const query = {
foo: 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: '/?bar=2',
method: 'GET',
query
}, (err, data) => {
t.equal(err.message, 'Query params cannot be passed when url contains "?" already.')
})
})
})

test('basic head', (t) => {
t.plan(14)

Expand Down Expand Up @@ -1589,3 +1779,26 @@ test('async iterator yield object error', (t) => {
})
})
})

function buildParams (path) {
const cleanPath = path.replace('/?', '').replace('/', '').split('&')
const builtParams = cleanPath.reduce((acc, entry) => {
const [key, value] = entry.split('=')
if (key.length === 0) {
return acc
}

if (acc[key]) {
if (Array.isArray(acc[key])) {
acc[key].push(value)
} else {
acc[key] = [acc[key], value]
}
} else {
acc[key] = value
}
return acc
}, {})

return builtParams
}
2 changes: 2 additions & 0 deletions types/dispatcher.d.ts
Expand Up @@ -47,6 +47,8 @@ declare namespace Dispatcher {
body?: string | Buffer | Uint8Array | Readable | null | FormData;
/** Default: `null` */
headers?: IncomingHttpHeaders | string[] | null;
/** Query string params to be embedded in the request URL. Default: `null` */
query?: Record<string, any>;
/** Whether the requests can be safely retried or not. If `false` the request won't be sent until all preceding requests in the pipeline have completed. Default: `true` if `method` is `HEAD` or `GET`. */
idempotent?: boolean;
/** Upgrade the request. Should be used to specify the kind of upgrade i.e. `'Websocket'`. Default: `method === 'CONNECT' || null`. */
Expand Down