fix: deduplicate distractors against each other, guard thin distractor pool
This commit is contained in:
lila
parent
a02f3b139d
commit
a02d3b3335
4 changed files with 68 additions and 370 deletions
|
|
@ -42,4 +42,21 @@ describe("InMemoryGameSessionStore", () => {
|
|||
const result = await store.get("session-1");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("update persists modified session data", async () => {
|
||||
const data = {
|
||||
answers: new Map([
|
||||
["q1", 2],
|
||||
["q2", 1],
|
||||
]),
|
||||
userId: "user-1",
|
||||
};
|
||||
await store.create("session-1", data, 60_000);
|
||||
|
||||
const updated = { answers: new Map([["q2", 1]]), userId: "user-1" };
|
||||
await store.update("session-1", updated);
|
||||
|
||||
const result = await store.get("session-1");
|
||||
expect(result).toEqual(updated);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -32,7 +32,14 @@ const fakeTerms = [
|
|||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
mockGetGameTerms.mockResolvedValue(fakeTerms);
|
||||
mockGetDistractors.mockResolvedValue(["wrong1", "wrong2", "wrong3"]);
|
||||
mockGetDistractors.mockResolvedValue([
|
||||
"wrong1",
|
||||
"wrong2",
|
||||
"wrong3",
|
||||
"wrong4",
|
||||
"wrong5",
|
||||
"wrong6",
|
||||
]);
|
||||
});
|
||||
|
||||
describe("createGameSession", () => {
|
||||
|
|
@ -151,6 +158,39 @@ describe("createGameSession", () => {
|
|||
createGameSession(validRequest, store, "user-1"),
|
||||
).rejects.toThrow("db timeout");
|
||||
});
|
||||
|
||||
it("throws when fewer than 3 unique distractors remain after deduplication", async () => {
|
||||
mockGetDistractors.mockResolvedValueOnce([
|
||||
"cane",
|
||||
"cane",
|
||||
"cane",
|
||||
"cane",
|
||||
"cane",
|
||||
"cane",
|
||||
]);
|
||||
|
||||
await expect(
|
||||
createGameSession(validRequest, store, "user-1"),
|
||||
).rejects.toThrow("Not enough unique distractors");
|
||||
});
|
||||
|
||||
it("duplicate distractors are deduplicated against each other", async () => {
|
||||
mockGetDistractors.mockResolvedValueOnce([
|
||||
"wrong1",
|
||||
"wrong1",
|
||||
"wrong1",
|
||||
"wrong2",
|
||||
"wrong3",
|
||||
"wrong4",
|
||||
]);
|
||||
|
||||
const session = await createGameSession(validRequest, store, "user-1");
|
||||
const question = session.questions[0]!;
|
||||
const optionTexts = question.options.map((o) => o.text);
|
||||
|
||||
expect(new Set(optionTexts).size).toBe(4);
|
||||
expect(question.options).toHaveLength(4);
|
||||
});
|
||||
});
|
||||
|
||||
describe("evaluateAnswer", () => {
|
||||
|
|
|
|||
|
|
@ -42,9 +42,16 @@ export const createGameSession = async (
|
|||
6,
|
||||
);
|
||||
|
||||
const uniqueDistractors = distractorTexts.filter(
|
||||
(t) => t !== term.targetText,
|
||||
);
|
||||
const uniqueDistractors = [
|
||||
...new Set(distractorTexts.filter((t) => t !== term.targetText)),
|
||||
];
|
||||
|
||||
if (uniqueDistractors.length < 3) {
|
||||
throw new Error(
|
||||
`Not enough unique distractors for term: ${term.targetText}`,
|
||||
);
|
||||
}
|
||||
|
||||
const optionTexts = [term.targetText, ...uniqueDistractors.slice(0, 3)];
|
||||
const shuffledTexts = shuffleArray(optionTexts);
|
||||
const correctOptionId = shuffledTexts.indexOf(term.targetText);
|
||||
|
|
|
|||
|
|
@ -1,366 +0,0 @@
|
|||
# `gameService.ts` — Code Review & Fixes
|
||||
|
||||
---
|
||||
|
||||
## 1. Hardcoded singleton kills the abstraction
|
||||
|
||||
**Problem**
|
||||
|
||||
A `GameSessionStore` interface exists, an `InMemoryGameSessionStore` implements it, and then the concrete class is immediately hardcoded as a module-level singleton. The interface is decorative — nothing can inject an alternative implementation without editing this file.
|
||||
|
||||
```ts
|
||||
// ❌ current — store is unreachable from outside
|
||||
const gameSessionStore = new InMemoryGameSessionStore();
|
||||
|
||||
export const createGameSession = async (request: GameRequest) => { ... };
|
||||
export const evaluateAnswer = async (submission: AnswerSubmission) => { ... };
|
||||
```
|
||||
|
||||
**Fix — inject the store**
|
||||
|
||||
Accept the store as a parameter (or use a factory). The simplest approach that requires no framework:
|
||||
|
||||
```ts
|
||||
// ✅ inject the store
|
||||
export const createGameSession = async (
|
||||
request: GameRequest,
|
||||
store: GameSessionStore,
|
||||
): Promise<GameSession> => { ... };
|
||||
|
||||
export const evaluateAnswer = async (
|
||||
submission: AnswerSubmission,
|
||||
store: GameSessionStore,
|
||||
): Promise<AnswerResult> => { ... };
|
||||
```
|
||||
|
||||
The call site (controller) owns the store instance and passes it in. Tests can pass a fresh `InMemoryGameSessionStore` per test — no mocking required, no shared state.
|
||||
|
||||
```ts
|
||||
// gameController.ts
|
||||
const store = new InMemoryGameSessionStore();
|
||||
|
||||
// later, swap for ValKeyGameSessionStore with one line change
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 2. Sessions are never deleted — memory leak
|
||||
|
||||
**Problem**
|
||||
|
||||
`GameSessionStore.delete()` is defined and implemented but never called. Every session ever created stays in the Map until the process restarts. Under real traffic this is a slow memory leak; under a spike it's a fast one.
|
||||
|
||||
**Fix — delete after answer, or add a TTL**
|
||||
|
||||
The simplest fix: delete the session once the last question is answered. If partial completion is needed, add a TTL on creation instead.
|
||||
|
||||
```ts
|
||||
// ✅ option A — delete on answer
|
||||
export const evaluateAnswer = async (
|
||||
submission: AnswerSubmission,
|
||||
store: GameSessionStore,
|
||||
): Promise<AnswerResult> => {
|
||||
const session = await store.get(submission.sessionId);
|
||||
if (!session)
|
||||
throw new NotFoundError(`Game session not found: ${submission.sessionId}`);
|
||||
|
||||
const correctOptionId = session.answers.get(submission.questionId);
|
||||
if (correctOptionId === undefined)
|
||||
throw new NotFoundError(`Question not found: ${submission.questionId}`);
|
||||
|
||||
// delete answered question; delete session when all questions are answered
|
||||
session.answers.delete(submission.questionId);
|
||||
if (session.answers.size === 0) {
|
||||
await store.delete(submission.sessionId);
|
||||
}
|
||||
|
||||
return {
|
||||
questionId: submission.questionId,
|
||||
isCorrect: submission.selectedOptionId === correctOptionId,
|
||||
correctOptionId,
|
||||
selectedOptionId: submission.selectedOptionId,
|
||||
};
|
||||
};
|
||||
```
|
||||
|
||||
```ts
|
||||
// ✅ option B — TTL in InMemoryGameSessionStore
|
||||
export class InMemoryGameSessionStore implements GameSessionStore {
|
||||
private sessions = new Map<
|
||||
string,
|
||||
{ data: GameSessionData; expiresAt: number }
|
||||
>();
|
||||
private readonly ttlMs: number;
|
||||
|
||||
constructor(ttlMs = 30 * 60 * 1000) {
|
||||
// 30 minutes default
|
||||
this.ttlMs = ttlMs;
|
||||
}
|
||||
|
||||
create(sessionId: string, data: GameSessionData): Promise<void> {
|
||||
this.sessions.set(sessionId, { data, expiresAt: Date.now() + this.ttlMs });
|
||||
return Promise.resolve();
|
||||
}
|
||||
|
||||
get(sessionId: string): Promise<GameSessionData | null> {
|
||||
const entry = this.sessions.get(sessionId);
|
||||
if (!entry) return Promise.resolve(null);
|
||||
if (Date.now() > entry.expiresAt) {
|
||||
this.sessions.delete(sessionId);
|
||||
return Promise.resolve(null);
|
||||
}
|
||||
return Promise.resolve(entry.data);
|
||||
}
|
||||
|
||||
delete(sessionId: string): Promise<void> {
|
||||
this.sessions.delete(sessionId);
|
||||
return Promise.resolve();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
**Problem**
|
||||
|
||||
`GameRequest.rounds` is typed as `string` in `@lila/shared`, forcing the service to cast it every time:
|
||||
|
||||
```ts
|
||||
// ❌ why is a round count a string?
|
||||
Number(request.rounds);
|
||||
```
|
||||
|
||||
**Fix — fix the schema in `@lila/shared`**
|
||||
|
||||
```ts
|
||||
// ✅ in packages/shared
|
||||
export const GameRequestSchema = z.object({
|
||||
source_language: z.string(),
|
||||
target_language: z.string(),
|
||||
pos: z.string(),
|
||||
difficulty: z.string(),
|
||||
rounds: z.coerce.number().int().min(1).max(50), // coerce handles form inputs, validates range
|
||||
});
|
||||
|
||||
export type GameRequest = z.infer<typeof GameRequestSchema>;
|
||||
```
|
||||
|
||||
The `z.coerce.number()` handles the case where the value arrives as a string from a query param or form — Zod does the conversion at the boundary so the rest of the system never sees a string.
|
||||
|
||||
---
|
||||
|
||||
**Problem**
|
||||
|
||||
The variable holds `terms` — word pairs fetched from the database. Calling them `correctAnswers` jumps ahead semantically; they only become "correct answers" once options are constructed around them.
|
||||
|
||||
```ts
|
||||
// ❌ these are terms, not answers yet
|
||||
const correctAnswers = await getGameTerms(...);
|
||||
```
|
||||
|
||||
**Fix**
|
||||
|
||||
```ts
|
||||
// ✅
|
||||
const terms = await getGameTerms(...);
|
||||
|
||||
// and inside the map:
|
||||
terms.map(async (term) => {
|
||||
const distractorTexts = await getDistractors(
|
||||
term.termId,
|
||||
term.targetText,
|
||||
...
|
||||
);
|
||||
...
|
||||
const correctOptionId = shuffledTexts.indexOf(term.targetText);
|
||||
...
|
||||
});
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 6. Tautological test: `"distractors are never the correct answer"`
|
||||
|
||||
**Problem**
|
||||
|
||||
The test filters the correct answer out of the options array, then asserts the remaining items are not the correct answer. It is testing that `Array.filter` works.
|
||||
|
||||
```ts
|
||||
// ❌ this cannot fail
|
||||
it("distractors are never the correct answer", async () => {
|
||||
const distractorTexts = question.options
|
||||
.map((o) => o.text)
|
||||
.filter((t) => t !== correctText); // removes correct answer...
|
||||
|
||||
for (const text of distractorTexts) {
|
||||
expect(text).not.toBe(correctText); // ...then checks they're not the correct answer
|
||||
}
|
||||
});
|
||||
```
|
||||
|
||||
**What to actually test**
|
||||
|
||||
The real concern is that `getDistractors` doesn't return the target word. Test that the service handles it correctly if it does:
|
||||
|
||||
```ts
|
||||
// ✅ test that the correct answer appears exactly once even if a distractor collides
|
||||
it("correct answer appears exactly once in options even if distractor matches", async () => {
|
||||
// simulate getDistractors returning the correct answer as one of the distractors
|
||||
mockGetDistractors.mockResolvedValueOnce(["cane", "wrong2", "wrong3"]);
|
||||
|
||||
const session = await createGameSession(
|
||||
validRequest,
|
||||
new InMemoryGameSessionStore(),
|
||||
);
|
||||
const question = session.questions[0]!;
|
||||
const optionTexts = question.options.map((o) => o.text);
|
||||
|
||||
// "cane" should only appear once regardless of the duplicate from getDistractors
|
||||
expect(optionTexts.filter((t) => t === "cane")).toHaveLength(1);
|
||||
expect(question.options).toHaveLength(4);
|
||||
});
|
||||
```
|
||||
|
||||
> **Note:** the current implementation doesn't actually handle this case — a duplicate distractor would produce a 4-option list where the correct answer appears twice and one distractor slot is wasted. Worth fixing in `createGameSession` alongside the test.
|
||||
|
||||
---
|
||||
|
||||
## 7. Store not reset between tests
|
||||
|
||||
**Problem**
|
||||
|
||||
`beforeEach` calls `vi.clearAllMocks()` which resets mock functions, but the `gameSessionStore` module-level singleton is never cleared. Ghost sessions from earlier tests persist for the entire test run.
|
||||
|
||||
It doesn't bite today because each session gets a unique UUID and tests don't share IDs — but it's one non-UUID lookup away from a very confusing afternoon.
|
||||
|
||||
**Fix — a consequence of fixing issue #1**
|
||||
|
||||
Once the store is injected rather than module-level, each test creates its own instance:
|
||||
|
||||
```ts
|
||||
// ✅ no shared state, no ghost sessions
|
||||
describe("evaluateAnswer", () => {
|
||||
it("returns isCorrect: true for correct option", async () => {
|
||||
const store = new InMemoryGameSessionStore();
|
||||
const session = await createGameSession(validRequest, store);
|
||||
...
|
||||
const result = await evaluateAnswer({ ... }, store);
|
||||
...
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
No `beforeEach` cleanup needed — the store simply doesn't outlive the test that created it.
|
||||
|
||||
---
|
||||
|
||||
## 8. No answer replay protection
|
||||
|
||||
**Problem**
|
||||
|
||||
`evaluateAnswer` can be called multiple times with the same `questionId`. The
|
||||
service will evaluate it every time. In multiplayer this could be abused to
|
||||
farm points or desync state.
|
||||
|
||||
**Fix — delete the question from the answer key after first evaluation**
|
||||
|
||||
```ts
|
||||
// ✅ inside evaluateAnswer, after retrieving correctOptionId
|
||||
session.answers.delete(submission.questionId);
|
||||
|
||||
if (submission.selectedOptionId !== correctOptionId) {
|
||||
// already removed — can't retry
|
||||
}
|
||||
```
|
||||
|
||||
Once the question key is deleted, a second submission hits the
|
||||
`correctOptionId === undefined` branch and throws `NotFoundError`. One shot
|
||||
per question.
|
||||
|
||||
---
|
||||
|
||||
## 9. No ownership check in `evaluateAnswer`
|
||||
|
||||
**Problem**
|
||||
|
||||
The service accepts any `sessionId` without verifying it belongs to the
|
||||
requesting user. If auth middleware doesn't tie sessions to users at a higher
|
||||
layer, Alice can submit answers for Bob's session by guessing or intercepting
|
||||
his `sessionId`.
|
||||
|
||||
**Fix — store `userId` alongside the session and assert it on retrieval**
|
||||
|
||||
```ts
|
||||
// GameSessionStore.ts
|
||||
export type GameSessionData = { answers: Map<string, number>; userId: string };
|
||||
|
||||
// evaluateAnswer
|
||||
const session = await store.get(submission.sessionId);
|
||||
|
||||
if (!session) throw new NotFoundError(`Game session not found`);
|
||||
if (session.userId !== requestingUserId)
|
||||
throw new NotFoundError(`Game session not found`);
|
||||
// ^^^ same error — don't confirm the session exists to the wrong user
|
||||
```
|
||||
|
||||
Pass `requestingUserId` in from the controller, where it's already available
|
||||
via auth middleware.
|
||||
|
||||
---
|
||||
|
||||
## 10. No test for empty `getGameTerms` result
|
||||
|
||||
**Problem**
|
||||
|
||||
If the database returns zero terms (no words match the difficulty/language/pos
|
||||
filter), `createGameSession` happily returns a session with an empty
|
||||
`questions` array. The frontend receives it, tries to render question 1, and
|
||||
crashes. The user sees nothing useful.
|
||||
|
||||
**Fix — guard in the service and add a test**
|
||||
|
||||
```ts
|
||||
// ✅ inside createGameSession, after fetching terms
|
||||
if (terms.length === 0) {
|
||||
throw new AppError("No terms found for the given filters", 404);
|
||||
}
|
||||
```
|
||||
|
||||
```ts
|
||||
// ✅ test
|
||||
it("throws when getGameTerms returns no terms", async () => {
|
||||
mockGetGameTerms.mockResolvedValue([]);
|
||||
|
||||
await expect(
|
||||
createGameSession(validRequest, new InMemoryGameSessionStore()),
|
||||
).rejects.toThrow("No terms found");
|
||||
});
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 11. No test for `getDistractors` rejection
|
||||
|
||||
**Problem**
|
||||
|
||||
`createGameSession` uses `Promise.all` over the terms array. If
|
||||
`getDistractors` rejects for any single term, the entire `Promise.all` rejects
|
||||
— no session is created, no partial recovery, the user gets a 500 with
|
||||
"connection refused" leaking through.
|
||||
|
||||
**Fix — test the failure path and consider a fallback**
|
||||
|
||||
```ts
|
||||
// ✅ test
|
||||
it("propagates getDistractors failure", async () => {
|
||||
mockGetDistractors.mockRejectedValue(new Error("db timeout"));
|
||||
|
||||
await expect(
|
||||
createGameSession(validRequest, new InMemoryGameSessionStore()),
|
||||
).rejects.toThrow("db timeout");
|
||||
});
|
||||
```
|
||||
|
||||
For resilience, consider catching per-term distractor failures and falling back
|
||||
to random terms from the already-fetched set rather than collapsing the whole
|
||||
session.
|
||||
Loading…
Add table
Add a link
Reference in a new issue