129 lines
8.7 KiB
Markdown
129 lines
8.7 KiB
Markdown
# Review Report — Milestone 2: Backend & DB (Reviewer 2 - v2)
|
|
|
|
## Review Summary
|
|
|
|
**Verdict**: **FAIL** (Requires Changes)
|
|
|
|
*The implementation contains multiple critical and major issues regarding transaction atomicity, type safety, unmapped fields, missing validation schemas, and logical data structure corruption in the translation handling of the MCP tool.*
|
|
|
|
---
|
|
|
|
## Findings
|
|
|
|
### 1. [Critical] Transaction Integrity Violation in PUT Route
|
|
- **What**: In `PUT /menus/:name`, the parent `Menu` record is created or updated using the standard `prisma` client *before and outside* the `prisma.$transaction` block where `MenuItem` and `MenuItemTranslation` records are modified.
|
|
- **Where**: `apps/backend/src/routes/menus.ts` (Lines 97-101, 103-153)
|
|
- **Why**: If a database error, unique constraint violation, or connection drop occurs inside the transaction block, the transaction will roll back all modifications to menu items. However, the changes to the `Menu` record (such as update to `payload`, `isActive`, or `rootImage`) will *not* roll back. This violates the ACID properties of the database and can lead to corrupt menu states.
|
|
- **Suggestion**: Perform the `prisma.menu.findUnique`, `prisma.menu.create`, and `prisma.menu.update` calls inside the transaction callback, executing them on the `tx` transaction client.
|
|
|
|
### 2. [Major] Unmapped Fields in POST Route
|
|
- **What**: `POST /menus` ignores the `payload` and `isActive` fields, preventing menus from being fully configured on creation.
|
|
- **Where**: `apps/backend/src/routes/menus.ts` (Lines 32-73)
|
|
- **Why**: The Prisma schema defines `payload` (Json) and `isActive` (Boolean) on the `Menu` model. However, the POST endpoint's TypeBox schema and database mapping omit these fields, preventing users from initializing menu structures or changing active status via POST.
|
|
- **Suggestion**: Add `payload` and `isActive` to the TypeBox body schema and pass them to the `prisma.menu.create` data parameter.
|
|
|
|
### 3. [Major] Missing TypeBox Validation Schema on PATCH Endpoint
|
|
- **What**: The `.patch("/items/:id/translations")` route completely lacks an Elysia TypeBox validation schema.
|
|
- **Where**: `apps/backend/src/routes/menus.ts` (Lines 183-228)
|
|
- **Why**: Bypassing schema validation leaves the endpoint open to arbitrary payloads. The request body is manually parsed after casting to `any` (`const payload = body as any`), violating the project standard of strict TypeBox validation.
|
|
- **Suggestion**: Pass a TypeBox schema option defining the expected structure of `params` (validating `id` as a number) and `body`.
|
|
|
|
### 4. [Major] Relative Import Path Compilation Errors under NodeNext Resolution
|
|
- **What**: Relative import paths are missing explicit `.js` file extensions.
|
|
- **Where**: `apps/backend/src/routes/menus.ts` (Lines 2-3)
|
|
- **Why**: Under ECMAScript Module Resolution (`NodeNext`/`Node16` configured in `tsconfig.json`), TypeScript strictly requires file extensions on relative imports. Without them, the project fails to compile, throwing `TS2835: Relative import paths need explicit file extensions...`.
|
|
- **Suggestion**: Append `.js` extensions:
|
|
```typescript
|
|
import prisma from "../lib/prisma.js";
|
|
import { adminOnly } from "../middleware/auth.js";
|
|
```
|
|
|
|
### 5. [Major] Return Type and Status Mismatch for Non-Existent Menu
|
|
- **What**: `GET /menus/:name` returns a mock object without an `id` field and a `200` OK status when a menu is not found.
|
|
- **Where**: `apps/backend/src/routes/menus.ts` (Lines 16-30)
|
|
- **Why**: The shared `Menu` interface in `packages/types/index.ts` requires `id: number`. Returning a mock object like `{ name, items: [], payload: null, rootImage: null, isActive: false }` lacks an `id` and violates type conformance. Returning `200` OK instead of `404 Not Found` also hides resource non-existence from the client.
|
|
- **Suggestion**: Set `set.status = 404` and return a structured error message like `{ error: "Menu not found" }` when a menu is missing.
|
|
|
|
### 6. [Minor] Shared Types Menu Interface is Missing `isActive`
|
|
- **What**: The shared `Menu` interface does not include `isActive`.
|
|
- **Where**: `packages/types/index.ts` (Lines 205-211)
|
|
- **Why**: The database model defines `isActive Boolean @default(true)`. Missing it in the TypeScript declaration causes compilation or type-safety issues for consumers.
|
|
- **Suggestion**: Add `isActive: boolean` to the `Menu` interface.
|
|
|
|
### 7. [Major] Translation Array Spreading Bug in MCP Tools
|
|
- **What**: The `applyMenuAddNode` helper spreads `nextNode.translations` directly into an object, even if it is an array.
|
|
- **Where**: `apps/mcp-server/menu-tools.ts` (Lines 272-300)
|
|
- **Why**: `validateMenuNodeData` permits `translations` to be an array or object. If a list of translations is passed as an array (e.g. `[{ langCode: "th", label: "..." }]`), spreading it creates index-based keys (like `{ "0": { langCode: "th", label: "..." } }`) instead of language code keys. This corrupts the translatable object structure in the database.
|
|
- **Suggestion**: Convert the array to a key-value dictionary (e.g., `{ th: "..." }`) before merging.
|
|
|
|
### 8. [Minor] Bypassing Type Safety with `tx: any`
|
|
- **What**: The database transaction client is cast to `any`.
|
|
- **Where**: `apps/backend/src/routes/menus.ts` (Line 103)
|
|
- **Why**: Disabling type safety within the transaction block makes it easy to introduce undetected runtime errors during schema changes.
|
|
- **Suggestion**: Remove the `any` cast and type the transaction client properly.
|
|
|
|
---
|
|
|
|
## Verified Claims
|
|
|
|
- **Claim**: Menu API supports rootImage, columnImage, and hoverImage storage and retrieval → Verified via running `apps/backend/test_menu_api_challenger.ts` → **PASS** (retrieved values match input payloads exactly).
|
|
- **Claim**: Elysia routes perform validation checks on rootImage, columnImage, and order fields → Verified via running `test_menu_api_challenger.ts` → **PASS** (correctly throws 422 Unprocessable Entity for invalid types).
|
|
- **Claim**: Database schema correctly defines relation cascades → Verified via inspecting `schema.prisma` → **PASS** (cascades on delete of menus and menu items are present).
|
|
|
|
---
|
|
|
|
## Coverage Gaps
|
|
|
|
- **PATCH Endpoint Validation**: The `PATCH /menus/items/:id/translations` route has no validation schema. (Risk level: **Medium**; Recommendation: Add TypeBox validation).
|
|
- **POST Endpoint Mapping**: Missing `payload` and `isActive` fields on `POST /menus`. (Risk level: **Medium**; Recommendation: Map missing fields).
|
|
|
|
---
|
|
|
|
## Unverified Items
|
|
|
|
- **Frontend Integration**: Layout rendering and responsive constraints on frontend components. (Reason: Outside the scope of M2 Backend & DB review).
|
|
|
|
---
|
|
|
|
## Challenge Summary
|
|
|
|
**Overall risk assessment**: **HIGH**
|
|
|
|
*The combination of split transaction blocks (non-atomic updates) and logical bugs in translation array handling risks database state corruption that could crash storefront rendering of the Mega Menu.*
|
|
|
|
---
|
|
|
|
## Challenges
|
|
|
|
### 1. Non-Atomic Menu Updates
|
|
- **Assumption challenged**: PUT operations are atomic because they use `prisma.$transaction`.
|
|
- **Attack scenario**: A client PUTs an update to a menu payload. The Menu record is updated successfully, but a database error occurs during the subsequent MenuItem updates inside the transaction. The transaction rolls back, but the Menu changes remain committed.
|
|
- **Blast radius**: The Menu model metadata changes but its associated items do not, leaving the menu structure broken.
|
|
- **Mitigation**: Move the Menu creation/update query inside the transaction callback.
|
|
|
|
### 2. Translation Array Spreading Corruption
|
|
- **Assumption challenged**: Node translations are always plain objects.
|
|
- **Attack scenario**: A client calls `add_menu_node` with translations as an array. The tool spreads it, creating index keys.
|
|
- **Blast radius**: Corrupted database payload crashes storefront translation lookup.
|
|
- **Mitigation**: Detect array types and convert to dictionary.
|
|
|
|
### 3. Duplicate Menu 500 Leak
|
|
- **Assumption challenged**: Database uniqueness constraints are handled gracefully.
|
|
- **Attack scenario**: Creating a menu with an existing name throws P2002.
|
|
- **Blast radius**: The API crashes with `500 Internal Server Error` instead of `409 Conflict`, leaking DB constraints in the log.
|
|
- **Mitigation**: Intercept P2002 and return `409`.
|
|
|
|
---
|
|
|
|
## Stress Test Results
|
|
|
|
- **Duplicate Name POST** → Expect `400/409` → Got `500` → **FAIL**
|
|
- **PUT Transaction Failure** → Expect Rollback of all changes → Got partial update of Menu → **FAIL**
|
|
- **Array Translation on Node Add** → Expect `{ th: "..." }` → Got `{ "0": { langCode: "th", ... } }` → **FAIL**
|
|
|
|
---
|
|
|
|
## Unchallenged Areas
|
|
|
|
- **Menu Snapshot Storage**: Snapshot storage uses in-memory map which works fine but does not persist across server restarts (implied constraint for local tool execution).
|