From 2ac63f36b8887516fedbff0e13d9b871684896c5 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Mon, 13 Oct 2025 10:02:35 +0100 Subject: [PATCH 1/2] Added get status endpoint for single letter --- .../terraform/components/api/README.md | 1 + .../iam_role_api_gateway_execution_role.tf | 1 + .../terraform/components/api/locals.tf | 1 + .../api/module_lambda_get_letter.tf | 69 ++++++++ .../components/api/resources/spec.tmpl.json | 37 +++++ lambdas/api-handler/src/contracts/letters.ts | 10 +- .../src/handlers/__tests__/get-letter.test.ts | 151 ++++++++++++++++++ .../api-handler/src/handlers/get-letter.ts | 49 ++++++ lambdas/api-handler/src/index.ts | 1 + .../mappers/__tests__/letter-mapper.test.ts | 69 +++++++- .../api-handler/src/mappers/letter-mapper.ts | 38 +++-- .../__tests__/letter-operations.test.ts | 44 ++++- .../src/services/letter-operations.ts | 21 ++- sdk/_config.version.yml | 2 +- server/_config.version.yml | 2 +- src/server/_config.version.yml | 2 +- 16 files changed, 474 insertions(+), 24 deletions(-) create mode 100644 infrastructure/terraform/components/api/module_lambda_get_letter.tf create mode 100644 lambdas/api-handler/src/handlers/__tests__/get-letter.test.ts create mode 100644 lambdas/api-handler/src/handlers/get-letter.ts diff --git a/infrastructure/terraform/components/api/README.md b/infrastructure/terraform/components/api/README.md index fd16d4e6a..2f1c9e241 100644 --- a/infrastructure/terraform/components/api/README.md +++ b/infrastructure/terraform/components/api/README.md @@ -34,6 +34,7 @@ No requirements. |------|--------|---------| | [authorizer\_lambda](#module\_authorizer\_lambda) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-lambda.zip | n/a | | [domain\_truststore](#module\_domain\_truststore) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-s3bucket.zip | n/a | +| [get\_letter](#module\_get\_letter) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.24/terraform-lambda.zip | n/a | | [get\_letters](#module\_get\_letters) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-lambda.zip | n/a | | [kms](#module\_kms) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-kms.zip | n/a | | [logging\_bucket](#module\_logging\_bucket) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-s3bucket.zip | n/a | diff --git a/infrastructure/terraform/components/api/iam_role_api_gateway_execution_role.tf b/infrastructure/terraform/components/api/iam_role_api_gateway_execution_role.tf index 4ceca3cd4..879f5a9b9 100644 --- a/infrastructure/terraform/components/api/iam_role_api_gateway_execution_role.tf +++ b/infrastructure/terraform/components/api/iam_role_api_gateway_execution_role.tf @@ -49,6 +49,7 @@ data "aws_iam_policy_document" "api_gateway_execution_policy" { resources = [ module.authorizer_lambda.function_arn, + module.get_letter.function_arn, module.get_letters.function_arn, module.patch_letter.function_arn ] diff --git a/infrastructure/terraform/components/api/locals.tf b/infrastructure/terraform/components/api/locals.tf index 982bc1104..ced10a7d5 100644 --- a/infrastructure/terraform/components/api/locals.tf +++ b/infrastructure/terraform/components/api/locals.tf @@ -8,6 +8,7 @@ locals { APIG_EXECUTION_ROLE_ARN = aws_iam_role.api_gateway_execution_role.arn AWS_REGION = var.region AUTHORIZER_LAMBDA_ARN = module.authorizer_lambda.function_arn + GET_LETTER_LAMBDA_ARN = module.get_letter.function_arn GET_LETTERS_LAMBDA_ARN = module.get_letters.function_arn PATCH_LETTER_LAMBDA_ARN = module.patch_letter.function_arn }) diff --git a/infrastructure/terraform/components/api/module_lambda_get_letter.tf b/infrastructure/terraform/components/api/module_lambda_get_letter.tf new file mode 100644 index 000000000..a92a60515 --- /dev/null +++ b/infrastructure/terraform/components/api/module_lambda_get_letter.tf @@ -0,0 +1,69 @@ +module "get_letter" { + source = "https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.24/terraform-lambda.zip" + + function_name = "get_letter" + description = "Get letter status" + + aws_account_id = var.aws_account_id + component = var.component + environment = var.environment + project = var.project + region = var.region + group = var.group + + log_retention_in_days = var.log_retention_in_days + kms_key_arn = module.kms.key_arn + + iam_policy_document = { + body = data.aws_iam_policy_document.get_letter_lambda.json + } + + function_s3_bucket = local.acct.s3_buckets["lambda_function_artefacts"]["id"] + function_code_base_path = local.aws_lambda_functions_dir_path + function_code_dir = "api-handler/dist" + function_include_common = true + handler_function_name = "getLetter" + runtime = "nodejs22.x" + memory = 128 + timeout = 5 + log_level = var.log_level + + force_lambda_code_deploy = var.force_lambda_code_deploy + enable_lambda_insights = false + + send_to_firehose = true + log_destination_arn = local.destination_arn + log_subscription_role_arn = local.acct.log_subscription_role_arn + + lambda_env_vars = merge(local.common_lambda_env_vars, {}) +} + +data "aws_iam_policy_document" "get_letter_lambda" { + statement { + sid = "KMSPermissions" + effect = "Allow" + + actions = [ + "kms:Decrypt", + "kms:GenerateDataKey", + ] + + resources = [ + module.kms.key_arn, ## Requires shared kms module + ] + } + + statement { + sid = "AllowDynamoDBAccess" + effect = "Allow" + + actions = [ + "dynamodb:GetItem", + "dynamodb:Query" + ] + + resources = [ + aws_dynamodb_table.letters.arn + ] + } +} diff --git a/infrastructure/terraform/components/api/resources/spec.tmpl.json b/infrastructure/terraform/components/api/resources/spec.tmpl.json index ec58b67e4..2e5924da5 100644 --- a/infrastructure/terraform/components/api/resources/spec.tmpl.json +++ b/infrastructure/terraform/components/api/resources/spec.tmpl.json @@ -54,6 +54,43 @@ } }, "/letters/{id}": { + "get": { + "description": "Returns 200 OK with letter status.", + "responses": { + "200": { + "description": "OK" + }, + "400": { + "description": "Bad request, invalid input data" + }, + "404": { + "description": "Resource not found" + }, + "500": { + "description": "Server error" + } + }, + "security": [ + { + "LambdaAuthorizer": [] + } + ], + "summary": "Get letter", + "x-amazon-apigateway-integration": { + "contentHandling": "CONVERT_TO_TEXT", + "credentials": "${APIG_EXECUTION_ROLE_ARN}", + "httpMethod": "POST", + "passthroughBehavior": "WHEN_NO_TEMPLATES", + "responses": { + ".*": { + "statusCode": "200" + } + }, + "timeoutInMillis": 29000, + "type": "AWS_PROXY", + "uri": "arn:aws:apigateway:${AWS_REGION}:lambda:path/2015-03-31/functions/${GET_LETTER_LAMBDA_ARN}/invocations" + } + }, "parameters": [ { "description": "Unique identifier of this resource", diff --git a/lambdas/api-handler/src/contracts/letters.ts b/lambdas/api-handler/src/contracts/letters.ts index 26672c14a..d777018ac 100644 --- a/lambdas/api-handler/src/contracts/letters.ts +++ b/lambdas/api-handler/src/contracts/letters.ts @@ -36,7 +36,7 @@ export const PatchLetterRequestResourceSchema = z.object({ }).strict() }).strict(); -export const PatchLetterResponseResourceSchema = z.object({ +export const GetLetterResponseResourceSchema = z.object({ id: z.string(), type: z.literal('Letter'), attributes: z.object({ @@ -58,12 +58,16 @@ export const GetLettersResponseResourceSchema = z.object({ }).strict() }).strict(); +export const PatchLetterResponseResourceSchema = GetLetterResponseResourceSchema; + export type LetterStatus = z.infer; export const PatchLetterRequestSchema = makeDocumentSchema(PatchLetterRequestResourceSchema); -export const PatchLetterResponseSchema = makeDocumentSchema(PatchLetterResponseResourceSchema); +export const GetLetterResponseSchema = makeDocumentSchema(GetLetterResponseResourceSchema); export const GetLettersResponseSchema = makeCollectionSchema(GetLettersResponseResourceSchema); +export const PatchLetterResponseSchema = makeDocumentSchema(PatchLetterResponseResourceSchema); export type PatchLetterRequest = z.infer; -export type PatchLetterResponse = z.infer; +export type GetLetterResponse = z.infer; export type GetLettersResponse = z.infer; +export type PatchLetterResponse = z.infer; diff --git a/lambdas/api-handler/src/handlers/__tests__/get-letter.test.ts b/lambdas/api-handler/src/handlers/__tests__/get-letter.test.ts new file mode 100644 index 000000000..9590200d4 --- /dev/null +++ b/lambdas/api-handler/src/handlers/__tests__/get-letter.test.ts @@ -0,0 +1,151 @@ +import { Context } from 'aws-lambda'; +import { mockDeep } from 'jest-mock-extended'; +import * as letterService from '../../services/letter-operations'; +import { makeApiGwEvent } from './utils/test-utils'; +import { getLetter } from '../../index'; +import { ApiErrorDetail } from '../../contracts/errors'; +import { NotFoundError } from '../../errors'; + +jest.mock('../../services/letter-operations'); + +jest.mock('../../config/lambda-config', () => ({ + lambdaConfig: { + SUPPLIER_ID_HEADER: 'nhsd-supplier-id', + APIM_CORRELATION_HEADER: 'nhsd-correlation-id' + } +})); + +describe('API Lambda handler', () => { + + beforeEach(() => { + jest.clearAllMocks(); + jest.resetModules(); + }); + + it('returns 200 OK and the letter status', async () => { + + const mockedGetLetterById = letterService.getLetterById as jest.Mock; + mockedGetLetterById.mockResolvedValue({ + id: 'id1', + specificationId: 'spec1', + groupId: 'group1', + status: 'PENDING' + }); + + const event = makeApiGwEvent({path: '/letters/id1', + headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId'}, + pathParameters: {id: 'id1'}}); + + const result = await getLetter(event, mockDeep(), jest.fn()); + + const expected = { + data: { + id: 'id1', + type: 'Letter', + attributes: { + status: 'PENDING', + specificationId: 'spec1', + groupId: 'group1' + } + } + }; + + expect(result).toEqual({ + statusCode: 200, + body: JSON.stringify(expected, null, 2), + }); + }); + + it('includes the reason code and reason text if present', async () => { + + const mockedGetLetterById = letterService.getLetterById as jest.Mock; + mockedGetLetterById.mockResolvedValue({ + id: 'id1', + specificationId: 'spec1', + groupId: 'group1', + status: 'FAILED', + reasonCode: 100, + reasonText: 'failed validation' + }); + + const event = makeApiGwEvent({path: '/letters/id1', + headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId'}, + pathParameters: {id: 'id1'}}); + + const result = await getLetter(event, mockDeep(), jest.fn()); + + + const expected = { + data: { + id: 'id1', + type: 'Letter', + attributes: { + status: 'FAILED', + specificationId: 'spec1', + groupId: 'group1', + reasonCode: 100, + reasonText: 'failed validation' + } + } + }; + + expect(result).toEqual({ + statusCode: 200, + body: JSON.stringify(expected, null, 2), + }); + }); + + it('returns 404 Not Found when letter matching id is not found', async () => { + + const mockedGetLetterById = letterService.getLetterById as jest.Mock; + mockedGetLetterById.mockImplementation(() => { + throw new NotFoundError(ApiErrorDetail.NotFoundLetterId); + }); + + const event = makeApiGwEvent({path: '/letters/id1', + headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId'}, + pathParameters: {id: 'id1'}}); + + const result = await getLetter(event, mockDeep(), jest.fn()); + + expect(result).toEqual(expect.objectContaining({ + statusCode: 404, + })); + }); + + it ('returns 500 when correlation id is missing from header', async() => { + const event = makeApiGwEvent({path: '/letters/id1', + headers: {'nhsd-supplier-id': 'supplier1'}, + pathParameters: {id: 'id1'}}); + + const result = await getLetter(event, mockDeep(), jest.fn()); + + expect(result).toEqual(expect.objectContaining({ + statusCode: 500, + })); + }); + + it ('returns 400 when supplier id is missing from header', async() => { + const event = makeApiGwEvent({path: '/letters/id1', + headers: {'nhsd-correlation-id': 'correlationId'}, + pathParameters: {id: 'id1'}}); + + const result = await getLetter(event, mockDeep(), jest.fn()); + + expect(result).toEqual(expect.objectContaining({ + statusCode: 400, + })); + }); + + + it ('returns 400 when letter id is missing from path', async() => { + const event = makeApiGwEvent({path: '/letters/id1', + headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId'}}); + + const result = await getLetter(event, mockDeep(), jest.fn()); + + expect(result).toEqual(expect.objectContaining({ + statusCode: 400, + })); + }); +}); diff --git a/lambdas/api-handler/src/handlers/get-letter.ts b/lambdas/api-handler/src/handlers/get-letter.ts new file mode 100644 index 000000000..7e349a648 --- /dev/null +++ b/lambdas/api-handler/src/handlers/get-letter.ts @@ -0,0 +1,49 @@ +import pino from "pino"; +import { createLetterRepository } from "../infrastructure/letter-repo-factory"; +import { APIGatewayProxyHandler } from "aws-lambda"; +import { assertNotEmpty, lowerCaseKeys } from "../utils/validation"; +import { lambdaConfig } from "../config/lambda-config"; +import { ValidationError } from "../errors"; +import { ApiErrorDetail } from "../contracts/errors"; +import { getLetterById } from "../services/letter-operations"; +import { mapErrorToResponse } from "../mappers/error-mapper"; +import { mapToGetLetterResponse } from "../mappers/letter-mapper"; + + +const letterRepo = createLetterRepository(); +const log = pino(); + +export const getLetter: APIGatewayProxyHandler = async (event) => { + + let correlationId; + try { + assertNotEmpty(event.headers, new Error("The request headers are empty")); + const lowerCasedHeaders = lowerCaseKeys(event.headers); + + correlationId = assertNotEmpty(lowerCasedHeaders[lambdaConfig.APIM_CORRELATION_HEADER], + new Error("The request headers don't contain the APIM correlation id")); + + const supplierId = assertNotEmpty(lowerCasedHeaders[lambdaConfig.SUPPLIER_ID_HEADER], + new ValidationError(ApiErrorDetail.InvalidRequestMissingSupplierId)); + + const letterId = assertNotEmpty(event.pathParameters?.id, new ValidationError(ApiErrorDetail.InvalidRequestMissingLetterIdPathParameter)); + + const letter = await getLetterById(supplierId, letterId, letterRepo); + + const response = mapToGetLetterResponse(letter); + + log.info({ + description: 'Letter successfully fetched by id', + supplierId, + letterId + }); + + return { + statusCode: 200, + body: JSON.stringify(response, null, 2), + }; + } catch (error) + { + return mapErrorToResponse(error, correlationId); + } +} diff --git a/lambdas/api-handler/src/index.ts b/lambdas/api-handler/src/index.ts index 203102f73..14e88dd6b 100644 --- a/lambdas/api-handler/src/index.ts +++ b/lambdas/api-handler/src/index.ts @@ -1,3 +1,4 @@ // Export all handlers for ease of access +export { getLetter } from './handlers/get-letter'; export { getLetters } from './handlers/get-letters'; export { patchLetter } from './handlers/patch-letter'; diff --git a/lambdas/api-handler/src/mappers/__tests__/letter-mapper.test.ts b/lambdas/api-handler/src/mappers/__tests__/letter-mapper.test.ts index 96548cda8..b63825dc8 100644 --- a/lambdas/api-handler/src/mappers/__tests__/letter-mapper.test.ts +++ b/lambdas/api-handler/src/mappers/__tests__/letter-mapper.test.ts @@ -1,6 +1,6 @@ -import { mapToGetLettersResponse, mapToPatchLetterResponse } from '../letter-mapper'; +import { mapToGetLetterResponse, mapToGetLettersResponse, mapToPatchLetterResponse } from '../letter-mapper'; import { Letter } from '../../../../../internal/datastore'; -import { GetLettersResponse, PatchLetterResponse } from '../../contracts/letters'; +import { GetLetterResponse, GetLettersResponse, PatchLetterResponse } from '../../contracts/letters'; describe('letter-mapper', () => { it('maps an internal Letter to a PatchLetterResponse', () => { @@ -67,6 +67,71 @@ describe('letter-mapper', () => { }); }); + + it('maps an internal Letter to a GetLetterResponse', () => { + const letter: Letter = { + id: 'abc123', + status: 'PENDING', + supplierId: 'supplier1', + specificationId: 'spec123', + groupId: 'group123', + url: 'https://example.com/letter/abc123', + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + supplierStatus: 'supplier1#PENDING', + supplierStatusSk: Date.now().toString(), + ttl: 123 + }; + + const result: GetLetterResponse = mapToGetLetterResponse(letter); + + expect(result).toEqual({ + data: { + id: 'abc123', + type: 'Letter', + attributes: { + specificationId: 'spec123', + status: 'PENDING', + groupId: 'group123' + } + } + }); + }); + + it('maps an internal Letter to a GetLetterResponse with reasonCode and reasonText when present', () => { + const letter: Letter = { + id: 'abc123', + status: 'PENDING', + supplierId: 'supplier1', + specificationId: 'spec123', + groupId: 'group123', + url: 'https://example.com/letter/abc123', + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + supplierStatus: 'supplier1#PENDING', + supplierStatusSk: Date.now().toString(), + ttl: 123, + reasonCode: 123, + reasonText: 'Reason text' + }; + + const result: GetLetterResponse = mapToGetLetterResponse(letter); + + expect(result).toEqual({ + data: { + id: 'abc123', + type: 'Letter', + attributes: { + specificationId: 'spec123', + status: 'PENDING', + groupId: 'group123', + reasonCode: 123, + reasonText: 'Reason text', + } + } + }); + }); + it('maps an internal Letter collection to a GetLettersResponse', () => { const letter: Letter = { id: 'abc123', diff --git a/lambdas/api-handler/src/mappers/letter-mapper.ts b/lambdas/api-handler/src/mappers/letter-mapper.ts index e35bcb00c..971326fa0 100644 --- a/lambdas/api-handler/src/mappers/letter-mapper.ts +++ b/lambdas/api-handler/src/mappers/letter-mapper.ts @@ -1,5 +1,5 @@ import { LetterBase, LetterStatus } from "../../../../internal/datastore"; -import { GetLettersResponse, GetLettersResponseSchema, LetterDto, PatchLetterRequest, PatchLetterResponse, PatchLetterResponseSchema } from '../contracts/letters'; +import { GetLetterResponse, GetLetterResponseSchema, GetLettersResponse, GetLettersResponseSchema, LetterDto, PatchLetterRequest, PatchLetterResponse, PatchLetterResponseSchema } from '../contracts/letters'; export function mapToLetterDto(request: PatchLetterRequest, supplierId: string) : LetterDto { return { @@ -13,27 +13,37 @@ export function mapToLetterDto(request: PatchLetterRequest, supplierId: string) export function mapToPatchLetterResponse(letter: LetterBase): PatchLetterResponse { return PatchLetterResponseSchema.parse({ - data: { - id: letter.id, - type: 'Letter', - attributes: { - status: letter.status, - specificationId: letter.specificationId, - groupId: letter.groupId, - ...(letter.reasonCode != null && { reasonCode: letter.reasonCode }), - ...(letter.reasonText != null && { reasonText: letter.reasonText }) - } - } + data: letterToResourceResponse(letter) }); } export function mapToGetLettersResponse(letters: LetterBase[]): GetLettersResponse { return GetLettersResponseSchema.parse({ - data: letters.map(letterToResourceResponse) + data: letters.map(letterToGetLettersResourceResponse) + }); +} + +export function mapToGetLetterResponse(letter: LetterBase): GetLetterResponse { + return GetLetterResponseSchema.parse({ + data:letterToResourceResponse(letter) }); } function letterToResourceResponse(letter: LetterBase) { + return { + id: letter.id, + type: 'Letter', + attributes: { + status: letter.status, + specificationId: letter.specificationId, + groupId: letter.groupId, + ...(letter.reasonCode != null && { reasonCode: letter.reasonCode }), + ...(letter.reasonText != null && { reasonText: letter.reasonText }) + } + }; +}; + +function letterToGetLettersResourceResponse(letter: LetterBase) { return { id: letter.id, type: 'Letter', @@ -43,4 +53,4 @@ function letterToResourceResponse(letter: LetterBase) { groupId: letter.groupId } }; -} +}; diff --git a/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts b/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts index 7420c3110..324592969 100644 --- a/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts +++ b/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts @@ -1,6 +1,6 @@ import { Letter } from '../../../../../internal/datastore/src'; import { LetterDto, LetterStatus } from '../../contracts/letters'; -import { getLettersForSupplier, patchLetterStatus } from '../letter-operations'; +import { getLetterById, getLettersForSupplier, patchLetterStatus } from '../letter-operations'; function makeLetter(id: string, status: Letter['status']) : Letter { @@ -49,6 +49,48 @@ describe("getLetterIdsForSupplier", () => { }); }); +describe("getLetterById", () => { + + const testLetter = { id: "id1", status: "PENDING", specificationId: "s1", groupId: "g1", }; + + it("returns letter from the repository", async () => { + + const mockRepo = { + getLetterById: jest.fn().mockResolvedValue(testLetter), + }; + + const result = await getLetterById( + "supplier1", + "id1", + mockRepo as any, + ); + + expect(mockRepo.getLetterById).toHaveBeenCalledWith( + "supplier1", + "id1", + ); + expect(result).toEqual({ id: 'id1', status: 'PENDING', specificationId: 's1', groupId: 'g1' }); + }); + + it('should throw notFoundError when letter does not exist', async () => { + const mockRepo = { + getLetterById: jest.fn().mockRejectedValue(new Error('Letter with id l1 not found for supplier s1')) + }; + + await expect(getLetterById('supplierid', 'letter1', mockRepo as any)).rejects.toThrow('No resource found with that ID'); + }); + + it('should throw unexpected error', async () => { + + const mockRepo = { + getLetterById: jest.fn().mockRejectedValue(new Error('unexpected error')) + }; + + await expect(getLetterById('supplierid', 'letter1', mockRepo as any)).rejects.toThrow("unexpected error"); + }); + +}); + describe('patchLetterStatus function', () => { const updatedLetterDto: LetterDto = { diff --git a/lambdas/api-handler/src/services/letter-operations.ts b/lambdas/api-handler/src/services/letter-operations.ts index 4344ef95e..fb6c9ac6f 100644 --- a/lambdas/api-handler/src/services/letter-operations.ts +++ b/lambdas/api-handler/src/services/letter-operations.ts @@ -10,6 +10,22 @@ export const getLettersForSupplier = async (supplierId: string, status: string, return await letterRepo.getLettersBySupplier(supplierId, status, limit); } +export const getLetterById = async (supplierId: string, letterId: string, letterRepo: LetterRepository): Promise => { + + let letter; + + try { + letter = await letterRepo.getLetterById(supplierId, letterId); + } catch (error) { + if (isNotFoundError(error)) { + throw new NotFoundError(ApiErrorDetail.NotFoundLetterId); + } + throw error; + } + + return letter; +} + export const patchLetterStatus = async (letterToUpdate: LetterDto, letterId: string, letterRepo: LetterRepository): Promise => { if (letterToUpdate.id !== letterId) { @@ -21,7 +37,7 @@ export const patchLetterStatus = async (letterToUpdate: LetterDto, letterId: str try { updatedLetter = await letterRepo.updateLetterStatus(letterToUpdate); } catch (error) { - if (error instanceof Error && /^Letter with id \w+ not found for supplier \w+$/.test(error.message)) { + if (isNotFoundError(error)) { throw new NotFoundError(ApiErrorDetail.NotFoundLetterId); } throw error; @@ -29,3 +45,6 @@ export const patchLetterStatus = async (letterToUpdate: LetterDto, letterId: str return mapToPatchLetterResponse(updatedLetter); } +function isNotFoundError(error: any) { + return error instanceof Error && /^Letter with id \w+ not found for supplier \w+$/.test(error.message); +} diff --git a/sdk/_config.version.yml b/sdk/_config.version.yml index 517764705..a680fe417 100644 --- a/sdk/_config.version.yml +++ b/sdk/_config.version.yml @@ -1 +1 @@ -version: 0.2.0-20250724.105025+579bd65 +version: 0.2.0-20251010.134128+05606d5 diff --git a/server/_config.version.yml b/server/_config.version.yml index 6660203eb..d03844ea3 100644 --- a/server/_config.version.yml +++ b/server/_config.version.yml @@ -1 +1 @@ -version: 0.2.0-20250724.124925+dcf6731 +version: 0.2.0-20251010.134120+05606d5 diff --git a/src/server/_config.version.yml b/src/server/_config.version.yml index b092a70a1..591e87e24 100644 --- a/src/server/_config.version.yml +++ b/src/server/_config.version.yml @@ -1 +1 @@ -version: 0.2.0-20250724.105048+579bd65 +version: 0.2.0-20251010.134144+05606d5 From 494068bf0af363b0183937e42143e90c25d7d367 Mon Sep 17 00:00:00 2001 From: Steve Buxton Date: Wed, 22 Oct 2025 17:21:58 +0100 Subject: [PATCH 2/2] Fixes following merge from main --- .../terraform/components/api/README.md | 1 - .../src/handlers/__tests__/get-letter.test.ts | 50 +++++++++++++------ .../api-handler/src/handlers/get-letter.ts | 34 ++++++------- lambdas/api-handler/src/index.ts | 2 +- 4 files changed, 49 insertions(+), 38 deletions(-) diff --git a/infrastructure/terraform/components/api/README.md b/infrastructure/terraform/components/api/README.md index 17f0bd69b..1b60ce0ac 100644 --- a/infrastructure/terraform/components/api/README.md +++ b/infrastructure/terraform/components/api/README.md @@ -35,7 +35,6 @@ No requirements. | [authorizer\_lambda](#module\_authorizer\_lambda) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.24/terraform-lambda.zip | n/a | | [domain\_truststore](#module\_domain\_truststore) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-s3bucket.zip | n/a | | [get\_letter](#module\_get\_letter) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.24/terraform-lambda.zip | n/a | -| [get\_letters](#module\_get\_letters) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-lambda.zip | n/a | | [get\_letter\_data](#module\_get\_letter\_data) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.24/terraform-lambda.zip | n/a | | [get\_letters](#module\_get\_letters) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.24/terraform-lambda.zip | n/a | | [kms](#module\_kms) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.20/terraform-kms.zip | n/a | diff --git a/lambdas/api-handler/src/handlers/__tests__/get-letter.test.ts b/lambdas/api-handler/src/handlers/__tests__/get-letter.test.ts index 9590200d4..99b01b743 100644 --- a/lambdas/api-handler/src/handlers/__tests__/get-letter.test.ts +++ b/lambdas/api-handler/src/handlers/__tests__/get-letter.test.ts @@ -2,21 +2,34 @@ import { Context } from 'aws-lambda'; import { mockDeep } from 'jest-mock-extended'; import * as letterService from '../../services/letter-operations'; import { makeApiGwEvent } from './utils/test-utils'; -import { getLetter } from '../../index'; import { ApiErrorDetail } from '../../contracts/errors'; import { NotFoundError } from '../../errors'; +import { S3Client } from '@aws-sdk/client-s3'; +import pino from 'pino'; +import { LetterRepository } from '../../../../../internal/datastore/src'; +import { Deps } from '../../config/deps'; +import { EnvVars } from '../../config/env'; +import { createGetLetterHandler } from '../get-letter'; jest.mock('../../services/letter-operations'); -jest.mock('../../config/lambda-config', () => ({ - lambdaConfig: { - SUPPLIER_ID_HEADER: 'nhsd-supplier-id', - APIM_CORRELATION_HEADER: 'nhsd-correlation-id' - } -})); describe('API Lambda handler', () => { + const mockedDeps: jest.Mocked = { + s3Client: {} as unknown as S3Client, + letterRepo: {} as unknown as LetterRepository, + logger: { info: jest.fn(), error: jest.fn() } as unknown as pino.Logger, + env: { + SUPPLIER_ID_HEADER: 'nhsd-supplier-id', + APIM_CORRELATION_HEADER: 'nhsd-correlation-id', + LETTERS_TABLE_NAME: 'LETTERS_TABLE_NAME', + LETTER_TTL_HOURS: 12960, + DOWNLOAD_URL_TTL_SECONDS: 60, + MAX_LIMIT: 2500 + } as unknown as EnvVars + }; + beforeEach(() => { jest.clearAllMocks(); jest.resetModules(); @@ -33,9 +46,10 @@ describe('API Lambda handler', () => { }); const event = makeApiGwEvent({path: '/letters/id1', - headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId'}, + headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId', 'x-request-id': 'requestId'}, pathParameters: {id: 'id1'}}); + const getLetter = createGetLetterHandler(mockedDeps); const result = await getLetter(event, mockDeep(), jest.fn()); const expected = { @@ -69,12 +83,12 @@ describe('API Lambda handler', () => { }); const event = makeApiGwEvent({path: '/letters/id1', - headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId'}, + headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId', 'x-request-id': 'requestId'}, pathParameters: {id: 'id1'}}); + const getLetter = createGetLetterHandler(mockedDeps); const result = await getLetter(event, mockDeep(), jest.fn()); - const expected = { data: { id: 'id1', @@ -103,9 +117,10 @@ describe('API Lambda handler', () => { }); const event = makeApiGwEvent({path: '/letters/id1', - headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId'}, + headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId', 'x-request-id': 'requestId'}, pathParameters: {id: 'id1'}}); + const getLetter = createGetLetterHandler(mockedDeps); const result = await getLetter(event, mockDeep(), jest.fn()); expect(result).toEqual(expect.objectContaining({ @@ -115,9 +130,10 @@ describe('API Lambda handler', () => { it ('returns 500 when correlation id is missing from header', async() => { const event = makeApiGwEvent({path: '/letters/id1', - headers: {'nhsd-supplier-id': 'supplier1'}, + headers: {'nhsd-supplier-id': 'supplier1', 'x-request-id': 'requestId'}, pathParameters: {id: 'id1'}}); + const getLetter = createGetLetterHandler(mockedDeps); const result = await getLetter(event, mockDeep(), jest.fn()); expect(result).toEqual(expect.objectContaining({ @@ -125,23 +141,25 @@ describe('API Lambda handler', () => { })); }); - it ('returns 400 when supplier id is missing from header', async() => { + it ('returns 500 when supplier id is missing from header', async() => { const event = makeApiGwEvent({path: '/letters/id1', - headers: {'nhsd-correlation-id': 'correlationId'}, + headers: {'nhsd-correlation-id': 'correlationId', 'x-request-id': 'requestId'}, pathParameters: {id: 'id1'}}); + const getLetter = createGetLetterHandler(mockedDeps); const result = await getLetter(event, mockDeep(), jest.fn()); expect(result).toEqual(expect.objectContaining({ - statusCode: 400, + statusCode: 500, })); }); it ('returns 400 when letter id is missing from path', async() => { const event = makeApiGwEvent({path: '/letters/id1', - headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId'}}); + headers: {'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId', 'x-request-id': 'requestId'}}); + const getLetter = createGetLetterHandler(mockedDeps); const result = await getLetter(event, mockDeep(), jest.fn()); expect(result).toEqual(expect.objectContaining({ diff --git a/lambdas/api-handler/src/handlers/get-letter.ts b/lambdas/api-handler/src/handlers/get-letter.ts index 7e349a648..5c428dcef 100644 --- a/lambdas/api-handler/src/handlers/get-letter.ts +++ b/lambdas/api-handler/src/handlers/get-letter.ts @@ -1,40 +1,33 @@ -import pino from "pino"; -import { createLetterRepository } from "../infrastructure/letter-repo-factory"; import { APIGatewayProxyHandler } from "aws-lambda"; -import { assertNotEmpty, lowerCaseKeys } from "../utils/validation"; -import { lambdaConfig } from "../config/lambda-config"; +import { assertNotEmpty, validateCommonHeaders } from "../utils/validation"; import { ValidationError } from "../errors"; import { ApiErrorDetail } from "../contracts/errors"; import { getLetterById } from "../services/letter-operations"; import { mapErrorToResponse } from "../mappers/error-mapper"; import { mapToGetLetterResponse } from "../mappers/letter-mapper"; +import { Deps } from "../config/deps"; -const letterRepo = createLetterRepository(); -const log = pino(); +export function createGetLetterHandler(deps: Deps): APIGatewayProxyHandler { -export const getLetter: APIGatewayProxyHandler = async (event) => { + return async (event) => { - let correlationId; - try { - assertNotEmpty(event.headers, new Error("The request headers are empty")); - const lowerCasedHeaders = lowerCaseKeys(event.headers); - - correlationId = assertNotEmpty(lowerCasedHeaders[lambdaConfig.APIM_CORRELATION_HEADER], - new Error("The request headers don't contain the APIM correlation id")); + const commonHeadersResult = validateCommonHeaders(event.headers, deps); - const supplierId = assertNotEmpty(lowerCasedHeaders[lambdaConfig.SUPPLIER_ID_HEADER], - new ValidationError(ApiErrorDetail.InvalidRequestMissingSupplierId)); + if (!commonHeadersResult.ok) { + return mapErrorToResponse(commonHeadersResult.error, commonHeadersResult.correlationId, deps.logger); + } + try { const letterId = assertNotEmpty(event.pathParameters?.id, new ValidationError(ApiErrorDetail.InvalidRequestMissingLetterIdPathParameter)); - const letter = await getLetterById(supplierId, letterId, letterRepo); + const letter = await getLetterById(commonHeadersResult.value.supplierId, letterId, deps.letterRepo); const response = mapToGetLetterResponse(letter); - log.info({ + deps.logger.info({ description: 'Letter successfully fetched by id', - supplierId, + supplierId: commonHeadersResult.value.supplierId, letterId }); @@ -44,6 +37,7 @@ export const getLetter: APIGatewayProxyHandler = async (event) => { }; } catch (error) { - return mapErrorToResponse(error, correlationId); + return mapErrorToResponse(error, commonHeadersResult.value.correlationId, deps.logger); } + } } diff --git a/lambdas/api-handler/src/index.ts b/lambdas/api-handler/src/index.ts index ae1f593b5..bca91bc92 100644 --- a/lambdas/api-handler/src/index.ts +++ b/lambdas/api-handler/src/index.ts @@ -6,7 +6,7 @@ import { createPatchLetterHandler } from "./handlers/patch-letter"; const container = createDependenciesContainer(); -export const getLetterHandler = createGetLetterHandler(container); +export const getLetter = createGetLetterHandler(container); export const getLetterData = createGetLetterDataHandler(container); export const getLetters = createGetLettersHandler(container); export const patchLetter = createPatchLetterHandler(container);