Simplify how baseURL works and support URL paths with colons near the…

… front (#95)

The default resolveURL is more complex than one would hope because it
tries to use the two-argument URL constructor to combine a path with a
base URL, but for some reason back in 259206037 we decided that a
leading slash in the given path should be interpreted as relative to the
full base URL rather than just the host (ie, to treat it the same as no
leading slash), which breaks the algorithm performed by `new URL`. We
worked around it by messing around with slashes a bit but it turned out
if you passed `/foo:bar` as your path, it would end up treating that as
equivalent to `foo:bar`, which it interprets as an URL with scheme
`foo`!

We go for an approach that is in some ways much simpler. After this
change, the URL that is chosen is exactly the same as the URL you'd get
to by clicking a link with that value on a web page with the given base
URL.

When the base URL doesn't contain a path, this doesn't change the
behavior. But it does change the behavior if there is a path in a few
ways:

- If the path passed to a method such as `this.get()` starts with a
  slash, then it is resolved relative to the *host* of the base URL, not
  to the full base URL. That is, if `baseURL` is https://foo.com/a/b/c/,
  `this.get('d')` resolves to https://foo.com/a/b/c/d, but
  `this.get('/d')` resolves to https://foo.com/d
- If the base URL has a path element and does not end in a slash, then
  the given path replaces the last element of the path. That is, if
  `baseURL` is https://foo.com/a/b/c, `this.get('d')` resolves to
  https://foo.com/a/b/d

We think making this into a simple rule that matches browser behavior
(and fixes use with paths that have a colon in the first segment) is
a reasonable major-version change.

Fixes #23.

.changeset/many-swans-wait.md

@@ -0,0 +1,31 @@
+---
+'@apollo/datasource-rest': major
+---
+
+Simplify interpretation of `this.baseURL` so it works exactly like links in a web browser.
+
+If you set `this.baseURL` to an URL with a non-empty path component, this may change the URL that your methods talk to. Specifically:
+
+- Paths passed to methods such as `this.get('/foo')` now _replace_ the entire URL path from `this.baseURL`. If you did not intend this, write `this.get('foo')` instead.
+- If `this.baseURL` has a non-empty path and does not end in a trailing slash, paths such as `this.get('foo')` will _replace_ the last component of the URL path instead of adding a new component. If you did not intend this, add a trailing slash to `this.baseURL`.
+
+If you preferred the v4 semantics and do not want to make the changes described above, you can restore v4 semantics by overriding `resolveURL` in your subclass with the following code from v4:
+
+```ts
+override resolveURL(path: string): ValueOrPromise<URL> {
+  if (path.startsWith('/')) {
+    path = path.slice(1);
+  }
+  const baseURL = this.baseURL;
+  if (baseURL) {
+    const normalizedBaseURL = baseURL.endsWith('/')
+      ? baseURL
+      : baseURL.concat('/');
+    return new URL(path, normalizedBaseURL);
+  } else {
+    return new URL(path);
+  }
+}
+```
+
+As part of this change, it is now possible to specify URLs whose first path segment contains a colon, such as `this.get('/foo:bar')`.

README.md

@@ -65,6 +65,15 @@ class MoviesAPI extends RESTDataSource {
 }
 ```
 
+`RESTDataSource` interprets the string passed to methods such as `this.get()` as an URL in exactly the same way that a browser interprets a link on a web page whose address is the same as `this.baseURL`. This may lead to slightly surprising behavior if `this.baseURL` has a non-empty path component:
+
+- If the string passed to a method such as `this.get()` starts with a slash, then it is resolved relative to the *host* of the base URL, not to the full base URL. That is, if `this.baseURL` is `https://foo.com/a/b/c/`, then `this.get('d')` resolves to `https://foo.com/a/b/c/d`, but `this.get('/d')` resolves to `https://foo.com/d`.
+- If the base URL has a path element and does not end in a slash, then the given path replaces the last element of the path. That is, if `baseURL` is `https://foo.com/a/b/c`, `this.get('d')` resolves to `https://foo.com/a/b/d`
+
+In practice, this means that you should usually set `this.baseURL` to the common prefix of all URLs you want to access *including a trailing slash*, and you should pass paths *without a leading slash* to methods such as `this.get()`.
+
+If a resource's path starts with something that looks like an URL because it contains a colon and you want it to be added on to the full base URL after its path (so you can't pass it as `this.get('/foo:bar')`), you can pass a path starting with `./`, like `this.get('./foo:bar')`.
+
 ##### `memoizeGetRequests`
 By default, `RESTDataSource` caches all outgoing GET **requests** in a separate memoized cache from the regular response cache. It makes the assumption that all responses from HTTP GET calls are cacheable by their URL.
 If a request is made with the same cache key (URL by default) but with an HTTP method other than GET, the cached request is then cleared.

src/RESTDataSource.ts

@@ -86,18 +86,7 @@ export abstract class RESTDataSource {
     path: string,
     _request: RequestOptions,
   ): ValueOrPromise<URL> {
-    if (path.startsWith('/')) {
-      path = path.slice(1);
-    }
-    const baseURL = this.baseURL;
-    if (baseURL) {
-      const normalizedBaseURL = baseURL.endsWith('/')
-        ? baseURL
-        : baseURL.concat('/');
-      return new URL(path, normalizedBaseURL);
-    } else {
-      return new URL(path);
-    }
+    return new URL(path, this.baseURL);
   }
 
   protected cacheOptionsFor?(

src/__tests__/RESTDataSource.test.ts

@@ -52,12 +52,12 @@ describe('RESTDataSource', () => {
         }
       })();
 
-      nock(apiUrl).get('/bar/foo').reply(200, {});
+      nock(apiUrl).get('/foo').reply(200, {});
 
       await dataSource.getFoo();
     });
 
-    it('adds a trailing slash to the base URL if needed', async () => {
+    it('does not automatically adds a trailing slash to the base URL', async () => {
       const dataSource = new (class extends RESTDataSource {
         override baseURL = `${apiUrl}/api`;
 
@@ -66,11 +66,61 @@ describe('RESTDataSource', () => {
         }
       })();
 
+      nock(apiUrl).get('/foo').reply(200, {});
+
+      await dataSource.getFoo();
+    });
+
+    it('works as expected when the base URL has a trailing slash', async () => {
+      const dataSource = new (class extends RESTDataSource {
+        override baseURL = `${apiUrl}/api/`;
+
+        getFoo() {
+          return this.get('foo');
+        }
+      })();
+
       nock(apiUrl).get('/api/foo').reply(200, {});
 
       await dataSource.getFoo();
     });
 
+    it('can use a whole new URL, overriding baseURL', async () => {
+      const dataSource = new (class extends RESTDataSource {
+        override baseURL = `${apiUrl}/api/`;
+
+        getFoo() {
+          return this.get('https://different-api.example.com/foo/bar');
+        }
+      })();
+
+      nock('https://different-api.example.com').get('/foo/bar').reply(200, {});
+
+      await dataSource.getFoo();
+    });
+
+    it.each([
+      ['', './urn:foo:1', '/urn:foo:1'],
+      ['/api/', './urn:foo:1', '/api/urn:foo:1'],
+      ['', '/urn:foo:1', '/urn:foo:1'],
+      ['/api/', '/urn:foo:1', '/urn:foo:1'],
+    ])(
+      'supports paths with colons near the beginning (base URL path=%p, passed path=%p)',
+      async (baseURLPath, passedPath, nockPath) => {
+        const dataSource = new (class extends RESTDataSource {
+          override baseURL = `${apiUrl}${baseURLPath}`;
+
+          getFoo() {
+            return this.get(passedPath);
+          }
+        })();
+
+        nock(apiUrl).get(nockPath).reply(200, {});
+
+        await dataSource.getFoo();
+      },
+    );
+
     it('allows resolving a base URL asynchronously', async () => {
       const dataSource = new (class extends RESTDataSource {
         override async resolveURL(path: string, request: RequestOptions) {