CCM-14318: Endpoints fetching letters return 500 instead of 404 when letter not found for supplier (#485)

francisco-videira-nhs · web-flow · commit cd571a2d5381 · 2026-04-09T10:31:03.000+01:00
* Refactor handling of letter not found

* fix tests

* fix another test

* remove e2e test 500 not found

* fix import

* try

* fix

* remove regex from vale accepted

* lint and editor
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
@@ -27,6 +27,12 @@
 - [ ] This PR is a result of pair or mob programming
 <!-- - [ ] If I have used the 'skip-trivy-package' label I have done so responsibly and in the knowledge that this is being fixed as part of a separate ticket/PR. TODO - Re-visit Trivy usage https://nhsd-jira.digital.nhs.uk/browse/CCM-15549 -->
 
+## DT3-Specific Checklist
+
+<!-- Go over all the following points, and put an `x` in all the boxes that apply. -->
+
+- [ ] If I have added a new resource (SQS, Lambda, Gateway, DDB table, etc), I have created the appropriate alarms
+
 ---
 
 ## Sensitive Information Declaration
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -10,5 +10,6 @@
     "**/Thumbs.db": true,
     ".github": false,
     ".vscode": false
-  }
+  },
+  "js/ts.tsdk.path": "node_modules/typescript/lib"
 }
diff --git a/internal/datastore/src/__test__/letter-queue-repository.test.ts b/internal/datastore/src/__test__/letter-queue-repository.test.ts
@@ -8,9 +8,9 @@ import {
 } from "./db";
 import LetterQueueRepository from "../letter-queue-repository";
 import { InsertPendingLetter } from "../types";
-import { LetterAlreadyExistsError } from "../letter-already-exists-error";
+import LetterAlreadyExistsError from "../errors/letter-already-exists-error";
 import { createTestLogger } from "./logs";
-import { LetterDoesNotExistError } from "../letter-does-not-exist-error";
+import LetterNotFoundError from "../errors/letter-not-found-error";
 
 function createLetter(
   overrides: Partial<InsertPendingLetter> = {},
@@ -133,7 +133,7 @@ describe("LetterQueueRepository", () => {
     it("throws an error when the letter does not exist", async () => {
       await expect(
         letterQueueRepository.deleteLetter("supplier1", "letter1"),
-      ).rejects.toThrow(LetterDoesNotExistError);
+      ).rejects.toThrow(LetterNotFoundError);
     });
 
     it("rethrows errors from DynamoDB when deleting a letter", async () => {
diff --git a/internal/datastore/src/__test__/letter-repository.test.ts b/internal/datastore/src/__test__/letter-repository.test.ts
@@ -8,7 +8,7 @@ import {
 import { LetterRepository } from "../letter-repository";
 import { InsertLetter, Letter, UpdateLetter } from "../types";
 import { createTestLogger } from "./logs";
-import { LetterAlreadyExistsError } from "../letter-already-exists-error";
+import LetterAlreadyExistsError from "../errors/letter-already-exists-error";
 
 function createLetter(
   supplierId: string,
@@ -140,7 +140,7 @@ describe("LetterRepository", () => {
     await expect(
       letterRepository.getLetterById("supplier1", "letter1"),
     ).rejects.toThrow(
-      "Letter with id letter1 not found for supplier supplier1",
+      "Letter not found: supplierId=supplier1, letterId=letter1",
     );
   });
 
diff --git a/internal/datastore/src/errors/letter-already-exists-error.ts b/internal/datastore/src/errors/letter-already-exists-error.ts
@@ -1,8 +1,7 @@
 /**
  * Error thrown when attempting to create a letter that already exists in the database.
  */
-// eslint-disable-next-line import-x/prefer-default-export
-export class LetterAlreadyExistsError extends Error {
+export default class LetterAlreadyExistsError extends Error {
   constructor(
     public readonly supplierId: string,
     public readonly letterId: string,
diff --git a/internal/datastore/src/errors/letter-not-found-error.ts b/internal/datastore/src/errors/letter-not-found-error.ts
@@ -0,0 +1,12 @@
+/**
+ * Error thrown when a letter is not found in the database.
+ */
+export default class LetterNotFoundError extends Error {
+  constructor(
+    public readonly supplierId: string,
+    public readonly letterId: string,
+  ) {
+    super(`Letter not found: supplierId=${supplierId}, letterId=${letterId}`);
+    this.name = "LetterNotFoundError";
+  }
+}
diff --git a/internal/datastore/src/index.ts b/internal/datastore/src/index.ts
@@ -1,9 +1,9 @@
 export * from "./types";
-export * from "./letter-already-exists-error";
-export * from "./letter-does-not-exist-error";
 export * from "./mi-repository";
 export * from "./letter-repository";
 export * from "./supplier-repository";
 export * from "./supplier-config-repository";
 export { default as LetterQueueRepository } from "./letter-queue-repository";
 export { default as DBHealthcheck } from "./healthcheck";
+export { default as LetterAlreadyExistsError } from "./errors/letter-already-exists-error";
+export { default as LetterNotFoundError } from "./errors/letter-not-found-error";
diff --git a/internal/datastore/src/letter-does-not-exist-error.ts b/internal/datastore/src/letter-does-not-exist-error.ts
diff --git a/internal/datastore/src/letter-queue-repository.ts b/internal/datastore/src/letter-queue-repository.ts
@@ -13,8 +13,8 @@ import {
   PendingLetterBase,
   PendingLetterSchema,
 } from "./types";
-import { LetterAlreadyExistsError } from "./letter-already-exists-error";
-import { LetterDoesNotExistError } from "./letter-does-not-exist-error";
+import LetterAlreadyExistsError from "./errors/letter-already-exists-error";
+import LetterNotFoundError from "./errors/letter-not-found-error";
 
 type LetterQueueRepositoryConfig = {
   letterQueueTableName: string;
@@ -87,7 +87,7 @@ export default class LetterQueueRepository {
         error instanceof Error &&
         error.name === "ConditionalCheckFailedException"
       ) {
-        throw new LetterDoesNotExistError(supplierId, letterId);
+        throw new LetterNotFoundError(supplierId, letterId);
       }
       throw error;
     }
diff --git a/internal/datastore/src/letter-repository.ts b/internal/datastore/src/letter-repository.ts
@@ -9,7 +9,8 @@ import {
 import { ConditionalCheckFailedException } from "@aws-sdk/client-dynamodb";
 import { Logger } from "pino";
 import { InsertLetter, Letter, LetterSchema, UpdateLetter } from "./types";
-import { LetterAlreadyExistsError } from "./letter-already-exists-error";
+import LetterNotFoundError from "./errors/letter-not-found-error";
+import LetterAlreadyExistsError from "./errors/letter-already-exists-error";
 
 export type PagingOptions = Partial<{
   exclusiveStartKey: Record<string, any>;
@@ -103,9 +104,7 @@ export class LetterRepository {
     );
 
     if (!result.Item) {
-      throw new Error(
-        `Letter with id ${letterId} not found for supplier ${supplierId}`,
-      );
+      throw new LetterNotFoundError(supplierId, letterId);
     }
     return LetterSchema.parse(result.Item);
   }
diff --git a/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts b/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts
@@ -3,6 +3,7 @@ import pino from "pino";
 import { getSignedUrl } from "@aws-sdk/s3-request-presigner";
 import { S3Client } from "@aws-sdk/client-s3";
 import { SQSClient } from "@aws-sdk/client-sqs";
+import LetterNotFoundError from "@internal/datastore/src/errors/letter-not-found-error";
 import {
   enqueueLetterUpdateRequests,
   getLetterById,
@@ -124,13 +125,11 @@ describe("getLetterById", () => {
     const mockRepo = {
       getLetterById: jest
         .fn()
-        .mockRejectedValue(
-          new Error("Letter with id l1 not found for supplier s1"),
-        ),
+        .mockRejectedValue(new LetterNotFoundError("supplier1", "letter1")),
     };
 
     await expect(
-      getLetterById("supplierid", "letter1", mockRepo as any),
+      getLetterById("supplier1", "letter1", mockRepo as any),
     ).rejects.toThrow("No resource found with that ID");
   });
 
@@ -191,9 +190,7 @@ describe("getLetterDataUrl function", () => {
     deps.letterRepo = {
       getLetterById: jest
         .fn()
-        .mockRejectedValue(
-          new Error("Letter with id l1 not found for supplier s1"),
-        ),
+        .mockRejectedValue(new LetterNotFoundError("supplier1", "letter42")),
     } as unknown as LetterRepository;
 
     await expect(
diff --git a/lambdas/api-handler/src/services/letter-operations.ts b/lambdas/api-handler/src/services/letter-operations.ts
@@ -7,18 +7,12 @@ import {
 import { getSignedUrl } from "@aws-sdk/s3-request-presigner";
 import { GetObjectCommand, S3Client } from "@aws-sdk/client-s3";
 import { SendMessageBatchCommand } from "@aws-sdk/client-sqs";
+import LetterNotFoundError from "@internal/datastore/src/errors/letter-not-found-error";
 import NotFoundError from "../errors/not-found-error";
 import { UpdateLetterCommand } from "../contracts/letters";
 import { ApiErrorDetail } from "../contracts/errors";
 import { Deps } from "../config/deps";
 
-function isNotFoundError(error: any) {
-  return (
-    error instanceof Error &&
-    /^Letter with id \w+ not found for supplier \w+$/.test(error.message)
-  );
-}
-
 async function getDownloadUrl(
   s3Uri: string,
   s3Client: S3Client,
@@ -79,7 +73,7 @@ export const getLetterById = async (
   try {
     letter = await letterRepo.getLetterById(supplierId, letterId);
   } catch (error) {
-    if (isNotFoundError(error)) {
+    if (error instanceof LetterNotFoundError) {
       throw new NotFoundError(ApiErrorDetail.NotFoundLetterId);
     }
     throw error;
@@ -103,7 +97,7 @@ export const getLetterDataUrl = async (
       deps.env.DOWNLOAD_URL_TTL_SECONDS,
     );
   } catch (error) {
-    if (isNotFoundError(error)) {
+    if (error instanceof LetterNotFoundError) {
       throw new NotFoundError(ApiErrorDetail.NotFoundLetterId);
     }
     throw error;
diff --git a/lambdas/update-letter-queue/src/__tests__/update-letter-queue.test.ts b/lambdas/update-letter-queue/src/__tests__/update-letter-queue.test.ts
@@ -1,7 +1,7 @@
 import {
   Letter,
   LetterAlreadyExistsError,
-  LetterDoesNotExistError,
+  LetterNotFoundError,
   LetterQueueRepository,
 } from "@internal/datastore";
 import { mockDeep } from "jest-mock-extended";
@@ -192,7 +192,7 @@ describe("update-letter-queue Lambda", () => {
       const newLetter1 = generateLetter("ACCEPTED", { id: "1" });
       const newLetter2 = generateLetter("ACCEPTED", { id: "2" });
       (mockedDeps.letterQueueRepository.deleteLetter as jest.Mock)
-        .mockRejectedValueOnce(new LetterDoesNotExistError("supplier1", "1"))
+        .mockRejectedValueOnce(new LetterNotFoundError("supplier1", "1"))
         .mockResolvedValueOnce({});
 
       const testData = generateKinesisEvent([
@@ -338,7 +338,7 @@ describe("update-letter-queue Lambda", () => {
       const newLetter1 = generateLetter("ACCEPTED", { id: "1" });
       const newLetter2 = generateLetter("ACCEPTED", { id: "2" });
       (mockedDeps.letterQueueRepository.deleteLetter as jest.Mock)
-        .mockRejectedValueOnce(new LetterDoesNotExistError("supplier1", "1"))
+        .mockRejectedValueOnce(new LetterNotFoundError("supplier1", "1"))
         .mockResolvedValueOnce({});
 
       const testData = generateKinesisEvent([
diff --git a/lambdas/update-letter-queue/src/update-letter-queue.ts b/lambdas/update-letter-queue/src/update-letter-queue.ts
@@ -11,7 +11,7 @@ import {
   InsertPendingLetter,
   Letter,
   LetterAlreadyExistsError,
-  LetterDoesNotExistError,
+  LetterNotFoundError,
   LetterSchema,
 } from "@internal/datastore";
 import { Deps } from "./deps";
@@ -105,7 +105,7 @@ async function deletePendingLetterFromQueue(
     await deps.letterQueueRepository.deleteLetter(letter.supplierId, letter.id);
     return 1;
   } catch (error) {
-    if (error instanceof LetterDoesNotExistError) {
+    if (error instanceof LetterNotFoundError) {
       deps.logger.warn({
         description: "Letter does not exist",
         supplierId: letter.supplierId,
diff --git a/scripts/config/vale/styles/config/vocabularies/words/accept.txt b/scripts/config/vale/styles/config/vocabularies/words/accept.txt
@@ -1,6 +1,5 @@
 actioned
 API|api
-[A-Z]+s
 Bitwarden
 bot
 Cognito
diff --git a/tests/component-tests/apiGateway-tests/get-letter-pdf.spec.ts b/tests/component-tests/apiGateway-tests/get-letter-pdf.spec.ts
@@ -7,10 +7,7 @@ import {
   SUPPLIER_LETTERS,
 } from "../../constants/api-constants";
 import { createValidRequestHeaders } from "../../constants/request-headers";
-import {
-  error404ResponseBody,
-  error500ResponseBody,
-} from "../../helpers/common-types";
+import { error404ResponseBody } from "../../helpers/common-types";
 
 let baseUrl: string;
 
@@ -84,7 +81,7 @@ test.describe("API Gateway Tests to Verify Get Letter PDF Endpoint", () => {
   });
 
   // CCM-14318: Remove this test
-  test(`Get /letters/{id}/data returns 500 if letter is not found for supplierId ${SUPPLIERID}`, async ({
+  test(`Get /letters/{id}/data returns 404 if letter is not found for supplierId ${SUPPLIERID}`, async ({
     request,
   }) => {
     const id = "non-existing-id-12345";
@@ -98,7 +95,7 @@ test.describe("API Gateway Tests to Verify Get Letter PDF Endpoint", () => {
     );
 
     const responseBody = await response.json();
-    expect(response.status()).toBe(500);
-    expect(responseBody).toMatchObject(error500ResponseBody());
+    expect(response.status()).toBe(404);
+    expect(responseBody).toMatchObject(error404ResponseBody());
   });
 });
diff --git a/tests/component-tests/apiGateway-tests/get-letter-status.spec.ts b/tests/component-tests/apiGateway-tests/get-letter-status.spec.ts
@@ -3,10 +3,7 @@ import getRestApiGatewayBaseUrl from "../../helpers/aws-gateway-helper";
 import { getLettersBySupplier } from "../../helpers/generate-fetch-test-data";
 import { SUPPLIERID, SUPPLIER_LETTERS } from "../../constants/api-constants";
 import { createValidRequestHeaders } from "../../constants/request-headers";
-import {
-  error404ResponseBody,
-  error500ResponseBody,
-} from "../../helpers/common-types";
+import { error404ResponseBody } from "../../helpers/common-types";
 
 let baseUrl: string;
 
@@ -49,10 +46,10 @@ test.describe("API Gateway Tests to Verify Get Letter Status Endpoint", () => {
     });
   });
 
-  test(`Get /letters/{id} returns 404 if no resource is found for id`, async ({
+  test(`Get /letters/{id} returns 404 if letter is not found for supplierId ${SUPPLIERID}`, async ({
     request,
   }) => {
-    const id = "11";
+    const id = "non-existing-id-12345";
     const headers = createValidRequestHeaders();
     const response = await request.get(`${baseUrl}/${SUPPLIER_LETTERS}/${id}`, {
       headers,
@@ -62,18 +59,4 @@ test.describe("API Gateway Tests to Verify Get Letter Status Endpoint", () => {
     expect(response.status()).toBe(404);
     expect(responseBody).toMatchObject(error404ResponseBody());
   });
-
-  test(`Get /letters/{id} returns 500 if letter is not found for supplierId ${SUPPLIERID}`, async ({
-    request,
-  }) => {
-    const id = "non-existing-id-12345";
-    const headers = createValidRequestHeaders();
-    const response = await request.get(`${baseUrl}/${SUPPLIER_LETTERS}/${id}`, {
-      headers,
-    });
-
-    const responseBody = await response.json();
-    expect(response.status()).toBe(500);
-    expect(responseBody).toMatchObject(error500ResponseBody());
-  });
 });
diff --git a/tests/component-tests/events-tests/event-subscription.spec.ts b/tests/component-tests/events-tests/event-subscription.spec.ts
@@ -96,8 +96,8 @@ test.describe("Event Subscription SNS Tests", () => {
         headers,
       },
     );
+    expect(getLetterResponse.status()).toBe(404);
 
-    expect(getLetterResponse.status()).toBe(500);
     await pollUpsertLetterLogForError(
       "Message did not match an expected schema",
       domainId,
diff --git a/tests/e2e-tests/api/data/test_get_letter_data.py b/tests/e2e-tests/api/data/test_get_letter_data.py
@@ -45,15 +45,3 @@ def test_404_letter_does_not_exist(url, bearer_token):
     ErrorHandler.handle_retry(get_message_response)
     assert get_message_response.status_code == 404
     assert get_message_response.json().get("errors")[0].get("detail") == "No resource found with that ID"
-
-@pytest.mark.test
-@pytest.mark.devtest
-@pytest.mark.inttest
-@pytest.mark.prodtest
-def test_500_letter_does_not_exist(url, bearer_token):
-    letter_id = "00000000-0000-0000-0000-000000000000"
-    headers = Generators.generate_valid_headers(bearer_token.value)
-    get_message_response = requests.get(f"{url}/{LETTERS_ENDPOINT}/{letter_id}/data", headers=headers)
-
-    ErrorHandler.handle_retry(get_message_response)
-    assert get_message_response.status_code == 500

Original file line number	Diff line number	Diff line change
`@@ -10,5 +10,6 @@`
`10`	`10`	`"**/Thumbs.db": true,`
`11`	`11`	`".github": false,`
`12`	`12`	`".vscode": false`
`13`		`- }`
	`13`	`+ },`
	`14`	`+ "js/ts.tsdk.path": "node_modules/typescript/lib"`
`14`	`15`	`}`