Merge pull request hapijs#92 from geek/master

More robust baseUrl and uri merging

README.md

@@ -36,7 +36,9 @@ var wreckWithTimeout = wreck.defaults({
 
 // all attributes are optional
 var options = {
-    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`. When `baseUrl` is given, `uri` must also be a string.
+    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.
     payload:   readableStream || 'foo=bar' || new Buffer('foo=bar'),
     headers:   { /* http headers */ },
     redirects: 3,

lib/index.js

@@ -37,13 +37,31 @@ internals.Client = function (defaults) {
 
 Hoek.inherits(internals.Client, Events.EventEmitter);
 
+
 internals.Client.prototype.defaults = function (options) {
 
     options = Hoek.applyToDefaultsWithShallow(options, this._defaults, ['agent', 'payload', 'downstreamRes']);
     return new internals.Client(options);
 };
 
 
+internals.resolveUrl = function (baseUrl, path) {
+
+    if (!path) {
+        return baseUrl;
+    }
+
+    var parsedBase = Url.parse(baseUrl);
+    var parsedPath = Url.parse(path);
+
+    parsedBase.pathname += parsedPath.pathname;
+    parsedBase.pathname = parsedBase.pathname.replace(/[/]{2,}/g, '/');
+    parsedBase.search = parsedPath.search;      // Always use the querystring from the path argument
+
+    return Url.format(parsedBase);
+};
+
+
 internals.Client.prototype.request = function (method, url, options, callback, _trace) {
 
     var self = this;
@@ -57,23 +75,8 @@ internals.Client.prototype.request = function (method, url, options, callback, _
     Hoek.assert((options.agent === undefined || options.agent === null) || (typeof options.rejectUnauthorized !== 'boolean'),
         'options.agent cannot be set to an Agent at the same time as options.rejectUnauthorized is set');
 
-    // Setup request
     if (options.baseUrl) {
-
-        // Handle all cases to make sure that there's only one slash between
-        // baseUrl and uri.
-        var baseUrlEndsWithSlash = options.baseUrl.lastIndexOf('/') === options.baseUrl.length - 1;
-        var uriStartsWithSlash = url.indexOf('/') === 0;
-
-        if (baseUrlEndsWithSlash && uriStartsWithSlash) {
-            url = options.baseUrl + url.slice(1);
-        } else if (baseUrlEndsWithSlash || uriStartsWithSlash) {
-            url = options.baseUrl + url;
-        } else if (url === '') {
-            url = options.baseUrl;
-        } else {
-            url = options.baseUrl + '/' + url;
-        }
+        url = internals.resolveUrl(options.baseUrl, url);
     }
 
     var uri = Url.parse(url);
@@ -417,6 +420,7 @@ internals.Client.prototype.delete = function (uri, options, callback) {
 
 
 // Wrapper so that shortcut can be optimized with required params
+
 internals.Client.prototype._shortcutWrap = function (method, uri /* [options], callback */) {
 
     var options = (typeof arguments[2] === 'function' ? {} : arguments[2]);

test/index.js

@@ -7,6 +7,7 @@ var Fs = require('fs');
 var Events = require('events');
 var Stream = require('stream');
 var Code = require('code');
+var Hoek = require('hoek');
 var Lab = require('lab');
 var Wreck = require('../');
 
@@ -961,7 +962,7 @@ describe('request()', function () {
     });
 });
 
-describe('baseUrl', function () {
+describe('options.baseUrl', function () {
 
     it('uses baseUrl option with trailing slash and uri is prefixed with a slash', function (done) {
 
@@ -974,38 +975,65 @@ describe('baseUrl', function () {
 
     it('uses baseUrl option without trailing slash and uri is prefixed with a slash', function (done) {
 
-        var r = Wreck.request('get', '/foo', { baseUrl: 'http://localhost' }, function (err, res) {
+        var request = Wreck.request('get', '/foo', { baseUrl: 'http://localhost' }, Hoek.ignore);
 
-            expect(r._headers.host).to.equal('localhost');
-            done();
-        });
+        expect(request._headers.host).to.equal('localhost');
+        expect(request.path).to.equal('/foo');
+        done();
     });
 
     it('uses baseUrl option with trailing slash and uri is prefixed without a slash', function (done) {
 
-        var r = Wreck.request('get', 'foo', { baseUrl: 'http://localhost/' }, function (err, res) {
+        var request = Wreck.request('get', 'foo', { baseUrl: 'http://localhost/' }, Hoek.ignore);
 
-            expect(r._headers.host).to.equal('localhost');
-            done();
-        });
+        expect(request._headers.host).to.equal('localhost');
+        expect(request.path).to.equal('/foo');
+        done();
     });
 
     it('uses baseUrl option without trailing slash and uri is prefixed without a slash', function (done) {
 
-        var r = Wreck.request('get', 'foo', { baseUrl: 'http://localhost' }, function (err, res) {
+        var request = Wreck.request('get', 'foo', { baseUrl: 'http://localhost' }, Hoek.ignore);
 
-            expect(r._headers.host).to.equal('localhost');
-            done();
-        });
+        expect(request._headers.host).to.equal('localhost');
+        expect(request.path).to.equal('/foo');
+        done();
     });
 
     it('uses baseUrl option when uri is an empty string', function (done) {
 
-        var r = Wreck.request('get', '', { baseUrl: 'http://localhost' }, function (err, res) {
+        var request = Wreck.request('get', '', { baseUrl: 'http://localhost' }, Hoek.ignore);
 
-            expect(r._headers.host).to.equal('localhost');
-            done();
-        });
+        expect(request._headers.host).to.equal('localhost');
+        expect(request.path).to.equal('/');
+        done();
+    });
+
+    it('uses baseUrl option with a path', function (done) {
+
+        var request = Wreck.request('get', '/bar', { baseUrl: 'http://localhost/foo' }, Hoek.ignore);
+
+        expect(request._headers.host).to.equal('localhost');
+        expect(request.path).to.equal('/foo/bar');
+        done();
+    });
+
+    it('uses baseUrl option with a path and removes extra slashes', function (done) {
+
+        var request = Wreck.request('get', '/bar', { baseUrl: 'http://localhost/foo/' }, Hoek.ignore);
+
+        expect(request._headers.host).to.equal('localhost');
+        expect(request.path).to.equal('/foo/bar');
+        done();
+    });
+
+    it('uses baseUrl option with a url that has a querystring', function (done) {
+
+        var request = Wreck.request('get', '/bar?test=hello', { baseUrl: 'http://localhost/foo' }, Hoek.ignore);
+
+        expect(request._headers.host).to.equal('localhost');
+        expect(request.path).to.equal('/foo/bar?test=hello');
+        done();
     });
 });