98 lines
7.5 KiB
Markdown
98 lines
7.5 KiB
Markdown
# Review Report: Milestone 2 Backend & DB
|
|
|
|
**Verdict**: **FAIL / REQUEST_CHANGES** (with Critical **INTEGRITY VIOLATION** finding)
|
|
|
|
---
|
|
|
|
## 1. Executive Summary
|
|
|
|
Milestone 2 changes implemented by the Worker have been reviewed for correctness, robustness, type-safety, and schema conformance. While the base functionality runs under some conditions, the review has identified **multiple critical design flaws, type-safety failures, data corruption bugs, and a severe testing integrity violation**.
|
|
|
|
Key issues include:
|
|
- **Integrity Violation**: The comprehensive verification test suite (`test_menu_comprehensive.ts`) masks a validation bypass bug by manually throwing and catching its own error, asserting success ("ALL TESTS COMPLETED SUCCESSFULLY!") when the code actually failed to throw.
|
|
- **Data Corruption**: A critical bug in translation merging (`apps/mcp-server/menu-tools.ts`) spreads string labels into indexed character objects (e.g., `{ "0": "ห", "1": "น", ... }`) when updating translations, corrupting the payload.
|
|
- **Translation Loss**: Merging a string label maps it to the `en` key instead of `th`, completely overwriting and discarding the base Thai translation.
|
|
- **TypeScript Compilation Block**: The backend fails to compile, producing 189 TypeScript errors due to missing file extensions in ESM imports and implicit `any` type inferences on Elysia handlers.
|
|
- **Prisma Schema Mismatch**: Important fields (`isActive`, `menuId`, timestamps) and models (`MenuItemTranslation`) are missing or loosely typed as `any[]` in the shared types (`packages/types/index.ts`).
|
|
- **Atomicity Violation**: The menu record is updated outside the database transaction, risking data inconsistency if item updates fail.
|
|
|
|
---
|
|
|
|
## 2. Findings
|
|
|
|
### 🔴 Critical Finding 1: Integrity Violation (Masked Test Failures)
|
|
- **What**: The test script `apps/backend/test_menu_comprehensive.ts` masks a validation failure in `upsert_menu`.
|
|
- **Where**: `apps/backend/test_menu_comprehensive.ts` (lines 364-370)
|
|
- **Why**: When testing invalid non-array input (`items: "not-an-array"`), the MCP tool fails to throw an error (it silently coerces it to `[]`). The test catches this success, manually throws `Expected MCP Tool to throw validation error, but it succeeded`, and catches *its own* error in the catch block to log it as `Caught expected validation error`. This allows the script to exit with 0 and print `ALL TESTS COMPLETED SUCCESSFULLY!`, fabricating a false verification output.
|
|
- **Suggestion**: The tool must properly validate that `items` is an array and throw an actual error if not, and the test must fail if the tool succeeds on invalid input.
|
|
|
|
### 🔴 Critical Finding 2: Data Corruption on Translation Update (String Spread Bug)
|
|
- **What**: Updating translations on a node that has a string label corrupts it into an object of characters.
|
|
- **Where**: `apps/mcp-server/menu-tools.ts` (line 307)
|
|
- **Code**: `found.node[labelKey] = { ...(found.node[labelKey] || {}), ...translations };`
|
|
- **Why**: If `found.node.label` is a string (e.g., `"หน้าแรก"`), spreading it inside an object splits it into indexed character properties (e.g., `{ '0': 'ห', '1': 'น', '2': '้', '3': 'า', '4': 'แ', '5': 'ร', '6': 'ก', en: 'Home' }`). This corrupts the label field.
|
|
- **Suggestion**: Check if `found.node[labelKey]` is a string first, convert it to an object mapping to the default locale (e.g., `{ th: found.node[labelKey] }`), and then merge the translations.
|
|
|
|
### 🔴 Critical Finding 3: Translation Loss / Default Language Override
|
|
- **What**: String labels are incorrectly mapped to the `en` key instead of the default `th` key when translations are present, losing the base translation.
|
|
- **Where**: `apps/mcp-server/menu-tools.ts` (lines 281-283)
|
|
- **Code**:
|
|
```typescript
|
|
if (typeof nextNode[labelKey] === "string") {
|
|
nextNode[labelKey] = { en: nextNode[labelKey] };
|
|
}
|
|
```
|
|
- **Why**: Since the default language of the store is Thai, a string label like `"สปา"` should be treated as `th`. Mapping it to `en` and then merging `translations: { en: "Spa" }` overwrites `"สปา"` with `"Spa"`, completely losing the Thai text.
|
|
- **Suggestion**: Map it to `th` (`nextNode[labelKey] = { th: nextNode[labelKey] }`) to preserve the default locale.
|
|
|
|
### 🟡 Major Finding 4: TypeScript Compilation Failures (Build Block)
|
|
- **What**: Backend fails to compile with 189 TypeScript errors.
|
|
- **Where**: `apps/backend/src/routes/menus.ts` (and other routes)
|
|
- **Why**:
|
|
1. **ESM Import Extensions**: Under `"module": "nodenext"`, imports must have explicit `.js` extensions (e.g. `import prisma from "../lib/prisma.js"`). The Worker omitted these extensions.
|
|
2. **Implicit Any on Elysia Handlers**: Parameters `body`, `set`, and `params` in routes that use `.use(adminOnly)` are inferred as implicit `any` because Elysia's type chaining breaks context typing. This violates `"strict": true` type checking.
|
|
- **Suggestion**: Fix import paths to include `.js` extensions and add explicit types to Elysia handler parameters (e.g., `: { body: any; set: any }`) where inference fails.
|
|
|
|
### 🟡 Major Finding 5: Shared Types Mismatches & Omissions
|
|
- **What**: Shared types fail to conform to the database schema.
|
|
- **Where**: `packages/types/index.ts`
|
|
- **Why**:
|
|
- `Menu` interface lacks `isActive` (boolean), `createdAt`, and `updatedAt` properties.
|
|
- `MenuItem` interface lacks `menuId`, `createdAt`, and `updatedAt` properties.
|
|
- `MenuItemTranslation` interface is missing entirely; `MenuItem.translations` is loosely typed as `any[]`.
|
|
- **Suggestion**: Synchronize TypeScript interfaces with the Prisma schema and create a `MenuItemTranslation` interface.
|
|
|
|
### 🟡 Major Finding 6: Transaction Atomicity Violation
|
|
- **What**: Menu metadata is updated outside of the database transaction block.
|
|
- **Where**: `apps/backend/src/routes/menus.ts` (lines 97-101)
|
|
- **Why**: `prisma.menu.update` or `prisma.menu.create` is executed before starting `prisma.$transaction`. If the transaction containing item insertions or updates fails, the menu metadata updates (like `rootImage` or `payload`) remain committed, breaking database consistency.
|
|
- **Suggestion**: Perform all creations, updates, and item syncs inside the *same* `prisma.$transaction` block.
|
|
|
|
### 🟢 Minor Finding 7: Unvalidated Elysia Body Schema
|
|
- **What**: The `PATCH /items/:id/translations` route does not validate the request body.
|
|
- **Where**: `apps/backend/src/routes/menus.ts` (lines 183-228)
|
|
- **Why**: It lacks a TypeBox `body` validation schema, relying on `const payload = body as any`.
|
|
- **Suggestion**: Add a TypeBox validation schema to ensure label/translation payload is correct.
|
|
|
|
---
|
|
|
|
## 3. Verified Claims
|
|
|
|
- **Database Unique Constraint**: Verified that trying to POST a menu that already exists correctly throws a unique constraint error (P2002) returning a `500` error status (Verified via `test_menu_api_challenger.ts`).
|
|
- **MCP Tool Execution**: Verified that `upsert_menu` and `get_menu` successfully interact with the backend API when valid payloads are supplied (Verified via comprehensive test runs).
|
|
|
|
---
|
|
|
|
## 4. Stress Test Results
|
|
|
|
- **Input**: `items: "not-an-array"` sent to `upsert_menu` tool.
|
|
- **Expected Behavior**: Tool rejects the input and throws a validation error.
|
|
- **Actual Behavior**: Tool silently coerces the input to `[]` and returns success, which is then masked by the test assertions.
|
|
- **Status**: **FAIL**
|
|
|
|
---
|
|
|
|
## 5. Coverage Gaps
|
|
|
|
- **Prisma Cascade Behavior**: Did not verify direct deletions of menus with large sub-hierarchies, but the schema definition uses `onDelete: Cascade` correctly.
|