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: add new dispatch compose #2826

Merged
merged 25 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
560d35b
feat: add new dispatch compose
metcoder95 Feb 23, 2024
5337ea0
fix: review
metcoder95 Feb 25, 2024
8019e59
revert: linting
metcoder95 Feb 25, 2024
1d54170
docs: add documentation
metcoder95 Feb 25, 2024
051caa1
Merge remote-tracking branch 'origin/main' into feat/compose-dispatch
metcoder95 Feb 25, 2024
b45ecad
fix: smaller tweaks to proxy interceptor
metcoder95 Feb 28, 2024
9048eb5
test: fix tests for proxy
metcoder95 Feb 28, 2024
7656e1b
refactor: expose interceptor as is
metcoder95 Feb 29, 2024
9f0631e
Merge branch 'main' into feat/compose-dispatch
metcoder95 Feb 29, 2024
5588a1d
test: add testing for retry
metcoder95 Mar 1, 2024
04c8acb
refactor: rewrite interceptors
metcoder95 Mar 3, 2024
8ac252d
refactor: proxy interceptor
metcoder95 Mar 3, 2024
8c8c064
feat: redirect interceptor
metcoder95 Mar 3, 2024
9d1a6c0
Merge branch 'main' into feat/compose-dispatch
metcoder95 Mar 3, 2024
0fd9ea7
refactor: change the compose behaviour
metcoder95 Mar 6, 2024
53f4cfa
docs: update docs
metcoder95 Mar 6, 2024
6580d84
test: add testing for compose
metcoder95 Mar 6, 2024
1d987a2
Merge branch 'main' into feat/compose-dispatch
metcoder95 Mar 6, 2024
d66530f
feat: composed dispatcher
metcoder95 Mar 8, 2024
b932ad2
docs: adjust documentation
metcoder95 Mar 8, 2024
20d3a33
refactor: apply review
metcoder95 Mar 8, 2024
2d59acc
Merge branch 'main' into feat/compose-dispatch
metcoder95 Mar 8, 2024
2f5982e
docs: tweaks
metcoder95 Mar 8, 2024
eb065f7
Merge branch 'main' into feat/compose-dispatch
metcoder95 Mar 12, 2024
620f9bc
feat: drop proxy
metcoder95 Mar 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const MockClient = require('./lib/mock/mock-client')
const MockAgent = require('./lib/mock/mock-agent')
const MockPool = require('./lib/mock/mock-pool')
const mockErrors = require('./lib/mock/mock-errors')
const ProxyAgent = require('./lib/proxy-agent')
const Proxy = require('./lib/interceptor/proxy')
const RetryAgent = require('./lib/retry-agent')
const RetryHandler = require('./lib/handler/RetryHandler')
const { getGlobalDispatcher, setGlobalDispatcher } = require('./lib/global')
Expand All @@ -29,13 +29,18 @@ module.exports.Client = Client
module.exports.Pool = Pool
module.exports.BalancedPool = BalancedPool
module.exports.Agent = Agent
module.exports.ProxyAgent = ProxyAgent
module.exports.ProxyAgent = Proxy.ProxyAgent
module.exports.RetryAgent = RetryAgent
module.exports.RetryHandler = RetryHandler

module.exports.DecoratorHandler = DecoratorHandler
module.exports.RedirectHandler = RedirectHandler
module.exports.createRedirectInterceptor = createRedirectInterceptor
module.exports.interceptors = {
proxy: Proxy.interceptor,
redirect: require('./lib/interceptor/redirect'),
retry: require('./lib/interceptor/retry')
}

module.exports.buildConnector = buildConnector
module.exports.errors = errors
Expand Down
25 changes: 24 additions & 1 deletion lib/dispatcher.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
'use strict'

const EventEmitter = require('node:events')

const kDispatcherVersion = Symbol.for('undici.dispatcher.version')

class Dispatcher extends EventEmitter {
[kDispatcherVersion] = 1

dispatch () {
throw new Error('not implemented')
}
Expand All @@ -14,6 +17,26 @@ class Dispatcher extends EventEmitter {
destroy () {
throw new Error('not implemented')
}

compose (...interceptors) {
let dispatcher = this
for (const interceptor of interceptors) {
if (interceptor == null) {
continue
}

if (typeof interceptor !== 'function') {
throw new Error('invalid interceptor')
}

dispatcher = interceptor(dispatcher) ?? dispatcher

if (dispatcher[kDispatcherVersion] !== 1) {
throw new Error('invalid dispatcher')
}
}
return dispatcher
}
}

module.exports = Dispatcher
88 changes: 69 additions & 19 deletions lib/proxy-agent.js → lib/interceptor/proxy.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
'use strict'

const { kProxy, kClose, kDestroy, kInterceptors } = require('./core/symbols')
const { URL } = require('node:url')
const Agent = require('./agent')
const Pool = require('./pool')
const DispatcherBase = require('./dispatcher-base')
const { InvalidArgumentError, RequestAbortedError } = require('./core/errors')
const buildConnector = require('./core/connect')
const Agent = require('../agent')
const Pool = require('../pool')
const DispatcherBase = require('../dispatcher-base')
const { kProxy, kClose, kDestroy, kInterceptors } = require('../core/symbols')
const { InvalidArgumentError, RequestAbortedError } = require('../core/errors')
const buildConnector = require('../core/connect')

const kAgent = Symbol('proxy agent')
const kClient = Symbol('proxy client')
Expand All @@ -27,36 +27,47 @@ class ProxyAgent extends DispatcherBase {
constructor (opts) {
super()

if (!opts || (typeof opts === 'object' && !(opts instanceof URL) && !opts.uri)) {
if (
!opts ||
(typeof opts === 'object' && !(opts instanceof URL) && !opts.uri)
) {
throw new InvalidArgumentError('Proxy uri is mandatory')
}

const { clientFactory = defaultFactory } = opts
if (typeof clientFactory !== 'function') {
throw new InvalidArgumentError('Proxy opts.clientFactory must be a function.')
throw new InvalidArgumentError(
'Proxy opts.clientFactory must be a function.'
)
}

const url = this.#getUrl(opts)
const { href, origin, port, protocol, username, password } = url

this[kProxy] = { uri: href, protocol }
this[kAgent] = new Agent(opts)
this[kInterceptors] = opts.interceptors?.ProxyAgent && Array.isArray(opts.interceptors.ProxyAgent)
? opts.interceptors.ProxyAgent
: []
this[kInterceptors] =
opts.interceptors?.ProxyAgent &&
Array.isArray(opts.interceptors.ProxyAgent)
? opts.interceptors.ProxyAgent
: []
ronag marked this conversation as resolved.
Show resolved Hide resolved
this[kRequestTls] = opts.requestTls
this[kProxyTls] = opts.proxyTls
this[kProxyHeaders] = opts.headers || {}

if (opts.auth && opts.token) {
throw new InvalidArgumentError('opts.auth cannot be used in combination with opts.token')
throw new InvalidArgumentError(
'opts.auth cannot be used in combination with opts.token'
)
ronag marked this conversation as resolved.
Show resolved Hide resolved
} else if (opts.auth) {
/* @deprecated in favour of opts.token */
this[kProxyHeaders]['proxy-authorization'] = `Basic ${opts.auth}`
} else if (opts.token) {
this[kProxyHeaders]['proxy-authorization'] = opts.token
} else if (username && password) {
this[kProxyHeaders]['proxy-authorization'] = `Basic ${Buffer.from(`${decodeURIComponent(username)}:${decodeURIComponent(password)}`).toString('base64')}`
this[kProxyHeaders]['proxy-authorization'] = `Basic ${Buffer.from(
`${decodeURIComponent(username)}:${decodeURIComponent(password)}`
).toString('base64')}`
ronag marked this conversation as resolved.
Show resolved Hide resolved
}

const connect = buildConnector({ ...opts.proxyTls })
Expand All @@ -82,7 +93,11 @@ class ProxyAgent extends DispatcherBase {
})
if (statusCode !== 200) {
socket.on('error', () => {}).destroy()
callback(new RequestAbortedError(`Proxy response (${statusCode}) !== 200 when HTTP Tunneling`))
callback(
new RequestAbortedError(
`Proxy response (${statusCode}) !== 200 when HTTP Tunneling`
)
)
ronag marked this conversation as resolved.
Show resolved Hide resolved
}
if (opts.protocol !== 'https:') {
callback(null, socket)
Expand All @@ -94,7 +109,10 @@ class ProxyAgent extends DispatcherBase {
} else {
servername = opts.servername
}
this[kConnectEndpoint]({ ...opts, servername, httpSocket: socket }, callback)
this[kConnectEndpoint](
{ ...opts, servername, httpSocket: socket },
callback
)
ronag marked this conversation as resolved.
Show resolved Hide resolved
} catch (err) {
callback(err)
}
Expand Down Expand Up @@ -173,11 +191,43 @@ function buildHeaders (headers) {
* It should be removed in the next major version for performance reasons
*/
function throwIfProxyAuthIsSent (headers) {
const existProxyAuth = headers && Object.keys(headers)
.find((key) => key.toLowerCase() === 'proxy-authorization')
const existProxyAuth =
headers &&
Object.keys(headers).find(
key => key.toLowerCase() === 'proxy-authorization'
)
if (existProxyAuth) {
throw new InvalidArgumentError('Proxy-Authorization should be sent in ProxyAgent constructor')
throw new InvalidArgumentError(
'Proxy-Authorization should be sent in ProxyAgent constructor'
)
}
}

module.exports = ProxyAgent
module.exports = {
ProxyAgent,
interceptor: opts => {
if (typeof opts === 'string') {
opts = { uri: opts }
}

if (!opts || !opts.uri) {
throw new InvalidArgumentError('Proxy opts.uri is mandatory')
}

const { clientFactory = defaultFactory } = opts

if (typeof clientFactory !== 'function') {
throw new InvalidArgumentError(
'Proxy opts.clientFactory must be a function.'
)
}

if (opts.auth && opts.token) {
throw new InvalidArgumentError(
'opts.auth cannot be used in combination with opts.token'
)
}

return dispatcher => new ProxyAgent(dispatcher, opts)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't an interceptor?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I was referring to when I suggested that ProxyAgent works differently 😅
It requires to hope over the whole dispatch, as it forwards the request to the proxy instead of the intended dispatch.
It does not respects the previous dispatcher, so if proxy is used, it should be placed at the very beginning of the chain.

I couldn't think of any other way without re-working the ProxyAgent, but totally open to suggestions

Copy link
Member

Choose a reason for hiding this comment

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

I will have to investigate this

Copy link
Member

@ronag ronag Mar 10, 2024

Choose a reason for hiding this comment

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

It sounds to me like this should not be an interceptor.

Copy link
Member

Choose a reason for hiding this comment

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

Let's skip this for now so we can land.

54 changes: 54 additions & 0 deletions lib/interceptor/redirect.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
const { InvalidArgumentError } = require('../core/errors')
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
const Dispatcher = require('../dispatcher-base')
const RedirectHandler = require('../handler/RedirectHandler')

class RedirectDispatcher extends Dispatcher {
Copy link
Member

Choose a reason for hiding this comment

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

This class is useful as its own export.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add it as a RedirectAgent

#opts
#dispatcher

constructor (dispatcher, opts) {
super()

this.#dispatcher = dispatcher
this.#opts = opts
}

dispatch (opts, handler) {
return this.#dispatcher.dispatch(
opts,
new RedirectHandler(this.#dispatcher, opts, this.#opts, handler)
)
}

close (...args) {
return this.#dispatcher.close(...args)
}

destroy (...args) {
return this.#dispatcher.destroy(...args)
}
}

module.exports = opts => {
if (opts?.maxRedirections == null || opts?.maxRedirections === 0) {
return null
}

if (!Number.isInteger(opts.maxRedirections) || opts.maxRedirections < 0) {
throw new InvalidArgumentError('maxRedirections must be a positive number')
}

return dispatcher => new RedirectDispatcher(dispatcher, opts)
}

module.exports = opts => {
if (opts?.maxRedirections == null || opts?.maxRedirections === 0) {
return null
}

if (!Number.isInteger(opts.maxRedirections) || opts.maxRedirections < 0) {
throw new InvalidArgumentError('maxRedirections must be a positive number')
}

return dispatcher => new RedirectDispatcher(dispatcher, opts)
}
33 changes: 33 additions & 0 deletions lib/interceptor/retry.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
const Dispatcher = require('../dispatcher-base')
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
const RetryHandler = require('../handler/RetryHandler')

class RetryDispatcher extends Dispatcher {
#dispatcher
#opts

constructor (dispatcher, opts) {
super()

this.#dispatcher = dispatcher
this.#opts = opts
}

dispatch (opts, handler) {
return this.#dispatcher.dispatch(
opts,
new RetryHandler(this.#dispatcher, opts, this.#opts, handler)
)
}

close (...args) {
return this.#dispatcher.close(...args)
}

destroy (...args) {
return this.#dispatcher.destroy(...args)
}
}

module.exports = opts => {
return dispatcher => new RetryDispatcher(dispatcher, opts)
}