From 09d72905573e07595cfd1a69f9ea2b0d181fbaa2 Mon Sep 17 00:00:00 2001 From: William Valencia Date: Mon, 14 Oct 2024 20:18:36 -0400 Subject: [PATCH 1/7] CMR-10147: Removing query parameters from self links --- src/routes/catalog.ts | 2 +- src/routes/rootCatalog.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/routes/catalog.ts b/src/routes/catalog.ts index 8a98c192..48c0b2ee 100644 --- a/src/routes/catalog.ts +++ b/src/routes/catalog.ts @@ -105,7 +105,7 @@ export const providerCatalogHandler = async (req: Request, res: Response) => { const selfLinks = generateSelfLinks(req); const childLinks = (collections ?? []).map(({ id, title }) => ({ rel: "child", - href: `${self}/collections/${encodeURIComponent(id)}`, + href: `${self.replace(/\?.*$/, "")}/collections/${encodeURIComponent(id)}`, title, type: "application/json", })); diff --git a/src/routes/rootCatalog.ts b/src/routes/rootCatalog.ts index 5151d346..f36e4f3f 100644 --- a/src/routes/rootCatalog.ts +++ b/src/routes/rootCatalog.ts @@ -45,7 +45,7 @@ const providerLinks = (req: Request, providers: Provider[]): Link[] => { rel: "child", title, type: "application/json", - href: `${self}/${providerId}`, + href: `${self.replace(/\?.*$/, "")}/${providerId}`, })); }; From 1b37e99999087b44d4f37c52e289c819df60269b Mon Sep 17 00:00:00 2001 From: William Valencia Date: Tue, 15 Oct 2024 02:29:47 -0400 Subject: [PATCH 2/7] CMR-10147: Adding spec tests --- src/__tests__/providerCatalog.spec.ts | 38 +++++++++++++++++++++++++++ src/__tests__/rootCatalog.spec.ts | 19 +++++++++++++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/__tests__/providerCatalog.spec.ts b/src/__tests__/providerCatalog.spec.ts index ef55cd2c..6754a9cf 100644 --- a/src/__tests__/providerCatalog.spec.ts +++ b/src/__tests__/providerCatalog.spec.ts @@ -164,6 +164,44 @@ describe("GET /:provider", () => { }); }); }); + + describe(`given the provider has a collection`, () => { + it("has a child link for that collection without query parameters", async function () { + this.timeout(5000); + + sandbox + .stub(Provider, "getProviders") + .resolves([null, [{ "provider-id": "TEST", "short-name": "TEST" }]]); + + const mockCollections = generateSTACCollections(1); + sandbox.stub(Collections, "getCollectionIds").resolves({ + count: mockCollections.length, + cursor: "foundCursor", + items: mockCollections.map((coll) => ({ + id: `${coll.shortName}_${coll.version}`, + title: coll.title ?? faker.random.words(4), + })), + }); + + const { body: catalog } = await request(stacApp).get("/stac/TEST?param=value"); + + const children = catalog.links.filter((l: Link) => l.rel === "child"); + expect(children).to.have.length(mockCollections.length); + + mockCollections.forEach((collection) => { + const childLink = children.find((l: Link) => l.href.endsWith(collection.id)); + + expect(childLink, JSON.stringify(children, null, 2)).to.have.property( + "type", + "application/json" + ); + expect(childLink.href, JSON.stringify(childLink, null, 2)).to.not.contain("?param=value"); + expect(childLink.href, JSON.stringify(childLink, null, 2)).to.match( + /^https?:\/\/.*TEST\/collections/ + ); + }); + }); + }); }); describe("given CMR providers endpoint responds with an error", () => { diff --git a/src/__tests__/rootCatalog.spec.ts b/src/__tests__/rootCatalog.spec.ts index 090c3acb..61a1c87c 100644 --- a/src/__tests__/rootCatalog.spec.ts +++ b/src/__tests__/rootCatalog.spec.ts @@ -55,7 +55,7 @@ describe("GET /stac", () => { }); describe("given CMR responds with providers", () => { - before(() => { + beforeEach(() => { sandbox .stub(Providers, "getProviders") .resolves([null, [{ "provider-id": "TEST", "short-name": "TEST" }]]); @@ -76,6 +76,23 @@ describe("GET /stac", () => { expect(providerLink.title).to.equal(provider["provider-id"]); }); }); + + it("should have an entry for each provider in the links without query parameters", async () => { + const { statusCode, body } = await request(app).get("/stac?param=value"); + + expect(statusCode).to.equal(200); + const [, expectedProviders] = cmrProvidersResponse; + + expectedProviders!.forEach((provider) => { + const providerLink = body.links.find((l: Link) => l.href.includes(provider["provider-id"])); + + expect(providerLink.href).to.match(/^(http)s?:\/\/.*\w+/); + expect(providerLink.href).to.not.contain("?param=value"); + expect(providerLink.rel).to.equal("child"); + expect(providerLink.type).to.equal("application/json"); + expect(providerLink.title).to.equal(provider["provider-id"]); + }); + }); }); describe("given CMR providers endpoint responds with an error", () => { From bf65b2f11e60299a26cf97de06edadb312f9cd39 Mon Sep 17 00:00:00 2001 From: William Valencia Date: Tue, 15 Oct 2024 16:27:52 -0400 Subject: [PATCH 3/7] CMR-147: Addressing PR comments, adding common getBaseUrl function --- src/__tests__/providerCatalog.spec.ts | 2 -- src/routes/browse.ts | 9 +++------ src/routes/catalog.ts | 4 ++-- src/routes/rootCatalog.ts | 4 ++-- src/utils/index.ts | 8 ++++++++ 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/__tests__/providerCatalog.spec.ts b/src/__tests__/providerCatalog.spec.ts index 6754a9cf..ba72ef5c 100644 --- a/src/__tests__/providerCatalog.spec.ts +++ b/src/__tests__/providerCatalog.spec.ts @@ -167,8 +167,6 @@ describe("GET /:provider", () => { describe(`given the provider has a collection`, () => { it("has a child link for that collection without query parameters", async function () { - this.timeout(5000); - sandbox .stub(Provider, "getProviders") .resolves([null, [{ "provider-id": "TEST", "short-name": "TEST" }]]); diff --git a/src/routes/browse.ts b/src/routes/browse.ts index 2ebb7d4f..8ccdad23 100644 --- a/src/routes/browse.ts +++ b/src/routes/browse.ts @@ -5,7 +5,7 @@ import { Links } from "../@types/StacCatalog"; import { getCollections } from "../domains/collections"; import { buildQuery, stringifyQuery } from "../domains/stac"; import { ItemNotFound } from "../models/errors"; -import { mergeMaybe, stacContext } from "../utils"; +import { getBaseUrl, mergeMaybe, stacContext } from "../utils"; const collectionLinks = (req: Request, nextCursor?: string | null): Links => { const { stacRoot, self, path } = stacContext(req); @@ -64,13 +64,10 @@ export const collectionsHandler = async (req: Request, res: Response): Promise { collection.links.push({ rel: "self", - href: `${baseUrl}/${encodeURIComponent(collection.id)}`, + href: `${getBaseUrl(self)}/${encodeURIComponent(collection.id)}`, type: "application/json", }); collection.links.push({ @@ -92,7 +89,7 @@ export const collectionsHandler = async (req: Request, res: Response): Promise { const selfLinks = generateSelfLinks(req); const childLinks = (collections ?? []).map(({ id, title }) => ({ rel: "child", - href: `${self.replace(/\?.*$/, "")}/collections/${encodeURIComponent(id)}`, + href: `${getBaseUrl(self)}/collections/${encodeURIComponent(id)}`, title, type: "application/json", })); diff --git a/src/routes/rootCatalog.ts b/src/routes/rootCatalog.ts index f36e4f3f..8a6d4405 100644 --- a/src/routes/rootCatalog.ts +++ b/src/routes/rootCatalog.ts @@ -3,7 +3,7 @@ import { Link, STACCatalog } from "../@types/StacCatalog"; import { conformance } from "../domains/providers"; import { Provider } from "../models/CmrModels"; -import { stacContext } from "../utils"; +import { getBaseUrl, stacContext } from "../utils"; const STAC_VERSION = process.env.STAC_VERSION ?? "1.0.0"; @@ -45,7 +45,7 @@ const providerLinks = (req: Request, providers: Provider[]): Link[] => { rel: "child", title, type: "application/json", - href: `${self.replace(/\?.*$/, "")}/${providerId}`, + href: `${getBaseUrl(self)}/${providerId}`, })); }; diff --git a/src/utils/index.ts b/src/utils/index.ts index a86fd5c1..18af1324 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -174,3 +174,11 @@ export const generatePossibleCollectionIds = (id: string, separator: string, rep return tokensCopy.join(separator); }); + +/** + * Returns the base URL + * This is used to remove Query Parameters that may have been added to the URL. + */ +export const getBaseUrl = (url: string) => { + return url.replace(/\?.*$/, ""); +}; From a6ccf81e13e78d9461e5ff7b75f5e0c6b3927dc9 Mon Sep 17 00:00:00 2001 From: William Valencia Date: Tue, 15 Oct 2024 16:31:45 -0400 Subject: [PATCH 4/7] CMR-10147: Adding timeout back to test as it fails in github --- src/__tests__/providerCatalog.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/__tests__/providerCatalog.spec.ts b/src/__tests__/providerCatalog.spec.ts index ba72ef5c..6754a9cf 100644 --- a/src/__tests__/providerCatalog.spec.ts +++ b/src/__tests__/providerCatalog.spec.ts @@ -167,6 +167,8 @@ describe("GET /:provider", () => { describe(`given the provider has a collection`, () => { it("has a child link for that collection without query parameters", async function () { + this.timeout(5000); + sandbox .stub(Provider, "getProviders") .resolves([null, [{ "provider-id": "TEST", "short-name": "TEST" }]]); From f75eac3ffc4397f9a039444323d260a2f325c824 Mon Sep 17 00:00:00 2001 From: William Valencia Date: Tue, 15 Oct 2024 16:57:58 -0400 Subject: [PATCH 5/7] CMR-10147: Adjusting test --- src/__tests__/providerCatalog.spec.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/__tests__/providerCatalog.spec.ts b/src/__tests__/providerCatalog.spec.ts index 6754a9cf..824be609 100644 --- a/src/__tests__/providerCatalog.spec.ts +++ b/src/__tests__/providerCatalog.spec.ts @@ -191,10 +191,6 @@ describe("GET /:provider", () => { mockCollections.forEach((collection) => { const childLink = children.find((l: Link) => l.href.endsWith(collection.id)); - expect(childLink, JSON.stringify(children, null, 2)).to.have.property( - "type", - "application/json" - ); expect(childLink.href, JSON.stringify(childLink, null, 2)).to.not.contain("?param=value"); expect(childLink.href, JSON.stringify(childLink, null, 2)).to.match( /^https?:\/\/.*TEST\/collections/ From e8d2db7242b3add083fe20c7b274e6a87a4187e2 Mon Sep 17 00:00:00 2001 From: William Valencia Date: Tue, 15 Oct 2024 17:26:06 -0400 Subject: [PATCH 6/7] CMR-10147: Fixing test to use entryId --- src/__tests__/providerCatalog.spec.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/__tests__/providerCatalog.spec.ts b/src/__tests__/providerCatalog.spec.ts index 37ef04b1..35e8275f 100644 --- a/src/__tests__/providerCatalog.spec.ts +++ b/src/__tests__/providerCatalog.spec.ts @@ -178,7 +178,7 @@ describe("GET /:provider", () => { count: mockCollections.length, cursor: "foundCursor", items: mockCollections.map((coll) => ({ - id: `${coll.shortName}_${coll.version}`, + id: `${coll.id}`, title: coll.title ?? faker.random.words(4), })), }); @@ -191,6 +191,10 @@ describe("GET /:provider", () => { mockCollections.forEach((collection) => { const childLink = children.find((l: Link) => l.href.endsWith(collection.id)); + expect(childLink, JSON.stringify(children, null, 2)).to.have.property( + "type", + "application/json" + ); expect(childLink.href, JSON.stringify(childLink, null, 2)).to.not.contain("?param=value"); expect(childLink.href, JSON.stringify(childLink, null, 2)).to.match( /^https?:\/\/.*TEST\/collections/ From 9e01d024e4e2694dfe5965d0de015eb606f37a51 Mon Sep 17 00:00:00 2001 From: William Valencia Date: Tue, 15 Oct 2024 17:28:13 -0400 Subject: [PATCH 7/7] CMR-10147: Removing timeout from test --- src/__tests__/providerCatalog.spec.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/__tests__/providerCatalog.spec.ts b/src/__tests__/providerCatalog.spec.ts index 35e8275f..6c8666a8 100644 --- a/src/__tests__/providerCatalog.spec.ts +++ b/src/__tests__/providerCatalog.spec.ts @@ -167,8 +167,6 @@ describe("GET /:provider", () => { describe(`given the provider has a collection`, () => { it("has a child link for that collection without query parameters", async function () { - this.timeout(5000); - sandbox .stub(Provider, "getProviders") .resolves([null, [{ "provider-id": "TEST", "short-name": "TEST" }]]);