Merge pull request #793 from nextstrain/james/preserve-queries-across…

…-redirects

Preserve URL queries across redirects

src/endpoints/sources.js

@@ -57,9 +57,7 @@ export const canonicalizeDataset = (pathBuilder) => (req, res, next) => {
 
   const version = dataset.versionDescriptor ? `@${dataset.versionDescriptor}` : '';
 
-  const canonicalPath = pathBuilder.length >= 2
-    ? pathBuilder(req, resolvedDataset.pathParts.join("/") + version)
-    : pathBuilder(resolvedDataset.pathParts.join("/") + version);
+  const canonicalPath = pathBuilder(req, resolvedDataset.pathParts.join("/") + version);
 
   /* 307 Temporary Redirect preserves request method, unlike 302 Found, which
    * is important since this middleware function may be used in non-GET routes.
@@ -356,27 +354,13 @@ function receiveSubresource(subresourceExtractor) {
  * @returns {String} Path for {@link Source#dataset} or {@link Source#narrative}
  */
 
-/* Confused about the duplication below?  It's the documented way to handle
- * overloaded (e.g. arity-dependent) function signatures.¹  Note that it relies
- * on the "nestled" or "cuddled" end and start comment markers.
- *   -trs, 16 June 2022
- *
- * ¹ https://github.com/jsdoc/jsdoc/issues/1017
- */
 /**
  * @callback pathBuilder
- *
- * @param {String} path - Canonical path for the dataset within the context of
- *   the current {@link Source}
- * @returns {String} Fully-specified path to redirect to
- *//**
-* @callback pathBuilder
-*
-* @param {express.request} req
-* @param {String} path - Canonical path for the dataset within the context of
-*   the current {@link Source}
-* @returns {String} Fully-specified path to redirect to
-*/
+ * @param {express.request} req
+ * @param {String} path - Canonical path (not including query) for the dataset
+ *   within the context of the current {@link Source}
+ * @returns {String} Fully-specified path (including query) to redirect to
+ */
 
 /**
  * @callback subresourceExtractor

src/routing/core.js

@@ -1,3 +1,5 @@
+import url from 'url';
+
 import * as endpoints from '../endpoints/index.js';
 import * as sources from '../sources/index.js';
 
@@ -52,7 +54,10 @@ export function setup(app) {
   app.use([coreBuildRoutes, "/narratives/*"], setSource(req => new CoreSource())); // eslint-disable-line no-unused-vars
 
   app.routeAsync(coreBuildRoutes)
-    .all(setDataset(req => req.path), canonicalizeDataset(path => `/${path}`))
+    .all(
+      setDataset(req => req.path),
+      canonicalizeDataset((req, path) => url.format({pathname: `/${path}`, query: req.query}))
+    )
     .getAsync(getDataset)
     .putAsync(putDataset)
     .deleteAsync(deleteDataset)

src/routing/setup.js

@@ -31,6 +31,12 @@ export function setupApp() {
   app.use(nakedRedirect({reverse: true})); // redirect www.nextstrain.org to nextstrain.org
   app.use(middleware.rejectParentTraversals);
 
+  /* Parse queries with `querystring` so that we can roundtrip queries using
+   * `querystring.stringify` or the single-argument form of `url.format` (which
+   * uses `querystring.stringify`)
+   */
+  app.set("query parser", "simple")
+
 
   /* Setup a request-scoped context object for passing arbitrary request-local
    * data between route and param middleware and route handlers.  Akin to

src/routing/staging.js

@@ -1,3 +1,5 @@
+import url from 'url';
+
 import * as endpoints from '../endpoints/index.js';
 import * as sources from '../sources/index.js';
 
@@ -40,7 +42,10 @@ export function setup(app) {
   ;
 
   app.routeAsync("/staging/*")
-    .all(setDataset(req => req.params[0]), canonicalizeDataset(path => `/staging/${path}`))
+    .all(
+      setDataset(req => req.params[0]),
+      canonicalizeDataset((req, path) => url.format({pathname: `/staging/${path}`, query: req.query}))
+    )
     .getAsync(getDataset)
     .putAsync(putDataset)
     .deleteAsync(deleteDataset)

test/date_descriptor.test.js

@@ -307,13 +307,16 @@ const redirects = [
   not checking the existence/validity of any version descriptor against the WNV
   resource, rather we are redirecting and deferring those checks */
   ["WNV@2020-01-01", "WNV/NA@2020-01-01"],
+  /* URL queries should be preserved across the redirect, but only for the RESTful API */
+  ["WNV@2020-01-01?c=region", "WNV/NA@2020-01-01?c=region", {charon: false}],
+  ["staging/WNV@2020-01-01?c=region", "staging/WNV/NA@2020-01-01?c=region", {charon: false}],
 ]
 
 describe("Paths redirect with version descriptors", () => {
   /* See <https://github.com/node-fetch/node-fetch#manual-redirect> for how
   fetch stores the redirect location when {redirect: 'manual'} */
 
-  redirects.forEach(([fromUrl, toUrl]) => {
+  redirects.forEach(([fromUrl, toUrl, opts]) => {
     /* test RESTful API */
     (['html', 'json']).forEach((type) => {
       it(`REST API: ${fromUrl} goes to ${toUrl} (${type})`, async () => {
@@ -325,6 +328,14 @@ describe("Paths redirect with version descriptors", () => {
 
 
     /* test Charon API */
+    if (opts?.charon === false) return;
+
+    // Charon API datasets (prefix query) must not contain a query themselves
+    it (`Charon prefix doesn't include query params`, () => {
+      expect(fromUrl.includes('?')).toBeFalsy();
+      expect(toUrl.includes('?')).toBeFalsy();
+    })
+
     const charonUrl = (path, sidecar) =>
       `${BASE_URL}/charon/getDataset?prefix=${path}${sidecar ? `&type=root-sequence` : ''}`;