Skip to content

Commit 0d85318

Browse files
stevebuxstevebux
committed
Delete from letter queue when letter no longer pending
1 parent 337a30b commit 0d85318

9 files changed

Lines changed: 274 additions & 55 deletions

File tree

‎infrastructure/terraform/components/api/ddb_table_letter_queue.tf‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,16 @@ resource "aws_dynamodb_table" "letter_queue" {
33
billing_mode = "PAY_PER_REQUEST"
44

55
hash_key = "supplierId"
6-
range_key = "queueTimestamp"
6+
range_key = "letterId"
77

88
ttl {
99
attribute_name = "ttl"
1010
enabled = true
1111
}
1212

1313
local_secondary_index {
14-
name = "letterId-index"
15-
range_key = "letterId"
14+
name = "queueTimestamp-index"
15+
range_key = "queueTimestamp"
1616
projection_type = "ALL"
1717
}
1818

‎infrastructure/terraform/components/api/module_lambda_update_letter_queue.tf‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ data "aws_iam_policy_document" "update_letter_queue_lambda" {
4747

4848
actions = [
4949
"dynamodb:PutItem",
50+
"dynamodb:DeleteItem",
5051
]
5152

5253
resources = [

‎internal/datastore/src/__test__/letter-queue-repository.test.ts‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { GetCommand } from "@aws-sdk/lib-dynamodb";
12
import { Logger } from "pino";
23
import {
34
DBContext,
@@ -7,8 +8,9 @@ import {
78
} from "./db";
89
import LetterQueueRepository from "../letter-queue-repository";
910
import { InsertPendingLetter } from "../types";
10-
import { LetterAlreadyExistsError } from "../errors";
11+
import { LetterAlreadyExistsError } from "../letter-already-exists-error";
1112
import { createTestLogger } from "./logs";
13+
import { LetterDoesNotExistError } from "../letter-does-not-exist-error";
1214

1315
function createLetter(letterId = "letter1"): InsertPendingLetter {
1416
return {
@@ -77,6 +79,7 @@ describe("LetterQueueRepository", () => {
7779
expect(timestampInMillis).toBeGreaterThanOrEqual(before);
7880
expect(timestampInMillis).toBeLessThanOrEqual(after);
7981
assertTtl(pendingLetter.ttl, before, after);
82+
expect(await letterExists(db, "supplier1", "letter1")).toBe(true);
8083
});
8184

8285
it("throws LetterAlreadyExistsError when creating a letter which already exists", async () => {
@@ -101,4 +104,48 @@ describe("LetterQueueRepository", () => {
101104
).rejects.toThrow("Cannot do operations on a non-existent table");
102105
});
103106
});
107+
108+
describe("deleteLetter", () => {
109+
it("deletes a letter from the database", async () => {
110+
await letterQueueRepository.putLetter(createLetter());
111+
112+
await letterQueueRepository.deleteLetter("supplier1", "letter1");
113+
114+
expect(await letterExists(db, "supplier1", "letter1")).toBe(false);
115+
});
116+
117+
it("throws an error when the letter does not exist", async () => {
118+
await expect(
119+
letterQueueRepository.deleteLetter("supplier1", "letter1"),
120+
).rejects.toThrow(LetterDoesNotExistError);
121+
});
122+
123+
it("rethrows errors from DynamoDB when deleting a letter", async () => {
124+
const misconfiguredRepository = new LetterQueueRepository(
125+
db.docClient,
126+
logger,
127+
{
128+
...db.config,
129+
letterQueueTableName: "nonexistent-table",
130+
},
131+
);
132+
await expect(
133+
misconfiguredRepository.deleteLetter("supplier1", "letter1"),
134+
).rejects.toThrow("Cannot do operations on a non-existent table");
135+
});
136+
});
104137
});
138+
139+
async function letterExists(
140+
db: DBContext,
141+
supplierId: string,
142+
letterId: string,
143+
): Promise<boolean> {
144+
const result = await db.docClient.send(
145+
new GetCommand({
146+
TableName: db.config.letterQueueTableName,
147+
Key: { supplierId, letterId },
148+
}),
149+
);
150+
return result.Item !== undefined;
151+
}

‎internal/datastore/src/index.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
export * from "./types";
2-
export * from "./errors";
2+
export * from "./letter-already-exists-error";
3+
export * from "./letter-does-not-exist-error";
34
export * from "./mi-repository";
45
export * from "./letter-repository";
56
export * from "./supplier-repository";

internal/datastore/src/errors.ts renamed to internal/datastore/src/letter-already-exists-error.ts

File renamed without changes.

‎internal/datastore/src/letter-does-not-exist-error.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
/**
2+
* Error thrown when attempting to delete a letter that does not exist in the database.
3+
*/
4+
// eslint-disable-next-line import-x/prefer-default-export
5+
export class LetterDoesNotExistError extends Error {
6+
constructor(
7+
public readonly supplierId: string,
8+
public readonly letterId: string,
9+
) {
10+
super(
11+
`Letter does not exist: supplierId=${supplierId}, letterId=${letterId}`,
12+
);
13+
this.name = "LetterDoesNotExistError";
14+
}
15+
}

‎internal/datastore/src/letter-queue-repository.ts‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,16 @@
1-
import { DynamoDBDocumentClient, PutCommand } from "@aws-sdk/lib-dynamodb";
1+
import {
2+
DeleteCommand,
3+
DynamoDBDocumentClient,
4+
PutCommand,
5+
} from "@aws-sdk/lib-dynamodb";
26
import { Logger } from "pino";
37
import {
48
InsertPendingLetter,
59
PendingLetter,
610
PendingLetterSchema,
711
} from "./types";
8-
import { LetterAlreadyExistsError } from "./errors";
12+
import { LetterAlreadyExistsError } from "./letter-already-exists-error";
13+
import { LetterDoesNotExistError } from "./letter-does-not-exist-error";
914

1015
type LetterQueueRepositoryConfig = {
1116
letterQueueTableName: string;
@@ -52,4 +57,24 @@ export default class LetterQueueRepository {
5257
}
5358
return PendingLetterSchema.parse(pendingLetter);
5459
}
60+
61+
async deleteLetter(supplierId: string, letterId: string): Promise<void> {
62+
try {
63+
await this.ddbClient.send(
64+
new DeleteCommand({
65+
TableName: this.config.letterQueueTableName,
66+
Key: { supplierId, letterId },
67+
ConditionExpression: "attribute_exists(letterId)",
68+
}),
69+
);
70+
} catch (error) {
71+
if (
72+
error instanceof Error &&
73+
error.name === "ConditionalCheckFailedException"
74+
) {
75+
throw new LetterDoesNotExistError(supplierId, letterId);
76+
}
77+
throw error;
78+
}
79+
}
5580
}

‎lambdas/update-letter-queue/src/__tests__/update-letter-queue.test.ts‎

Lines changed: 93 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {
22
Letter,
33
LetterAlreadyExistsError,
4+
LetterDoesNotExistError,
45
LetterQueueRepository,
56
} from "@internal/datastore";
67
import { mockDeep } from "jest-mock-extended";
@@ -21,6 +22,7 @@ import { LetterStatus } from "../../../api-handler/src/contracts/letters";
2122
const mockedDeps: jest.Mocked<Deps> = {
2223
letterQueueRepository: {
2324
putLetter: jest.fn(),
25+
deleteLetter: jest.fn(),
2426
} as unknown as LetterQueueRepository,
2527
logger: {
2628
info: jest.fn(),
@@ -50,7 +52,7 @@ function generateLetter(status: LetterStatus, id?: string): Letter {
5052
}
5153

5254
beforeEach(() => {
53-
jest.clearAllMocks();
55+
jest.resetAllMocks();
5456
});
5557

5658
describe("update-letter-queue Lambda", () => {
@@ -74,23 +76,25 @@ describe("update-letter-queue Lambda", () => {
7476
expect(result.batchItemFailures).toEqual([]);
7577
});
7678

77-
it("does not publish updates", async () => {
79+
it("deletes letters that are no longer pending", async () => {
7880
const handler = createHandler(mockedDeps);
7981
const oldLetter = generateLetter("PENDING");
80-
const newLetter = generateLetter("PENDING");
82+
const newLetter = generateLetter("ACCEPTED");
8183

8284
const testData = generateKinesisEvent([
8385
generateModifyRecord(oldLetter, newLetter),
8486
]);
8587
const result = await handler(testData, mockDeep<Context>(), jest.fn());
8688

87-
expect(mockedDeps.letterQueueRepository.putLetter).not.toHaveBeenCalled();
89+
expect(
90+
mockedDeps.letterQueueRepository.deleteLetter,
91+
).toHaveBeenCalledWith("supplier1", "1");
8892
expect(result.batchItemFailures).toEqual([]);
8993
});
9094

9195
it("does not publish non-PENDING letters", async () => {
9296
const handler = createHandler(mockedDeps);
93-
const newLetter = generateLetter("PRINTED");
97+
const newLetter = generateLetter("ACCEPTED");
9498

9599
const testData = generateKinesisEvent([generateInsertRecord(newLetter)]);
96100
const result = await handler(testData, mockDeep<Context>(), jest.fn());
@@ -99,6 +103,22 @@ describe("update-letter-queue Lambda", () => {
99103
expect(result.batchItemFailures).toEqual([]);
100104
});
101105

106+
it("does not delete letters that are still PENDING", async () => {
107+
const handler = createHandler(mockedDeps);
108+
const oldLetter = generateLetter("PENDING");
109+
const newLetter = generateLetter("PENDING");
110+
111+
const testData = generateKinesisEvent([
112+
generateModifyRecord(oldLetter, newLetter),
113+
]);
114+
const result = await handler(testData, mockDeep<Context>(), jest.fn());
115+
116+
expect(
117+
mockedDeps.letterQueueRepository.deleteLetter,
118+
).not.toHaveBeenCalled();
119+
expect(result.batchItemFailures).toEqual([]);
120+
});
121+
102122
it("handles empty Records array", async () => {
103123
const handler = createHandler(mockedDeps);
104124
const testData = { Records: [] } as unknown as KinesisStreamEvent;
@@ -116,11 +136,10 @@ describe("update-letter-queue Lambda", () => {
116136
const newLetter = { id: "1", status: "PENDING" } as Letter;
117137

118138
const testData = generateKinesisEvent([generateInsertRecord(newLetter)]);
119-
await expect(
120-
handler(testData, mockDeep<Context>(), jest.fn()),
121-
).rejects.toThrow();
139+
const result = await handler(testData, mockDeep<Context>(), jest.fn());
122140

123141
expect(mockedDeps.letterQueueRepository.putLetter).not.toHaveBeenCalled();
142+
expect(result.batchItemFailures).toEqual([{ itemIdentifier: "seq-0" }]);
124143
});
125144

126145
it("returns on the first failure", async () => {
@@ -143,7 +162,7 @@ describe("update-letter-queue Lambda", () => {
143162
expect(result.batchItemFailures).toEqual([{ itemIdentifier: "seq-0" }]);
144163
});
145164

146-
it("does not treat a replayed event as a failure", async () => {
165+
it("does not treat a replayed insert as a failure", async () => {
147166
const handler = createHandler(mockedDeps);
148167
const newLetter1 = generateLetter("PENDING", "1");
149168
const newLetter2 = generateLetter("PENDING", "2");
@@ -160,6 +179,25 @@ describe("update-letter-queue Lambda", () => {
160179
expect(result.batchItemFailures).toEqual([]);
161180
});
162181

182+
it("does not treat a replayed delete as a failure", async () => {
183+
const handler = createHandler(mockedDeps);
184+
const oldLetter1 = generateLetter("PENDING", "1");
185+
const oldLetter2 = generateLetter("PENDING", "2");
186+
const newLetter1 = generateLetter("ACCEPTED", "1");
187+
const newLetter2 = generateLetter("ACCEPTED", "2");
188+
(mockedDeps.letterQueueRepository.putLetter as jest.Mock)
189+
.mockRejectedValueOnce(new LetterDoesNotExistError("supplier1", "1"))
190+
.mockResolvedValueOnce({});
191+
192+
const testData = generateKinesisEvent([
193+
generateModifyRecord(oldLetter1, newLetter1),
194+
generateModifyRecord(oldLetter2, newLetter2),
195+
]);
196+
const result = await handler(testData, mockDeep<Context>(), jest.fn());
197+
198+
expect(result.batchItemFailures).toEqual([]);
199+
});
200+
163201
it("throws error when Kinesis payload cannot be parsed as JSON", async () => {
164202
const handler = createHandler(mockedDeps);
165203
const invalidJsonPayload = "not valid json {{{";
@@ -191,11 +229,12 @@ describe("update-letter-queue Lambda", () => {
191229
describe("Metrics", () => {
192230
it("emits success metrics when all letters are processed successfully", async () => {
193231
const handler = createHandler(mockedDeps);
194-
const newLetter1 = generateLetter("PENDING", "1");
232+
const oldLetter1 = generateLetter("PENDING", "1");
233+
const newLetter1 = generateLetter("ACCEPTED", "1");
195234
const newLetter2 = generateLetter("PENDING", "2");
196235

197236
const testData = generateKinesisEvent([
198-
generateInsertRecord(newLetter1),
237+
generateModifyRecord(oldLetter1, newLetter1),
199238
generateInsertRecord(newLetter2),
200239
]);
201240
await handler(testData, mockDeep<Context>(), jest.fn());
@@ -204,7 +243,7 @@ describe("update-letter-queue Lambda", () => {
204243
assertFailureMetricLogged(0);
205244
});
206245

207-
it("emits failure metrics when a letter fails to process", async () => {
246+
it("emits failure metrics when a letter fails to be inserted", async () => {
208247
const handler = createHandler(mockedDeps);
209248
const newLetter1 = generateLetter("PENDING", "1");
210249
const newLetter2 = generateLetter("PENDING", "2");
@@ -222,10 +261,31 @@ describe("update-letter-queue Lambda", () => {
222261
assertFailureMetricLogged(1);
223262
});
224263

225-
it("does not count a reprocessed event as a success or failure", async () => {
264+
it("emits failure metrics when a letter fails to be deleted", async () => {
265+
const handler = createHandler(mockedDeps);
266+
const oldLetter1 = generateLetter("PENDING", "1");
267+
const oldLetter2 = generateLetter("PENDING", "2");
268+
const newLetter1 = generateLetter("ACCEPTED", "1");
269+
const newLetter2 = generateLetter("ACCEPTED", "2");
270+
(mockedDeps.letterQueueRepository.deleteLetter as jest.Mock)
271+
.mockResolvedValueOnce({})
272+
.mockRejectedValueOnce(new Error("DynamoDB error"));
273+
274+
const testData = generateKinesisEvent([
275+
generateModifyRecord(oldLetter1, newLetter1),
276+
generateModifyRecord(oldLetter2, newLetter2),
277+
]);
278+
await handler(testData, mockDeep<Context>(), jest.fn());
279+
280+
assertSuccessMetricLogged(1);
281+
assertFailureMetricLogged(1);
282+
});
283+
284+
it("does not count a replayed insert as a success or failure", async () => {
226285
const handler = createHandler(mockedDeps);
227286
const newLetter1 = generateLetter("PENDING", "1");
228287
const newLetter2 = generateLetter("PENDING", "2");
288+
229289
(mockedDeps.letterQueueRepository.putLetter as jest.Mock)
230290
.mockRejectedValueOnce(new LetterAlreadyExistsError("supplier1", "1"))
231291
.mockResolvedValueOnce({});
@@ -240,6 +300,26 @@ describe("update-letter-queue Lambda", () => {
240300
assertFailureMetricLogged(0);
241301
});
242302

303+
it("does not count a replayed delete as a success or failure", async () => {
304+
const handler = createHandler(mockedDeps);
305+
const oldLetter1 = generateLetter("PENDING", "1");
306+
const oldLetter2 = generateLetter("PENDING", "2");
307+
const newLetter1 = generateLetter("ACCEPTED", "1");
308+
const newLetter2 = generateLetter("ACCEPTED", "2");
309+
(mockedDeps.letterQueueRepository.deleteLetter as jest.Mock)
310+
.mockRejectedValueOnce(new LetterDoesNotExistError("supplier1", "1"))
311+
.mockResolvedValueOnce({});
312+
313+
const testData = generateKinesisEvent([
314+
generateModifyRecord(oldLetter1, newLetter1),
315+
generateModifyRecord(oldLetter2, newLetter2),
316+
]);
317+
await handler(testData, mockDeep<Context>(), jest.fn());
318+
319+
assertSuccessMetricLogged(1);
320+
assertFailureMetricLogged(0);
321+
});
322+
243323
it("emits zero success metrics when no pending letters are in the batch", async () => {
244324
const handler = createHandler(mockedDeps);
245325
const newLetter = generateLetter("PRINTED");

0 commit comments

Comments
 (0)