From 2146a2fc34052b6f1cf7823446e56c1699b02629 Mon Sep 17 00:00:00 2001 From: Nathan Lisgo Date: Mon, 25 Sep 2023 11:59:18 +0100 Subject: [PATCH] feat: handle missing article version, update Dockerfile for dev setup (#840) * feat: handle missing article version, update Dockerfile for dev setup This commit updates the Dockerfile to include a setup for development environment. It also modifies the `getArticleVersion` function in `preprints-controller.ts`, `model.ts`, `mongodb-repository.ts`, and `mongodb-repository.test.ts` to handle cases where no matching article version is found. Instead of throwing an error, it now returns null, and handles the null case appropriately. * rename getArticleVersions to findArticleVersions for better semantics * move log to controller level where it is more apropriate * Alter the lines in docker-compose to match Dockerfile * add integration tests for invalid identifiers * add missing result parameter to test response --------- Co-authored-by: Will Byrne Co-authored-by: aisha --- Dockerfile | 3 +++ docker-compose.yaml | 3 +-- integration-tests/api.test.ts | 10 ++++++++ src/controller/preprints-controller.ts | 26 +++++++++++++------- src/model/model.ts | 2 +- src/model/mongodb/mongodb-repository.test.ts | 12 ++++----- src/model/mongodb/mongodb-repository.ts | 4 +-- 7 files changed, 40 insertions(+), 20 deletions(-) diff --git a/Dockerfile b/Dockerfile index b3cda508..27620c5e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -27,3 +27,6 @@ COPY src/ src/ EXPOSE 3000 CMD [ "yarn", "start" ] + +FROM prod as dev +CMD [ "yarn", "start:dev" ] diff --git a/docker-compose.yaml b/docker-compose.yaml index b07fc4d5..0cc54fee 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -60,8 +60,7 @@ services: api: build: context: . - target: prod - command: yarn start:dev + target: dev healthcheck: test: ["CMD-SHELL", "sh -c 'apk add curl; curl http://api:3000/'"] interval: 5s diff --git a/integration-tests/api.test.ts b/integration-tests/api.test.ts index 562b21c8..6f8cba93 100644 --- a/integration-tests/api.test.ts +++ b/integration-tests/api.test.ts @@ -969,6 +969,16 @@ describe('server tests', () => { }, }); }); + + it('returns a 404 when an invalid identifier is used', async () => { + const app = createApp(articleStore); + await request(app) + .get('/api/preprints/thisisnotanid') + .expect(404, { + result: false, + message: 'no result found for: (thisisnotanid)', + }); + }); }); describe('/api/citations/:publisherId/:articleId/bibtex', () => { diff --git a/src/controller/preprints-controller.ts b/src/controller/preprints-controller.ts index f4f26fbe..8655de9d 100644 --- a/src/controller/preprints-controller.ts +++ b/src/controller/preprints-controller.ts @@ -49,16 +49,24 @@ export const preprintsController = (repo: ArticleRepository) => { const getPreprintsByIdentifier = async (req: Request, res: Response, next: NextFunction) => { try { - const version = await repo.getArticleVersion(req.params.identifier); - const { msid, versionIdentifier } = version.article; - const pdfUrl = `https://github.com/elifesciences/enhanced-preprints-data/raw/master/data/${msid}/v${versionIdentifier}/${msid}-v${versionIdentifier}.pdf`; - try { - const { status } = await axios.get(pdfUrl); - if (status === 200) { - version.article.pdfUrl = pdfUrl; + const version = await repo.findArticleVersion(req.params.identifier); + if (!version) { + logger.info(`Cannot find a matching article version (${req.params.identifier})`); + res.status(404).send({ + result: false, + message: `no result found for: (${req.params.identifier})`, + }); + } else { + const { msid, versionIdentifier } = version.article; + const pdfUrl = `https://github.com/elifesciences/enhanced-preprints-data/raw/master/data/${msid}/v${versionIdentifier}/${msid}-v${versionIdentifier}.pdf`; + try { + const { status } = await axios.get(pdfUrl); + if (status === 200) { + version.article.pdfUrl = pdfUrl; + } + } finally { + res.send(version); } - } finally { - res.send(version); } } catch (err) { next(err); diff --git a/src/model/model.ts b/src/model/model.ts index 3b6898df..020382aa 100644 --- a/src/model/model.ts +++ b/src/model/model.ts @@ -156,7 +156,7 @@ export interface ArticleRepository { getArticle(id: string): Promise; getArticleSummaries(): Promise; storeEnhancedArticle(article: EnhancedArticle): Promise; - getArticleVersion(identifier: string): Promise; + findArticleVersion(identifier: string): Promise; getEnhancedArticleSummaries(): Promise; deleteArticleVersion(identifier: string): Promise; } diff --git a/src/model/mongodb/mongodb-repository.test.ts b/src/model/mongodb/mongodb-repository.test.ts index 4004df2b..0dbc86f6 100644 --- a/src/model/mongodb/mongodb-repository.test.ts +++ b/src/model/mongodb/mongodb-repository.test.ts @@ -235,8 +235,8 @@ describe('article-stores', () => { }])); }); - it('throws an error with unknown identifier', async () => { - expect(async () => articleStore.getArticleVersion('not-an-id')).rejects.toThrow(); + it('returns null with unknown identifier', async () => { + expect(await articleStore.findArticleVersion('not-an-id')).toBeNull(); }); it('stores and retrieves a Versioned Article by id with all fields', async () => { @@ -261,7 +261,7 @@ describe('article-stores', () => { }, }; const result = await articleStore.storeEnhancedArticle(inputArticle); - const article = await articleStore.getArticleVersion('testid1'); + const article = await articleStore.findArticleVersion('testid1'); expect(result).toStrictEqual(true); expect(article).toMatchObject({ @@ -303,7 +303,7 @@ describe('article-stores', () => { }, }; const result = await articleStore.storeEnhancedArticle(inputArticle); - const article = await articleStore.getArticleVersion('testid2'); + const article = await articleStore.findArticleVersion('testid2'); expect(result).toStrictEqual(true); expect(article).toMatchObject({ @@ -362,7 +362,7 @@ describe('article-stores', () => { }; const result1 = await articleStore.storeEnhancedArticle(inputArticle1); const result2 = await articleStore.storeEnhancedArticle(inputArticle2); - const article = await articleStore.getArticleVersion('testid3.1'); + const article = await articleStore.findArticleVersion('testid3.1'); expect(result1).toStrictEqual(true); expect(result2).toStrictEqual(true); @@ -432,7 +432,7 @@ describe('article-stores', () => { }; const result1 = await articleStore.storeEnhancedArticle(inputArticle1); const result2 = await articleStore.storeEnhancedArticle(inputArticle2); - const article = await articleStore.getArticleVersion('testid4'); + const article = await articleStore.findArticleVersion('testid4'); expect(result1).toStrictEqual(true); expect(result2).toStrictEqual(true); diff --git a/src/model/mongodb/mongodb-repository.ts b/src/model/mongodb/mongodb-repository.ts index 71e557bb..ea2a44b9 100644 --- a/src/model/mongodb/mongodb-repository.ts +++ b/src/model/mongodb/mongodb-repository.ts @@ -128,7 +128,7 @@ class MongoDBArticleRepository implements ArticleRepository { })); } - async getArticleVersion(identifier: string): Promise { + async findArticleVersion(identifier: string): Promise { const version = await this.versionedCollection.findOne( { $or: [{ _id: identifier }, { msid: identifier }] }, { @@ -140,7 +140,7 @@ class MongoDBArticleRepository implements ArticleRepository { ); if (!version) { - throw Error('Cannot find a matching article version'); + return null; } const allVersions = await this.versionedCollection.find(