Prevent removing trailing slash in request URL (#96)

* Change `isAbsolute` property to `isComplete`

The check was not actually whether or not a URL was absolute, but
whether it was "complete".  For example, "http://foo.com" should have
been recognized, although that is not actually what an "absolute url"
usually refers to, which is something like "/foo".  The name change just
increases the clarify of what is being verifyied.

* Correctly build full URL from parts

The old way of building a URL was too brittle and didn't cover a lot of
the different cases that could take place, in different combinations of
the host, namespace, and segment having or not having slashes included.

The new approach implemented here handles the various cases correctly,
and ensures that we do not remove a trailing slash if one was provided.

Closes #70

addon/mixins/ajax-request.js

@@ -43,6 +43,27 @@ function isJSONAPIContentType(header) {
   return header.indexOf(JSONAPIContentType) === 0;
 }
 
+function startsWithSlash(string) {
+  return string.charAt(0) === '/';
+}
+
+function endsWithSlash(string) {
+  return string.charAt(string.length - 1) === '/';
+}
+
+function stripSlashes(path) {
+  // make sure path starts with `/`
+  if (startsWithSlash(path)) {
+    path = path.substring(1);
+  }
+
+  // remove end `/`
+  if (endsWithSlash(path)) {
+    path = path.slice(0, -1);
+  }
+  return path;
+}
+
 let pendingRequestCount = 0;
 if (testing) {
   Test.registerWaiter(function() {
@@ -223,20 +244,55 @@ export default Mixin.create({
     return options;
   },
 
-  _buildURL(url, options) {
-    const host = options.host || get(this, 'host');
-    const namespace = get(this, 'namespace');
+  /**
+   * Build a URL for a request
+   *
+   * If the provided `url` is deemed to be a complete URL, it will be returned
+   * directly.  If it is not complete, then the segment provided will be combined
+   * with the `host` and `namespace` options of the request class to create the
+   * full URL.
+   *
+   * @private
+   * @param {string} url the url, or url segment, to request
+   * @param {object} [options] the options for the request being made
+   * @param {object.host} [host] the host to use for this request
+   * @returns {string} the URL to make a request to
+   */
+  _buildURL(url, options = {}) {
     const urlObject = new RequestURL(url);
 
     // If the URL passed is not relative, return the whole URL
-    if (urlObject.isAbsolute) {
+    if (urlObject.isComplete) {
       return urlObject.href;
     }
 
-    let _url = this._normalizePath(url);
-    let _namespace = this._normalizePath(namespace);
+    const host = options.host || get(this, 'host');
+    let namespace = get(this, 'namespace');
+    if (namespace) {
+      namespace = stripSlashes(namespace);
+    }
+
+    let fullUrl = '';
+    // Add the host, if it exists
+    if (host) {
+      fullUrl += host;
+    }
+    // Add the namespace, if it exists
+    if (namespace) {
+      if (!endsWithSlash(fullUrl)) {
+        fullUrl += '/';
+      }
+      fullUrl += namespace;
+    }
+    // Add the URL segment, if it exists
+    if (url) {
+      if (!startsWithSlash(url)) {
+        fullUrl += '/';
+      }
+      fullUrl += url;
+    }
 
-    return [ host, _namespace, _url ].join('');
+    return fullUrl;
   },
 
   _normalizePath(path) {
@@ -348,9 +404,9 @@ export default Mixin.create({
 
     const urlObject = new RequestURL(url);
     const trustedHosts = get(this, 'trustedHosts') || Ember.A();
-    // Add headers on relative URLs
 
-    if (!urlObject.isAbsolute) {
+    // Add headers on relative URLs
+    if (!urlObject.isComplete) {
       return true;
     } else if (trustedHosts.find((matcher) => this._matchHosts(urlObject.hostname, matcher))) {
       return true;

addon/utils/url-helpers.js

@@ -1,7 +1,7 @@
 /* global require, module, URL */
 import isFastBoot from './is-fastboot';
 
-const absoluteUrlRegex = /^(http|https)/;
+const completeUrlRegex = /^(http|https)/;
 
 /*
  * Isomorphic URL parsing
@@ -81,8 +81,8 @@ export class RequestURL {
     return this._url;
   }
 
-  get isAbsolute() {
-    return this.url.match(absoluteUrlRegex);
+  get isComplete() {
+    return this.url.match(completeUrlRegex);
   }
 
   set url(value) {

tests/unit/ajax-request-test.js

@@ -624,3 +624,72 @@ test('it JSON encodes JSON:API "extension" request data automatically', function
     }
   });
 });
+
+test('it correctly creates the URL to request', function(assert) {
+  class NamespaceLeadingSlash extends AjaxRequest {
+    static get slashType() {
+      return 'leading slash';
+    }
+    get namespace() {
+      return '/bar';
+    }
+  }
+
+  class NamespaceTrailingSlash extends AjaxRequest {
+    static get slashType() {
+      return 'trailing slash';
+    }
+    get namespace() {
+      return 'bar/';
+    }
+  }
+
+  class NamespaceTwoSlash extends AjaxRequest {
+    static get slashType() {
+      return 'leading and trailing slash';
+    }
+    get namespace() {
+      return '/bar/';
+    }
+  }
+
+  class NamespaceNoSlash extends AjaxRequest {
+    static get slashType() {
+      return 'no slashes';
+    }
+    get namespace() {
+      return 'bar';
+    }
+  }
+
+  const hosts = [
+    { hostType: 'trailing slash', host: 'http://foo.com/' },
+    { hostType: 'no trailing slash', host: 'http://foo.com' }
+  ];
+
+  [NamespaceLeadingSlash, NamespaceTrailingSlash, NamespaceTwoSlash, NamespaceNoSlash].forEach((Klass) => {
+    let req = new Klass();
+
+    hosts.forEach((exampleHost) => {
+      const { hostType, host } = exampleHost;
+      ['/baz', 'baz'].forEach((segment) => {
+        assert.equal(
+          req._buildURL(segment, { host }),
+          'http://foo.com/bar/baz',
+          `Host with ${hostType}, Namespace with ${Klass.slashType}, segment: ${segment}`
+        );
+      });
+      ['/baz/', 'baz/'].forEach((segment) => {
+        assert.equal(
+          req._buildURL(segment, { host }),
+          'http://foo.com/bar/baz/',
+          `Host with ${hostType}, Namespace with ${Klass.slashType}, segment: ${segment}`
+        );
+      });
+    });
+  });
+
+  let req = new AjaxRequest();
+  assert.equal(req._buildURL('/baz', { host: 'http://foo.com' }), 'http://foo.com/baz', 'Builds URL correctly without namespace');
+  assert.equal(req._buildURL('/baz'), '/baz', 'Builds URL correctly without namespace or host');
+});

tests/unit/utils/url-helpers-test.js

@@ -15,18 +15,18 @@ test('RequestURL Class: parses the parts of a URL correctly', function(assert) {
   assert.equal(obj.hash, '#hash');
 });
 
-test('RequestURL Class: can detect if the url is absolute', function(assert) {
+test('RequestURL Class: can detect if the url is complete', function(assert) {
   const obj = new RequestURL('http://google.com/test');
-  assert.ok(obj.isAbsolute);
+  assert.ok(obj.isComplete);
 
   const obj2 = new RequestURL('google.com/test');
-  assert.notOk(obj2.isAbsolute);
+  assert.notOk(obj2.isComplete);
 
   const obj3 = new RequestURL('/test');
-  assert.notOk(obj3.isAbsolute);
+  assert.notOk(obj3.isComplete);
 
   const obj4 = new RequestURL('test/http/http');
-  assert.notOk(obj4.isAbsolute);
+  assert.notOk(obj4.isComplete);
 });
 
 test('RequestURL Class: can detect if two hosts are the same', function(assert) {