From c88a87d4938c6d9bdaa6f91437594da77c3d9417 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Wed, 17 Jan 2024 10:24:32 +1300 Subject: [PATCH] [version descriptors] redirect versioned URLs The desired behaviour is that (e.g.) dengue/denv1@version redirects to dengue/denv1/genome@version, similarly to how the non-versioned URL currently behaves. This failed because of three inter-related aspects: 1. The previous commit removed (e.g.) "dengue/denv1" from the index, choosing instead to place that data under the "dengue/denv1/genome" ID. 2. The server instantiates the Resource (Dataset) class for the request before resolving/redirecting the URL. During this instantiation the requested version was checked against the index and since the resource wasn't present in the index this failed (NotFound error). This was solved by deferring the version lookup until after the URL resolving/redirecting stage. 3. The redirect URL didn't include the version descriptor. --- src/endpoints/sources.js | 9 ++++--- src/sources/models.js | 10 +++++--- test/date_descriptor.test.js | 48 ++++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/src/endpoints/sources.js b/src/endpoints/sources.js index 67d8855df..70e4e51c3 100644 --- a/src/endpoints/sources.js +++ b/src/endpoints/sources.js @@ -43,7 +43,8 @@ export const setDataset = (pathExtractor) => (req, res, next) => { /** * Generate Express middleware that redirects to the canonical path for the - * current {@link Dataset} if it is not fully resolved. + * current {@link Dataset} if it is not fully resolved. Any provided version + * descriptor is included in the redirect. * * @param {pathBuilder} pathBuilder - Function to build a fully-specified path * @returns {expressMiddleware} @@ -54,9 +55,11 @@ export const canonicalizeDataset = (pathBuilder) => (req, res, next) => { if (dataset === resolvedDataset) return next(); + const version = dataset.versionDescriptor ? `@${dataset.versionDescriptor}` : ''; + const canonicalPath = pathBuilder.length >= 2 - ? pathBuilder(req, resolvedDataset.pathParts.join("/")) - : pathBuilder(resolvedDataset.pathParts.join("/")); + ? pathBuilder(req, resolvedDataset.pathParts.join("/") + version) + : pathBuilder(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. diff --git a/src/sources/models.js b/src/sources/models.js index 676f4995d..964c2f4b6 100644 --- a/src/sources/models.js +++ b/src/sources/models.js @@ -144,7 +144,6 @@ export class Resource { this.source = source; this.pathParts = pathParts; this.versionDescriptor = versionDescriptor; - [this.versionDate, this.versionUrls] = this.versionInfo(versionDescriptor); // Require baseParts, otherwise we have no actual dataset/narrative path. // This inspects baseParts because some of the pathParts (above) may not @@ -241,12 +240,15 @@ export class Subresource { * Note that the URL is not signed, and changes will be required to support * this as needed. */ - if (this.resource.versionUrls) { + + const versionUrls = this.resource.versionInfo(this.resource.versionDescriptor)[1]; + + if (versionUrls) { if (!['HEAD', 'GET'].includes(method)) { throw new InternalServerError(`Only the GET and HEAD methods are available for previous resource versions`); } - if (this.resource.versionUrls[this.type]) { - return this.resource.versionUrls[this.type]; + if (versionUrls[this.type]) { + return versionUrls[this.type]; } throw new NotFound(`This version of the resource does not have a subresource for ${this.type}`); } diff --git a/test/date_descriptor.test.js b/test/date_descriptor.test.js index 82fbd6b64..e5ad6e4f6 100644 --- a/test/date_descriptor.test.js +++ b/test/date_descriptor.test.js @@ -301,6 +301,54 @@ describe("Valid narratives", () => { } }) +const redirects = [ + ["WNV", "WNV/NA"], + /* Note that WNV is not in the index, so this versioned test ensures we are + 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"], +] + +describe("Paths redirect with version descriptors", () => { + /* See for how + fetch stores the redirect location when {redirect: 'manual'} */ + + redirects.forEach(([fromUrl, toUrl]) => { + /* test RESTful API */ + (['html', 'json']).forEach((type) => { + it(`REST API: ${fromUrl} goes to ${toUrl} (${type})`, async () => { + const res = await fetchType(`${BASE_URL}/${fromUrl}`, type); + expect(res.status).toEqual(307); + expect(res.headers.get('location')).toEqual(`${BASE_URL}/${toUrl}`); + }) + }) + + + /* test Charon API */ + const charonUrl = (path, sidecar) => + `${BASE_URL}/charon/getDataset?prefix=${path}${sidecar ? `&type=root-sequence` : ''}`; + + /* note that the redirect URL of charon requests is constructed by creating + a URL object (the RESTful API does not) which results in '/' and '@' being + escaped in the query. This isn't necessary, but it's also fine to do so. */ + + it(`Charon main dataset: ${fromUrl} goes to ${toUrl} `, async () => { + const res = await fetch(charonUrl(fromUrl, false), {redirect: 'manual'}); + expect(res.status).toEqual(307); + expect(decodeURIComponent(res.headers.get('location'))).toEqual(charonUrl(toUrl, false)); + }) + + it(`Charon sidecar: ${fromUrl} goes to ${toUrl} `, async () => { + const res = await fetch(charonUrl(fromUrl, true), {redirect: 'manual'}); + expect(res.status).toEqual(307); + expect(decodeURIComponent(res.headers.get('location'))).toEqual(charonUrl(toUrl, true)); + }) + + }) + +}) + + /** * Map the sidecar names we use day-to-day to their content types * used within the server