From c531c34400664764da9b70782c9946a7afc6ae83 Mon Sep 17 00:00:00 2001 From: geek Date: Wed, 27 Mar 2019 13:13:20 -0500 Subject: [PATCH 1/3] Remove usage of deprecated url.Parse --- README.md | 6 +++--- lib/index.js | 34 ++++++++++++++++++---------------- test/index.js | 18 +++++++++++++++++- 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 45d356c..14158f4 100755 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ Initiate an HTTP request. - `baseUrl` - fully qualified uri string used as the base url. Most useful with `request.defaults`, for example when you want to do many requests to the same domain. If `baseUrl` is `https://example.com/api/`, then requesting `/end/point?test=true` will fetch `https://example.com/api/end/point?test=true`. Any querystring in the `baseUrl` will be overwritten with the querystring in the `uri` When `baseUrl` is given, `uri` must also be a string. - - `socketPath` - `/path/to/unix/socket` for Server. + - `socketPath` - `/path/to/unix/socket` for Server. When using `socketPath` the `uri` parameter to `request` and the shortcut functions will be ignored. - `payload` - The request body as a string, Buffer, Readable Stream, or an object that can be serialized using `JSON.stringify()`. - `headers` - An object containing request headers. - `redirects` - The maximum number of redirects to follow. @@ -333,7 +333,7 @@ To enable events, use `Wreck.defaults({ events: true })`. Events are available v The request event is emitted just before *wreck* creates a request. The handler should accept the following arguments `(uri, options)` where: - - `uri` - the result of `Url.parse(uri)`. This will provide information about + - `uri` - the result of `new URL(uri)`. This will provide information about the resource requested. Also includes the headers and method. - `options` - the options passed into the request function. This will include a payload if there is one. @@ -360,7 +360,7 @@ handler should accept the following arguments `(err, details)` where: - `req` - the raw `ClientHttp` request object - `res` - the raw `IncomingMessage` response object - `start` - the time that the request was initiated - - `uri` - the result of `Url.parse(uri)`. This will provide information about + - `uri` - the result of `new URL(uri)`. This will provide information about the resource requested. Also includes the headers and method. This event is useful for logging all requests that go through *wreck*. The `err` diff --git a/lib/index.js b/lib/index.js index 5301a8f..1f223a9 100755 --- a/lib/index.js +++ b/lib/index.js @@ -57,24 +57,27 @@ internals.Client.prototype.defaults = function (options) { }; +// baseUrl needs to end in a trailing / if it contains paths that need to be preserved internals.resolveUrl = function (baseUrl, path) { if (!path) { return baseUrl; } - const parsedPath = Url.parse(path); - if (parsedPath.host) { - Hoek.assert(parsedPath.protocol, 'Invalid destination path missing protocol'); - return Url.format(parsedPath); + // The URL constructor does a resolve, so need to trim the leading / from path + if (path.indexOf('/') === 0) { + path = path.substr(1); } - const parsedBase = Url.parse(baseUrl); - parsedBase.pathname = parsedBase.pathname + parsedPath.pathname; - parsedBase.pathname = parsedBase.pathname.replace(/[/]{2,}/g, '/'); - parsedBase.search = parsedPath.search; // Always use the querystring from the path argument + const base = new Url.URL(baseUrl); + // When the pathname doesn't end in a trailing / it won't be part of the formatted url returned + if (base.pathname.lastIndexOf('/') !== base.pathname.length - 1) { + base.pathname += '/'; + } - return Url.format(parsedBase); + // Will default to path if it's not a relative URL + const url = new Url.URL(path, base); + return Url.format(url); }; @@ -124,22 +127,20 @@ internals.Client.prototype.request = function (method, url, options = {}) { internals.Client.prototype._request = function (method, url, options, relay, _trace) { const uri = {}; - let parsedUri; if (options.socketPath) { uri.socketPath = options.socketPath; - delete options.socketPath; - parsedUri = Url.parse(url); + // The host must be empty according to https://tools.ietf.org/html/rfc2616#section-14.23 + uri.host = ''; } else { uri.setHost = false; - parsedUri = new Url.URL(url); + const parsedUri = new Url.URL(url); + internals.applyUrlToOptions(uri, parsedUri); } - internals.applyUrlToOptions(uri, parsedUri); - uri.method = method.toUpperCase(); uri.headers = options.headers || {}; - uri.headers.host = parsedUri.host; + uri.headers.host = uri.host; const hasContentLength = internals.findHeader('content-length', uri.headers) !== undefined; if (options.payload && typeof options.payload === 'object' && !(options.payload instanceof Stream) && !Buffer.isBuffer(options.payload)) { @@ -664,6 +665,7 @@ internals.findHeader = function (headerName, headers) { internals.applyUrlToOptions = (options, url) => { + options.host = url.host; options.origin = url.origin; options.searchParams = url.searchParams; options.protocol = url.protocol; diff --git a/test/index.js b/test/index.js index 59a66d5..2622a64 100755 --- a/test/index.js +++ b/test/index.js @@ -975,6 +975,14 @@ describe('request()', () => { expect(body.toString()).to.equal(internals.payload); server.close(); }); + + it('requests a POST resource with headers using post shortcut', async () => { + + const server = await internals.server('echo', internals.socket); + const { payload } = await Wreck.post('/', { socketPath: internals.socket, headers: { 'user-agent': 'wreck' }, payload: internals.payload }); + expect(payload.toString()).to.equal(internals.payload); + server.close(); + }); }); }); @@ -1035,9 +1043,17 @@ describe('options.baseUrl', () => { expect(promise.req.path).to.equal('/foo/bar'); }); + it('includes the full path regardless of it begins with a slash', async () => { + + const promise = Wreck.request('get', 'baz', { baseUrl: 'http://localhost:0/foo/bar' }); + await expect(promise).to.reject(); + expect(promise.req._headers.host).to.equal('localhost:0'); + expect(promise.req.path).to.equal('/foo/bar/baz'); + }); + it('uses baseUrl option with a url that has a querystring', async () => { - const promise = Wreck.request('get', '/bar?test=hello', { baseUrl: 'http://localhost:0/foo' }); + const promise = Wreck.request('get', '/bar?test=hello', { baseUrl: 'http://localhost:0/foo/' }); await expect(promise).to.reject(); expect(promise.req._headers.host).to.equal('localhost:0'); expect(promise.req.path).to.equal('/foo/bar?test=hello'); From 0571191b6776efdac7ea733539f3f88ac9ba32c0 Mon Sep 17 00:00:00 2001 From: geek Date: Wed, 27 Mar 2019 13:22:05 -0500 Subject: [PATCH 2/3] Simplify path slash detection --- lib/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/index.js b/lib/index.js index 1f223a9..ea46e1f 100755 --- a/lib/index.js +++ b/lib/index.js @@ -65,13 +65,13 @@ internals.resolveUrl = function (baseUrl, path) { } // The URL constructor does a resolve, so need to trim the leading / from path - if (path.indexOf('/') === 0) { + if (path.startsWith('/')) { path = path.substr(1); } const base = new Url.URL(baseUrl); // When the pathname doesn't end in a trailing / it won't be part of the formatted url returned - if (base.pathname.lastIndexOf('/') !== base.pathname.length - 1) { + if (!base.pathname.endsWith('/')) { base.pathname += '/'; } From 6fc514c58c5c181327fa3e84d2a95d7d8b93f079 Mon Sep 17 00:00:00 2001 From: geek Date: Thu, 28 Mar 2019 14:41:35 -0500 Subject: [PATCH 3/3] Remove backward compatible baseUrl handling, use URL instead --- README.md | 4 ++-- lib/index.js | 13 +------------ test/index.js | 27 +++++++++++++++++++++------ 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 14158f4..c108a9f 100755 --- a/README.md +++ b/README.md @@ -102,8 +102,8 @@ Initiate an HTTP request. use a callback, pass `null` in this position. The options object supports the following optional keys: - `baseUrl` - fully qualified uri string used as the base url. Most useful with `request.defaults`, for example when you want to do many requests to the same domain. - If `baseUrl` is `https://example.com/api/`, then requesting `/end/point?test=true` will fetch `https://example.com/api/end/point?test=true`. Any - querystring in the `baseUrl` will be overwritten with the querystring in the `uri` When `baseUrl` is given, `uri` must also be a string. + If `baseUrl` is `https://example.com/api/`, then requesting `/end/point?test=true` will fetch `https://example.com/end/point?test=true`. Any + querystring in the `baseUrl` will be overwritten with the querystring in the `uri` When `baseUrl` is given, `uri` must also be a string. In order to retain the `/api/` portion of the `baseUrl` in the example, the `path` must not start with a leading `/` and the `baseUrl` must end with a trailing `/`. - `socketPath` - `/path/to/unix/socket` for Server. When using `socketPath` the `uri` parameter to `request` and the shortcut functions will be ignored. - `payload` - The request body as a string, Buffer, Readable Stream, or an object that can be serialized using `JSON.stringify()`. - `headers` - An object containing request headers. diff --git a/lib/index.js b/lib/index.js index ea46e1f..a97e623 100755 --- a/lib/index.js +++ b/lib/index.js @@ -64,19 +64,8 @@ internals.resolveUrl = function (baseUrl, path) { return baseUrl; } - // The URL constructor does a resolve, so need to trim the leading / from path - if (path.startsWith('/')) { - path = path.substr(1); - } - - const base = new Url.URL(baseUrl); - // When the pathname doesn't end in a trailing / it won't be part of the formatted url returned - if (!base.pathname.endsWith('/')) { - base.pathname += '/'; - } - // Will default to path if it's not a relative URL - const url = new Url.URL(path, base); + const url = new Url.URL(path, baseUrl); return Url.format(url); }; diff --git a/test/index.js b/test/index.js index 2622a64..1a682d8 100755 --- a/test/index.js +++ b/test/index.js @@ -988,6 +988,13 @@ describe('request()', () => { describe('options.baseUrl', () => { + it('uses path when path is a full URL', async () => { + + const promise = Wreck.request('get', 'http://localhost:8080/foo', { baseUrl: 'http://localhost:0/' }); + await expect(promise).to.reject(); + expect(promise.req._headers.host).to.equal('localhost:8080'); + }); + it('uses baseUrl option with trailing slash and uri is prefixed with a slash', async () => { const promise = Wreck.request('get', '/foo', { baseUrl: 'http://localhost:0/' }); @@ -1032,6 +1039,14 @@ describe('options.baseUrl', () => { const promise = Wreck.request('get', '/bar', { baseUrl: 'http://localhost:0/foo' }); await expect(promise).to.reject(); expect(promise.req._headers.host).to.equal('localhost:0'); + expect(promise.req.path).to.equal('/bar'); + }); + + it('uses baseUrl option with a relative path', async () => { + + const promise = Wreck.request('get', 'bar', { baseUrl: 'http://localhost:0/foo/' }); + await expect(promise).to.reject(); + expect(promise.req._headers.host).to.equal('localhost:0'); expect(promise.req.path).to.equal('/foo/bar'); }); @@ -1040,20 +1055,20 @@ describe('options.baseUrl', () => { const promise = Wreck.request('get', '/bar', { baseUrl: 'http://localhost:0/foo/' }); await expect(promise).to.reject(); expect(promise.req._headers.host).to.equal('localhost:0'); - expect(promise.req.path).to.equal('/foo/bar'); + expect(promise.req.path).to.equal('/bar'); }); - it('includes the full path regardless of it begins with a slash', async () => { + it('uses baseUrl option with a url that has a querystring', async () => { - const promise = Wreck.request('get', 'baz', { baseUrl: 'http://localhost:0/foo/bar' }); + const promise = Wreck.request('get', 'bar?test=hello', { baseUrl: 'http://localhost:0/foo/' }); await expect(promise).to.reject(); expect(promise.req._headers.host).to.equal('localhost:0'); - expect(promise.req.path).to.equal('/foo/bar/baz'); + expect(promise.req.path).to.equal('/foo/bar?test=hello'); }); - it('uses baseUrl option with a url that has a querystring', async () => { + it('uses baseUrl option with a url that has a querystring will override any base querystring', async () => { - const promise = Wreck.request('get', '/bar?test=hello', { baseUrl: 'http://localhost:0/foo/' }); + const promise = Wreck.request('get', 'bar?test=hello', { baseUrl: 'http://localhost:0/foo/?test=hi' }); await expect(promise).to.reject(); expect(promise.req._headers.host).to.equal('localhost:0'); expect(promise.req.path).to.equal('/foo/bar?test=hello');