diff --git a/infrastructure/terraform/components/api/README.md b/infrastructure/terraform/components/api/README.md index 97653bcac..00196a0bc 100644 --- a/infrastructure/terraform/components/api/README.md +++ b/infrastructure/terraform/components/api/README.md @@ -54,6 +54,7 @@ No requirements. | [amendment\_event\_transformer](#module\_amendment\_event\_transformer) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.29/terraform-lambda.zip | n/a | | [amendments\_queue](#module\_amendments\_queue) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/3.0.6/terraform-sqs.zip | n/a | | [authorizer\_lambda](#module\_authorizer\_lambda) | https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.29/terraform-lambda.zip | n/a | +| [ddb\_alarms\_idempotency](#module\_ddb\_alarms\_idempotency) | ../../modules/alarms-ddb | n/a | | [ddb\_alarms\_letter\_queue](#module\_ddb\_alarms\_letter\_queue) | ../../modules/alarms-ddb | n/a | | [ddb\_alarms\_letters](#module\_ddb\_alarms\_letters) | ../../modules/alarms-ddb | n/a | | [ddb\_alarms\_mi](#module\_ddb\_alarms\_mi) | ../../modules/alarms-ddb | n/a | diff --git a/infrastructure/terraform/components/api/ddb_table_idempotency.tf b/infrastructure/terraform/components/api/ddb_table_idempotency.tf new file mode 100644 index 000000000..4e7c4cd6e --- /dev/null +++ b/infrastructure/terraform/components/api/ddb_table_idempotency.tf @@ -0,0 +1,24 @@ +resource "aws_dynamodb_table" "idempotency" { + name = "${local.csi}-idempotency" + billing_mode = "PAY_PER_REQUEST" + hash_key = "id" + attribute { + name = "id" + type = "S" + } + ttl { + attribute_name = "expiration" + enabled = true + } + + point_in_time_recovery { + enabled = true + } + + tags = merge( + local.default_tags, + { + NHSE-Enable-Dynamo-Backup-Acct = "True" + } + ) +} diff --git a/infrastructure/terraform/components/api/module_ddb_alarms_idempotency.tf b/infrastructure/terraform/components/api/module_ddb_alarms_idempotency.tf new file mode 100644 index 000000000..755833ffe --- /dev/null +++ b/infrastructure/terraform/components/api/module_ddb_alarms_idempotency.tf @@ -0,0 +1,7 @@ +module "ddb_alarms_idempotency" { + count = local.alarms_enabled ? 1 : 0 + source = "../../modules/alarms-ddb" + alarm_prefix = local.csi + table_name = aws_dynamodb_table.idempotency.name + tags = local.default_tags +} diff --git a/infrastructure/terraform/components/api/module_lambda_upsert_letter.tf b/infrastructure/terraform/components/api/module_lambda_upsert_letter.tf index 201e10186..bac0f3a23 100644 --- a/infrastructure/terraform/components/api/module_lambda_upsert_letter.tf +++ b/infrastructure/terraform/components/api/module_lambda_upsert_letter.tf @@ -35,7 +35,8 @@ module "upsert_letter" { log_subscription_role_arn = local.acct.log_subscription_role_arn lambda_env_vars = merge(local.common_lambda_env_vars, { - VARIANT_MAP = jsonencode(var.letter_variant_map) + VARIANT_MAP = jsonencode(var.letter_variant_map), + IDEMPOTENCY_TABLE_NAME = aws_dynamodb_table.idempotency.name }) } @@ -67,6 +68,7 @@ data "aws_iam_policy_document" "upsert_letter_lambda" { resources = [ aws_dynamodb_table.letters.arn, + aws_dynamodb_table.idempotency.arn, "${aws_dynamodb_table.letters.arn}/index/supplierStatus-index" ] } diff --git a/lambdas/upsert-letter/package.json b/lambdas/upsert-letter/package.json index 01211d571..906cf5bd8 100644 --- a/lambdas/upsert-letter/package.json +++ b/lambdas/upsert-letter/package.json @@ -1,5 +1,6 @@ { "dependencies": { + "@aws-lambda-powertools/idempotency": "^2.33.0", "@aws-sdk/client-dynamodb": "^3.984.0", "@aws-sdk/lib-dynamodb": "^3.1008.0", "@internal/datastore": "*", diff --git a/lambdas/upsert-letter/src/config/__tests__/env.test.ts b/lambdas/upsert-letter/src/config/__tests__/env.test.ts index 7f3462798..fce4a108d 100644 --- a/lambdas/upsert-letter/src/config/__tests__/env.test.ts +++ b/lambdas/upsert-letter/src/config/__tests__/env.test.ts @@ -16,12 +16,14 @@ describe("lambdaEnv", () => { it("should load all environment variables successfully", () => { process.env.LETTERS_TABLE_NAME = "letters-table"; + process.env.IDEMPOTENCY_TABLE_NAME = "idempotency-table"; process.env.LETTER_TTL_HOURS = "12960"; const { envVars } = require("../env"); expect(envVars).toEqual({ LETTERS_TABLE_NAME: "letters-table", + IDEMPOTENCY_TABLE_NAME: "idempotency-table", LETTER_TTL_HOURS: 12_960, }); }); diff --git a/lambdas/upsert-letter/src/config/deps.ts b/lambdas/upsert-letter/src/config/deps.ts index 9303e1152..09292c6a3 100644 --- a/lambdas/upsert-letter/src/config/deps.ts +++ b/lambdas/upsert-letter/src/config/deps.ts @@ -1,5 +1,6 @@ import { DynamoDBClient } from "@aws-sdk/client-dynamodb"; import { DynamoDBDocumentClient } from "@aws-sdk/lib-dynamodb"; +import { DynamoDBPersistenceLayer } from "@aws-lambda-powertools/idempotency/dynamodb"; import { Logger } from "pino"; import { LetterRepository } from "@internal/datastore"; import { createLogger } from "@internal/helpers"; @@ -7,6 +8,7 @@ import { EnvVars, envVars } from "./env"; export type Deps = { letterRepo: LetterRepository; + idempotencyLayer: DynamoDBPersistenceLayer; logger: Logger; env: EnvVars; }; @@ -16,11 +18,7 @@ function createDocumentClient(): DynamoDBDocumentClient { return DynamoDBDocumentClient.from(ddbClient); } -function createLetterRepository( - log: Logger, - // eslint-disable-next-line @typescript-eslint/no-shadow - envVars: EnvVars, -): LetterRepository { +function createLetterRepository(log: Logger): LetterRepository { const config = { lettersTableName: envVars.LETTERS_TABLE_NAME, lettersTtlHours: envVars.LETTER_TTL_HOURS, @@ -29,11 +27,18 @@ function createLetterRepository( return new LetterRepository(createDocumentClient(), log, config); } +function createIdempotencyLayer(): DynamoDBPersistenceLayer { + return new DynamoDBPersistenceLayer({ + tableName: envVars.IDEMPOTENCY_TABLE_NAME, + }); +} + export function createDependenciesContainer(): Deps { const log = createLogger({ logLevel: envVars.PINO_LOG_LEVEL }); return { - letterRepo: createLetterRepository(log, envVars), + letterRepo: createLetterRepository(log), + idempotencyLayer: createIdempotencyLayer(), logger: log, env: envVars, }; diff --git a/lambdas/upsert-letter/src/config/env.ts b/lambdas/upsert-letter/src/config/env.ts index fd205813a..1f3a3bdf8 100644 --- a/lambdas/upsert-letter/src/config/env.ts +++ b/lambdas/upsert-letter/src/config/env.ts @@ -4,6 +4,7 @@ const EnvVarsSchema = z.object({ LETTERS_TABLE_NAME: z.string(), LETTER_TTL_HOURS: z.coerce.number().int(), PINO_LOG_LEVEL: z.coerce.string().optional(), + IDEMPOTENCY_TABLE_NAME: z.string(), }); export type EnvVars = z.infer; diff --git a/lambdas/upsert-letter/src/handler/__tests__/upsert-handler.test.ts b/lambdas/upsert-letter/src/handler/__tests__/upsert-handler.test.ts index 2ead57328..201f29999 100644 --- a/lambdas/upsert-letter/src/handler/__tests__/upsert-handler.test.ts +++ b/lambdas/upsert-letter/src/handler/__tests__/upsert-handler.test.ts @@ -6,6 +6,7 @@ import { } from "@internal/datastore"; import { LetterRequestPreparedEventV2 } from "@nhsdigital/nhs-notify-event-schemas-letter-rendering"; import { LetterRequestPreparedEvent } from "@nhsdigital/nhs-notify-event-schemas-letter-rendering-v1"; +import { makeIdempotent } from "@aws-lambda-powertools/idempotency"; import { $LetterStatusChangeEvent, LetterStatusChangeEvent, @@ -15,6 +16,14 @@ import { Deps } from "../../config/deps"; import { EnvVars } from "../../config/env"; import packageJson from "../../../package.json"; +jest.mock("@aws-lambda-powertools/idempotency", () => { + const original = jest.requireActual("@aws-lambda-powertools/idempotency"); + return { + ...original, + makeIdempotent: jest.fn((fn, _) => fn), + }; +}); + const renderingSchemaVersion: string = packageJson.dependencies[ "@nhsdigital/nhs-notify-event-schemas-letter-rendering" @@ -313,7 +322,7 @@ describe("createUpsertLetterHandler", () => { ); }); - it("does not treat a replayed insert as a failure", async () => { + it("does not treat a second insert for the same letter as a failure", async () => { const v1message = { letterEvent: createPreparedV1Event(), supplierSpec: { @@ -338,6 +347,26 @@ describe("createUpsertLetterHandler", () => { expect(result!.batchItemFailures).toEqual([]); }); + it("does not insert a letter if the same message is replayed", async () => { + const v1message = { + letterEvent: createPreparedV1Event(), + supplierSpec: { + supplierId: "supplier1", + specId: "spec1", + priority: 10, + billingId: "billing1", + }, + }; + const evt: SQSEvent = createSQSEvent([ + createSqsRecord("msg2", JSON.stringify(v1message)), + ]); + (makeIdempotent as jest.Mock).mockImplementationOnce((_fn) => "supplier1"); + + await createUpsertLetterHandler(mockedDeps)(evt, {} as any, {} as any); + + expect(mockedDeps.letterRepo.putLetter).not.toHaveBeenCalled(); + }); + test("unknown supplier has metric emitted with 'unknown' supplier dimension", async () => { const letterEvent = createSupplierStatusChangeEventWithoutSupplier(); diff --git a/lambdas/upsert-letter/src/handler/upsert-handler.ts b/lambdas/upsert-letter/src/handler/upsert-handler.ts index 677048ba5..61b6feb0d 100644 --- a/lambdas/upsert-letter/src/handler/upsert-handler.ts +++ b/lambdas/upsert-letter/src/handler/upsert-handler.ts @@ -1,4 +1,4 @@ -import { SQSBatchItemFailure, SQSEvent, SQSHandler } from "aws-lambda"; +import { Context, SQSBatchItemFailure, SQSEvent, SQSHandler } from "aws-lambda"; import { $LetterRequestPreparedEvent } from "@nhsdigital/nhs-notify-event-schemas-letter-rendering-v1"; import { InsertLetter, @@ -11,6 +11,10 @@ import { } from "@nhsdigital/nhs-notify-event-schemas-supplier-api/src/events/letter-events"; import { $LetterRequestPreparedEventV2 } from "@nhsdigital/nhs-notify-event-schemas-letter-rendering"; import { MetricsLogger, Unit, metricScope } from "aws-embedded-metrics"; +import { + IdempotencyConfig, + makeIdempotent, +} from "@aws-lambda-powertools/idempotency"; import { Deps } from "../config/deps"; import { PreparedEvents, @@ -20,6 +24,10 @@ import { UpsertOperation, } from "./schemas"; +const idempotencyConfig = new IdempotencyConfig({ + eventKeyJmesPath: "id", +}); + function getOperationFromType(type: string): UpsertOperation { if ( type.startsWith("uk.nhs.notify.letter-rendering.letter-request.prepared") @@ -180,8 +188,13 @@ function parseQueueMessage(queueMessage: string): QueueMessage { } export default function createUpsertLetterHandler(deps: Deps): SQSHandler { + const processRecordIdempotently = makeIdempotent(processRecord, { + persistenceStore: deps.idempotencyLayer, + config: idempotencyConfig, + }); + return metricScope((metrics: MetricsLogger) => { - return async (event: SQSEvent) => { + return async (event: SQSEvent, context: Context) => { const batchItemFailures: SQSBatchItemFailure[] = []; const perSupplierSuccess: Map = new Map(); const perSupplierFailure: Map = new Map(); @@ -216,29 +229,13 @@ export default function createUpsertLetterHandler(deps: Deps): SQSHandler { supplier: supplierSpec, }); - supplier = - !supplierSpec || !supplierSpec.supplierId - ? getSupplierIdFromEvent(letterEvent) - : supplierSpec.supplierId; - - const operation = getOperationFromType(letterEvent.type); - - await runUpsert( - operation, + idempotencyConfig.registerLambdaContext(context); + supplier = await processRecordIdempotently( letterEvent, - supplierSpec ?? { - supplierId: "unknown", - specId: "unknown", - priority: 10, - billingId: "unknown", - }, + supplierSpec, + perSupplierSuccess, deps, ); - - perSupplierSuccess.set( - supplier, - (perSupplierSuccess.get(supplier) || 0) + 1, - ); } catch (error) { deps.logger.error({ description: "Error processing upsert of record", @@ -261,3 +258,32 @@ export default function createUpsertLetterHandler(deps: Deps): SQSHandler { }; }); } + +async function processRecord( + letterEvent: LetterStatusChangeEvent | PreparedEvents, + supplierSpec: SupplierSpec | undefined, + perSupplierSuccess: Map, + deps: Deps, +) { + const supplier = + !supplierSpec || !supplierSpec.supplierId + ? getSupplierIdFromEvent(letterEvent) + : supplierSpec.supplierId; + + const operation = getOperationFromType(letterEvent.type); + + await runUpsert( + operation, + letterEvent, + supplierSpec ?? { + supplierId: "unknown", + specId: "unknown", + priority: 10, + billingId: "unknown", + }, + deps, + ); + + perSupplierSuccess.set(supplier, (perSupplierSuccess.get(supplier) || 0) + 1); + return supplier; +} diff --git a/package-lock.json b/package-lock.json index 32d4e293e..b43a56c15 100644 --- a/package-lock.json +++ b/package-lock.json @@ -310,6 +310,7 @@ "name": "nhs-notify-supplier-api-upsert-letter", "version": "0.0.1", "dependencies": { + "@aws-lambda-powertools/idempotency": "^2.33.0", "@aws-sdk/client-dynamodb": "^3.984.0", "@aws-sdk/lib-dynamodb": "^3.1008.0", "@internal/datastore": "*", @@ -588,6 +589,58 @@ "node": ">=14.0.0" } }, + "node_modules/@aws-lambda-powertools/commons": { + "version": "2.33.0", + "resolved": "https://registry.npmjs.org/@aws-lambda-powertools/commons/-/commons-2.33.0.tgz", + "integrity": "sha512-cXGfWT4FXtLO7Ny/shep6V/xYzgVN7hvukOA0bTa4nlg2GWiQsPoBoKvCEcfQ9I8GIS0tPyDfRkdx80SvA7LpA==", + "license": "MIT-0", + "dependencies": { + "@aws/lambda-invoke-store": "0.2.4" + } + }, + "node_modules/@aws-lambda-powertools/idempotency": { + "version": "2.33.0", + "resolved": "https://registry.npmjs.org/@aws-lambda-powertools/idempotency/-/idempotency-2.33.0.tgz", + "integrity": "sha512-2eMo4dkdXxqizcC+w4CjT2vVfTPQeKCceM17WzdatrGhWREuiZd2AYXNAE1Q/LWaiwjikz0fH8hf1sq3P2/q4Q==", + "license": "MIT-0", + "dependencies": { + "@aws-lambda-powertools/commons": "2.33.0", + "@aws-lambda-powertools/jmespath": "2.33.0" + }, + "peerDependencies": { + "@aws-sdk/client-dynamodb": ">=3.x", + "@aws-sdk/lib-dynamodb": ">=3.x", + "@middy/core": "4.x || 5.x || 6.x || 7.x", + "@redis/client": "^5.0.1", + "@valkey/valkey-glide": "^2.3.1" + }, + "peerDependenciesMeta": { + "@aws-sdk/client-dynamodb": { + "optional": true + }, + "@aws-sdk/lib-dynamodb": { + "optional": true + }, + "@middy/core": { + "optional": true + }, + "@redis/client": { + "optional": true + }, + "@valkey/valkey-glide": { + "optional": true + } + } + }, + "node_modules/@aws-lambda-powertools/jmespath": { + "version": "2.33.0", + "resolved": "https://registry.npmjs.org/@aws-lambda-powertools/jmespath/-/jmespath-2.33.0.tgz", + "integrity": "sha512-FkCHfv30mEyYQ984IurYx81TNzpggeORP7XAibNyj4qdNQboAZaKoyHxvqR8re2NgkxyaN5Qf9bodqdpVWbb2g==", + "license": "MIT-0", + "dependencies": { + "@aws-lambda-powertools/commons": "2.33.0" + } + }, "node_modules/@aws-sdk/client-api-gateway": { "version": "3.1002.0", "resolved": "https://registry.npmjs.org/@aws-sdk/client-api-gateway/-/client-api-gateway-3.1002.0.tgz", @@ -3079,9 +3132,9 @@ } }, "node_modules/@aws/lambda-invoke-store": { - "version": "0.2.3", - "resolved": "https://registry.npmjs.org/@aws/lambda-invoke-store/-/lambda-invoke-store-0.2.3.tgz", - "integrity": "sha512-oLvsaPMTBejkkmHhjf09xTgk71mOqyr/409NKhRIL08If7AhVfUsJhVsx386uJaqNd42v9kWamQ9lFbkoC2dYw==", + "version": "0.2.4", + "resolved": "https://registry.npmjs.org/@aws/lambda-invoke-store/-/lambda-invoke-store-0.2.4.tgz", + "integrity": "sha512-iY8yvjE0y651BixKNPgmv1WrQc+GZ142sb0z4gYnChDDY2YqI4P/jsSopBWrKfAt7LOJAkOXt7rC/hms+WclQQ==", "license": "Apache-2.0", "engines": { "node": ">=18.0.0" diff --git a/package.json b/package.json index 211a2cf97..747493a93 100644 --- a/package.json +++ b/package.json @@ -5,6 +5,7 @@ "@aws-sdk/client-s3": "^3.925.0", "@aws-sdk/client-sns": "^3.936.0", "@openapitools/openapi-generator-cli": "^2.21.4", + "@playwright/test": "^1.57.0", "@redocly/cli": "1.31.0", "@stoplight/spectral-cli": "^6.15.0", "@stylistic/eslint-plugin": "^5.9.0", @@ -14,7 +15,6 @@ "@types/node": "^25.2.2", "@typescript-eslint/eslint-plugin": "^8.46.2", "@typescript-eslint/parser": "^8.27.0", - "@playwright/test": "^1.57.0", "ajv": "^8.17.1", "allure-commandline": "^2.34.1", "allure-playwright": "^3.4.3", diff --git a/tests/component-tests/events-tests/event-subscription.spec.ts b/tests/component-tests/events-tests/event-subscription.spec.ts index 4d6044f60..82efbce31 100644 --- a/tests/component-tests/events-tests/event-subscription.spec.ts +++ b/tests/component-tests/events-tests/event-subscription.spec.ts @@ -104,16 +104,13 @@ test.describe("Event Subscription SNS Tests", () => { ); }); - test("Verify that the duplicate event throws an error", async () => { + test("Verify that an error is logged for a duplicate letter id", async () => { const domainId = randomUUID(); logger.info(`Testing event subscription with domainId: ${domainId}`); - const preparedEvent = createPreparedV1Event({ - domainId, - status: "PREPARED", - }); - const response = await sendSnsEvent(preparedEvent); + const preparedEvent1 = createPreparedV1Event({ domainId }); + const response1 = await sendSnsEvent(preparedEvent1); - expect(response.MessageId).toBeTruthy(); + expect(response1.MessageId).toBeTruthy(); // poll supplier allocator to check if supplier has been allocated const message = await pollSupplierAllocatorLogForResolvedSpec(domainId); @@ -129,11 +126,11 @@ test.describe("Event Subscription SNS Tests", () => { throw new Error("supplierId was not found in supplier allocator log"); } - // send same event again to simulate duplicate event - const duplicateResponse = await sendSnsEvent(preparedEvent); - expect(duplicateResponse.MessageId).toBeTruthy(); + const preparedEvent2 = createPreparedV1Event({ domainId }); + const response2 = await sendSnsEvent(preparedEvent2); + expect(response2.MessageId).toBeTruthy(); - // poll supplier upsert to check if duplicate event was processed + // poll supplier upsert to check if duplicate letter id was processed await pollUpsertLetterLogForWarning("Letter already exists", domainId); }); }); diff --git a/tests/component-tests/integration-tests/urgent-letter-priority.spec.ts b/tests/component-tests/integration-tests/urgent-letter-priority.spec.ts index c91d288cd..2a2f2858c 100644 --- a/tests/component-tests/integration-tests/urgent-letter-priority.spec.ts +++ b/tests/component-tests/integration-tests/urgent-letter-priority.spec.ts @@ -11,6 +11,7 @@ import { } from "tests/helpers/urgent-letter-priority-helper"; import { createValidRequestHeaders } from "tests/constants/request-headers"; import { SUPPLIER_LETTERS } from "tests/constants/api-constants"; +import { supplierDataSetup } from "tests/helpers/suppliers-setup-helper"; import { GetLettersResponse, GetLettersResponseSchema, @@ -20,6 +21,7 @@ let baseUrl: string; test.beforeAll(async () => { baseUrl = await getRestApiGatewayBaseUrl(); + await supplierDataSetup(supplier); }); test.describe("Urgent Letter Priority Tests", () => { diff --git a/tests/helpers/event-fixtures.ts b/tests/helpers/event-fixtures.ts index 5862cca20..e679ce950 100644 --- a/tests/helpers/event-fixtures.ts +++ b/tests/helpers/event-fixtures.ts @@ -2,10 +2,11 @@ import { randomUUID } from "node:crypto"; export function createPreparedV1Event(overrides: Record = {}) { const now = new Date().toISOString(); + const { id, ...dataOverrides } = overrides; return { specversion: "1.0", - id: (overrides.id as string) ?? "7b9a03ca-342a-4150-b56b-989109c45613", + id: (id as string) ?? randomUUID(), source: "/data-plane/letter-rendering/test", subject: "client/client1/letter-request/letterRequest1", type: "uk.nhs.notify.letter-rendering.letter-request.prepared.v1", @@ -14,23 +15,21 @@ export function createPreparedV1Event(overrides: Record = {}) { "https://notify.nhs.uk/cloudevents/schemas/letter-rendering/letter-request.prepared.1.0.0.schema.json", dataschemaversion: "1.0.0", data: { - domainId: - (overrides.domainId as string) ?? - "fe658e11-0ffc-44f4-8ad6-0fafe75bfeee", - letterVariantId: - (overrides.letterVariantId as string) ?? "digitrials-aspiring", + domainId: "fe658e11-0ffc-44f4-8ad6-0fafe75bfeee", + letterVariantId: "digitrials-aspiring", requestId: "request1", requestItemId: "requestItem1", requestItemPlanId: "requestItemPlan1", clientId: "client1", campaignId: "campaign1", templateId: "template1", - url: (overrides.url as string) ?? "s3://letterDataBucket/letter1.pdf", + url: "s3://letterDataBucket/letter1.pdf", sha256Hash: "3a7bd3e2360a3d29eea436fcfb7e44c735d117c8f2f1d2d1e4f6e8f7e6e8f7e6", createdAt: now, pageCount: 1, - status: (overrides.status as string) ?? "PREPARED", + status: "PREPARED", + ...dataOverrides, }, traceparent: "00-0af7651916cd43dd8448eb211c803191-b7ad6b7169203331-01", recordedtime: now, @@ -44,8 +43,8 @@ export function createPreparedEventBatchWithSameDomainId( overrides: Record = {}, ) { return [ - createPreparedV1Event({ id: randomUUID(), ...overrides }), - createPreparedV1Event({ id: randomUUID(), ...overrides }), - createPreparedV1Event({ id: randomUUID(), ...overrides }), + createPreparedV1Event(overrides), + createPreparedV1Event(overrides), + createPreparedV1Event(overrides), ]; }