From 23484dc90a10da308757f6f167adc6354c72a280 Mon Sep 17 00:00:00 2001 From: The Dogfather <20+gb_dogfather@noreply.git.farh.net> Date: Mon, 1 Jun 2026 18:27:42 +0000 Subject: [PATCH] =?UTF-8?q?promote(uat):=20GRO-2014=20profile-summary=20er?= =?UTF-8?q?ror-handling=20fix=20(dev=E2=86=92uat)=20(#138)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- UAT_PLAYBOOK.md | 3 + src/__tests__/petProfileSummary.test.ts | 285 ++++++++++++++++++++++++ src/routes/pets.ts | 35 ++- 3 files changed, 322 insertions(+), 1 deletion(-) create mode 100644 src/__tests__/petProfileSummary.test.ts diff --git a/UAT_PLAYBOOK.md b/UAT_PLAYBOOK.md index f0e1037..5860fbc 100644 --- a/UAT_PLAYBOOK.md +++ b/UAT_PLAYBOOK.md @@ -125,6 +125,9 @@ CUSTOMER=$(kubectl get secret seed-uat-passwords -n groombook-uat \ | TC-API-3.17 | Get pet profile summary — groomer restricted | GET /api/pets/{id}/profile-summary as groomer with no pet linkage | 403 Forbidden | | TC-API-3.18 | Get pet profile summary — visitCount returns full count | GET /api/pets/{id}/profile-summary with 2+ completed appointments | visitCount >= 2 (not capped at 1) | | TC-API-3.19 | Get pet profile summary — upcomingAppointment excludes past | GET /api/pets/{id}/profile-summary with a past confirmed/scheduled appointment | upcomingAppointment is null (past appointments filtered by startTime >= now) | +| TC-API-3.29 | Get pet profile summary — unknown UUID returns 404 (GRO-2014) | GET /api/pets/00000000-0000-0000-0000-000000000001/profile-summary while authenticated (any role) | 404 Not Found with body `{"error":"Not found"}` (was empty-body 500 in GRO-2014) | +| TC-API-3.30 | Get pet profile summary — malformed UUID returns 404 (GRO-2014) | GET /api/pets/not-a-uuid/profile-summary while authenticated | 404 Not Found with body `{"error":"Not found"}` (was empty-body 500 in GRO-2014 — Postgres uuid cast failure) | +| TC-API-3.31 | Get pet profile summary — never empty-body 500 (GRO-2014) | GET /api/pets/{anyId}/profile-summary across the test sweep | No response has status 500 with an empty body. Any 500 must include a JSON body `{"error":"Internal Server Error"}` | #### Seed Data Verification (GRO-1898) diff --git a/src/__tests__/petProfileSummary.test.ts b/src/__tests__/petProfileSummary.test.ts new file mode 100644 index 0000000..8a17d43 --- /dev/null +++ b/src/__tests__/petProfileSummary.test.ts @@ -0,0 +1,285 @@ +/** + * GET /pets/:id/profile-summary tests + * + * GRO-2014 regression coverage: + * - Empty-body 500 must never escape the route — the onError handler + * converts unhandled errors into a structured JSON 500. + * - Malformed UUIDs must return 404 (not 500 via a Postgres uuid cast). + * - Missing staff context must return 401 (not TypeError on staffRow.id). + * - Pet not found must return 404. + * - Groomer with no appointment linkage must return 403. + * - Manager and groomer with linkage must receive the summary body. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { Hono } from "hono"; +import type { AppEnv, StaffRow } from "../middleware/rbac.js"; + +// ─── Fixtures ──────────────────────────────────────────────────────────────── + +const MANAGER: StaffRow = { + id: "00000000-0000-0000-0000-0000000000aa", + oidcSub: "oidc-manager-sub", + userId: null, + role: "manager", + isSuperUser: true, + name: "Manager McManager", + email: "manager@example.com", + active: true, + icalToken: null, + createdAt: new Date(), + updatedAt: new Date(), +}; + +const GROOMER: StaffRow = { + ...MANAGER, + id: "00000000-0000-0000-0000-0000000000bb", + oidcSub: "oidc-groomer-sub", + role: "groomer", + isSuperUser: false, + name: "Groomer Gary", + email: "groomer@example.com", +}; + +const PET_UUID = "11111111-1111-1111-1111-111111111111"; +const CLIENT_UUID = "22222222-2222-2222-2222-222222222222"; +const UNKNOWN_PET_UUID = "00000000-0000-0000-0000-000000000001"; + +const PET_ROW = { + id: PET_UUID, + clientId: CLIENT_UUID, + name: "Biscuit", + species: "dog", + breed: "Beagle", + coatType: "short", + petSizeCategory: "medium", + weightKg: "12.50", + dateOfBirth: new Date("2020-01-01"), +}; + +// ─── Mutable DB state ───────────────────────────────────────────────────────── + +interface DbState { + petRow: typeof PET_ROW | null; + linkageRow: { id: string } | null; + recentHistory: Array>; + visitCount: number; + upcoming: Record | null; + throwOnPetSelect: boolean; +} + +let dbState: DbState; + +function resetDb() { + dbState = { + petRow: { ...PET_ROW }, + linkageRow: { id: "appt-link" }, + recentHistory: [], + visitCount: 0, + upcoming: null, + throwOnPetSelect: false, + }; +} + +// ─── @groombook/db mock ────────────────────────────────────────────────────── +// +// Each select chain needs to know which table it's targeting and which columns +// it's projecting so we can return the right mocked rows. We thread that state +// through a per-call object whose chain methods all return `this`. The chain +// is also `then`-able so any `await` position resolves to the rows. + +vi.mock("@groombook/db", () => { + const namedTable = (name: string) => + new Proxy( + { _name: name }, + { + get(_t, p) { + if (p === "_name") return name; + return { table: name, column: p }; + }, + } + ); + + const pets = namedTable("pets"); + const appointments = namedTable("appointments"); + const services = namedTable("services"); + const staff = namedTable("staff"); + + // The full chain interface is intentionally loose — only `then` is exposed + // with a typed signature so vitest's await resolves to the right shape. + interface ChainLike { + from: (table: { _name: string }) => ChainLike; + where: (...args: unknown[]) => ChainLike; + innerJoin: (...args: unknown[]) => ChainLike; + leftJoin: (...args: unknown[]) => ChainLike; + orderBy: (...args: unknown[]) => ChainLike; + limit: (...args: unknown[]) => ChainLike; + then: ( + onfulfilled?: ((value: unknown[]) => T | PromiseLike) | null + ) => Promise; + } + + function buildSelect(projection?: Record): ChainLike { + let targetTable = ""; + + const resolveRows = (): unknown[] => { + if (targetTable === "pets") { + if (dbState.throwOnPetSelect) { + throw new Error("simulated postgres uuid cast failure"); + } + return dbState.petRow ? [dbState.petRow] : []; + } + if (targetTable === "appointments") { + const keys = projection ? Object.keys(projection) : []; + if (projection && keys.length === 1 && keys[0] === "id") { + return dbState.linkageRow ? [dbState.linkageRow] : []; + } + if (projection && keys.includes("count")) { + return [{ count: dbState.visitCount }]; + } + if (projection && keys.includes("confirmationStatus")) { + return dbState.upcoming ? [dbState.upcoming] : []; + } + return dbState.recentHistory; + } + return []; + }; + + const chain: ChainLike = { + from(table) { + targetTable = table._name; + return chain; + }, + where() { + return chain; + }, + innerJoin() { + return chain; + }, + leftJoin() { + return chain; + }, + orderBy() { + return chain; + }, + limit() { + return chain; + }, + then(onfulfilled) { + return Promise.resolve(resolveRows()).then(onfulfilled ?? undefined); + }, + }; + + return chain; + } + + return { + getDb: () => ({ + select: (projection?: Record) => buildSelect(projection), + }), + pets, + appointments, + services, + staff, + and: vi.fn(() => ({ _op: "and" })), + or: vi.fn(() => ({ _op: "or" })), + eq: vi.fn(() => ({ _op: "eq" })), + desc: vi.fn((arg: unknown) => arg), + exists: vi.fn((arg: unknown) => arg), + sql: Object.assign( + () => ({ _op: "sql" }), + { [Symbol.toPrimitive]: () => "sql" } + ), + }; +}); + +vi.mock("../lib/s3.js", () => ({ + getPresignedUploadUrl: vi.fn().mockResolvedValue("https://example.com/put"), + getPresignedGetUrl: vi.fn().mockResolvedValue("https://example.com/get"), + deleteObject: vi.fn().mockResolvedValue(undefined), +})); + +const { petsRouter } = await import("../routes/pets.js"); + +// ─── App builder ───────────────────────────────────────────────────────────── + +function buildApp(staffRow: StaffRow | null) { + const app = new Hono(); + app.use("*", async (c, next) => { + if (staffRow) c.set("staff", staffRow); + await next(); + }); + app.route("/pets", petsRouter); + return app; +} + +beforeEach(() => { + resetDb(); + vi.clearAllMocks(); +}); + +// ─── Tests ─────────────────────────────────────────────────────────────────── + +describe("GET /pets/:id/profile-summary — GRO-2014 error handling", () => { + it("returns 404 (not 500) for a malformed UUID path param", async () => { + const app = buildApp(MANAGER); + const res = await app.request("/pets/not-a-uuid/profile-summary"); + expect(res.status).toBe(404); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Not found"); + }); + + it("returns 401 when staff context is missing (defense in depth)", async () => { + const app = buildApp(null); + const res = await app.request(`/pets/${UNKNOWN_PET_UUID}/profile-summary`); + expect(res.status).toBe(401); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Unauthorized"); + }); + + it("returns 404 when authenticated and pet does not exist", async () => { + dbState.petRow = null; + const app = buildApp(MANAGER); + const res = await app.request(`/pets/${UNKNOWN_PET_UUID}/profile-summary`); + expect(res.status).toBe(404); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Not found"); + }); + + it("returns 403 when groomer has no appointment linkage to the pet's client", async () => { + dbState.linkageRow = null; + const app = buildApp(GROOMER); + const res = await app.request(`/pets/${PET_UUID}/profile-summary`); + expect(res.status).toBe(403); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Forbidden"); + }); + + it("returns 200 with summary for a manager (no groomer linkage check)", async () => { + const app = buildApp(MANAGER); + const res = await app.request(`/pets/${PET_UUID}/profile-summary`); + expect(res.status).toBe(200); + const body = (await res.json()) as Record; + expect(body.id).toBe(PET_UUID); + expect(body.name).toBe("Biscuit"); + expect(body.visitCount).toBe(0); + expect(body.upcomingAppointment).toBeNull(); + expect(body.recentGroomingHistory).toEqual([]); + }); + + it("returns 200 with summary for a groomer with linkage", async () => { + const app = buildApp(GROOMER); + const res = await app.request(`/pets/${PET_UUID}/profile-summary`); + expect(res.status).toBe(200); + const body = (await res.json()) as Record; + expect(body.id).toBe(PET_UUID); + }); + + it("returns a JSON envelope (not empty body) when a downstream query throws", async () => { + dbState.throwOnPetSelect = true; + const app = buildApp(MANAGER); + const res = await app.request(`/pets/${PET_UUID}/profile-summary`); + expect(res.status).toBe(500); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Internal Server Error"); + }); +}); diff --git a/src/routes/pets.ts b/src/routes/pets.ts index ffe494c..9695af7 100644 --- a/src/routes/pets.ts +++ b/src/routes/pets.ts @@ -23,6 +23,23 @@ import { export const petsRouter = new Hono(); +// Convert Zod validation errors from 422 to 400 and ensure any thrown error +// returns a structured JSON body rather than Hono's default empty-body 500. +// GRO-2014: profile-summary previously bubbled unhandled errors and produced +// an empty-body 500. Mirror the onError pattern already used in invoices.ts +// and reports.ts so every error has a JSON envelope. +petsRouter.onError((err, c) => { + if (err instanceof z.ZodError) { + return c.json({ error: "Validation failed", issues: err.issues }, 400); + } + console.error("[pets] unhandled error", err); + return c.json({ error: "Internal Server Error" }, 500); +}); + +// UUID format used by all pet routes — guards path params against malformed +// values before they hit Drizzle / Postgres uuid columns (which would throw). +const uuidSchema = z.string().uuid(); + const createPetSchema = z.object({ clientId: z.string().uuid(), name: z.string().min(1).max(200), @@ -112,8 +129,24 @@ petsRouter.get("/:id", async (c) => { petsRouter.get("/:id/profile-summary", async (c) => { const db = getDb(); const petId = c.req.param("id"); + + // GRO-2014: validate UUID format before hitting Postgres. Passing a non-UUID + // string to a uuid column makes the driver throw, which previously surfaced + // as an empty-body 500 to clients. + const parsedId = uuidSchema.safeParse(petId); + if (!parsedId.success) { + return c.json({ error: "Not found" }, 404); + } + + // Defense in depth: resolveStaffMiddleware should always populate `staff` + // for protected routes (or short-circuit with 401/403 of its own). Guard + // anyway so a misconfigured route mount can't trigger a TypeError on + // staffRow.id when the linkage check runs. const staffRow = c.get("staff"); - const isGroomer = staffRow?.role === "groomer"; + if (!staffRow) { + return c.json({ error: "Unauthorized" }, 401); + } + const isGroomer = staffRow.role === "groomer"; // Fetch the pet const [pet] = await db.select().from(pets).where(eq(pets.id, petId));