From 1d2744a1092516bd34d0dd18a2a5e32876e089e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:01:14 +0200 Subject: [PATCH] fix(api): give every offset-paged list a unique tiebreaker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/api/measurements` and `/api/labs` sort on one non-unique column and page with `skip`/`take`. When the sort key ties, Postgres does not promise the same order for the query that fetches page N and the one that fetches page N+1, so a client paging its own history can receive a row twice and never receive another — silently, on both sides. Ties are not hypothetical here. The partial unique index forbids two live measurements of the same type at one instant, but the tie that occurs in production is the cross-type one: a daily `stats:` rollup posts steps, distance, flights and active energy at the identical timestamp, and an unfiltered history list pages straight through the block. A lab panel drawn in one sitting shares `takenAt` across every analyte in it. Appending `{ id: }` to the `orderBy` makes the order total. Same fix on the five siblings that page the same way — medication intake history, illness day-logs, custom-metric entries, reminder history, mental-health assessments — because the defect is the shape, not the two routes it was found on. No wire change; `/api/sync/changes` already ordered this way. The fixtures seed ids in an order unrelated to insertion order, so the assertion on order inside the tied block can only hold if the secondary key is doing the work: with the tiebreaker removed the new file fails on all three cases. --- .../api/custom-metrics/[id]/entries/route.ts | 4 +- .../illness/episodes/[id]/day-logs/route.ts | 3 +- src/app/api/labs/route.ts | 6 +- .../[id]/history/route.ts | 3 +- src/app/api/measurements/route.ts | 10 +- src/app/api/medications/[id]/intake/route.ts | 11 +- .../api/mental-health/assessments/route.ts | 4 +- .../list-pagination-tiebreaker.test.ts | 214 ++++++++++++++++++ 8 files changed, 247 insertions(+), 8 deletions(-) create mode 100644 tests/integration/list-pagination-tiebreaker.test.ts diff --git a/src/app/api/custom-metrics/[id]/entries/route.ts b/src/app/api/custom-metrics/[id]/entries/route.ts index 71a54f7e0..5fdd9ff4b 100644 --- a/src/app/api/custom-metrics/[id]/entries/route.ts +++ b/src/app/api/custom-metrics/[id]/entries/route.ts @@ -62,7 +62,9 @@ export const GET = apiHandler( const [rows, total] = await Promise.all([ prisma.customMetricEntry.findMany({ where, - orderBy: { measuredAt: sortDir }, + // Unique tiebreaker — see the measurements list: offset paging over + // a non-unique sort key can repeat and drop rows between pages. + orderBy: [{ measuredAt: sortDir }, { id: sortDir }], take: limit, skip: offset, }), diff --git a/src/app/api/illness/episodes/[id]/day-logs/route.ts b/src/app/api/illness/episodes/[id]/day-logs/route.ts index 9d01977df..2202f338f 100644 --- a/src/app/api/illness/episodes/[id]/day-logs/route.ts +++ b/src/app/api/illness/episodes/[id]/day-logs/route.ts @@ -88,7 +88,8 @@ export const GET = apiHandler( const [rows, total] = await Promise.all([ prisma.illnessDayLog.findMany({ where, - orderBy: { date: sortDir }, + // Unique tiebreaker — `date` is a calendar day and repeats. + orderBy: [{ date: sortDir }, { id: sortDir }], take: limit, skip: offset, include: dayLogSymptomInclude, diff --git a/src/app/api/labs/route.ts b/src/app/api/labs/route.ts index b420b745e..97fc344ce 100644 --- a/src/app/api/labs/route.ts +++ b/src/app/api/labs/route.ts @@ -105,7 +105,11 @@ export const GET = apiHandler(async (request: NextRequest) => { const [rows, total] = await Promise.all([ prisma.labResult.findMany({ where, - orderBy: { takenAt: sortDir }, + // Unique tiebreaker: `takenAt` is a day-resolution timestamp, so a + // panel drawn in one sitting ties across every analyte in it. Offset + // paging over a non-unique sort key can repeat a row on one page and + // drop another, invisibly to both sides. + orderBy: [{ takenAt: sortDir }, { id: sortDir }], take: limit, skip: offset, include: { diff --git a/src/app/api/measurement-reminders/[id]/history/route.ts b/src/app/api/measurement-reminders/[id]/history/route.ts index 19872c4fc..df16f4a93 100644 --- a/src/app/api/measurement-reminders/[id]/history/route.ts +++ b/src/app/api/measurement-reminders/[id]/history/route.ts @@ -45,7 +45,8 @@ export const GET = apiHandler( const [rows, total] = await Promise.all([ prisma.measurementReminderEvent.findMany({ where, - orderBy: { occurredAt: "desc" }, + // Unique tiebreaker — several reminder events can share one instant. + orderBy: [{ occurredAt: "desc" }, { id: "desc" }], take: limit, skip: offset, }), diff --git a/src/app/api/measurements/route.ts b/src/app/api/measurements/route.ts index d8f76ada0..87b22745d 100644 --- a/src/app/api/measurements/route.ts +++ b/src/app/api/measurements/route.ts @@ -544,7 +544,15 @@ export const GET = apiHandler(async (request: NextRequest) => { const [measurements, total] = await Promise.all([ prisma.measurement.findMany({ where, - orderBy: { [sortBy]: sortDir }, + // `{ id: sortDir }` is the tiebreaker, not decoration. `measuredAt` + // is not unique here and ties are the normal case — an Apple Health + // import stamps long runs with one instant, and the + // `stats::YYYY-MM-DD` daily-rollup convention puts every daily + // total at the same one. Without a unique secondary key Postgres is + // free to order the tied block differently between the query for page + // N and the query for page N+1, so a client paging its own history + // silently gets one row twice and never gets another. + orderBy: [{ [sortBy]: sortDir }, { id: sortDir }], take: limit, skip: offset, }), diff --git a/src/app/api/medications/[id]/intake/route.ts b/src/app/api/medications/[id]/intake/route.ts index 6d684b667..635ee69f1 100644 --- a/src/app/api/medications/[id]/intake/route.ts +++ b/src/app/api/medications/[id]/intake/route.ts @@ -729,10 +729,17 @@ export const GET = apiHandler( // the history view. Pinning them last keeps the descending order // reading today → yesterday → … with real timestamps first. Other // sort columns are non-null so they keep the simple shape. + // The trailing `{ id: sortDir }` is the unique tiebreaker. Scheduled + // slots share an instant by construction — a twice-daily medication puts + // every morning dose on the same `scheduledFor` — so offset paging over + // the primary key alone can repeat a row on one page and drop another. const orderBy = sortBy === "takenAt" - ? { takenAt: { sort: sortDir, nulls: "last" as const } } - : { [sortBy]: sortDir }; + ? [ + { takenAt: { sort: sortDir, nulls: "last" as const } }, + { id: sortDir }, + ] + : [{ [sortBy]: sortDir }, { id: sortDir }]; const [events, total] = await Promise.all([ prisma.medicationIntakeEvent.findMany({ diff --git a/src/app/api/mental-health/assessments/route.ts b/src/app/api/mental-health/assessments/route.ts index a37b223b3..4229ecfd3 100644 --- a/src/app/api/mental-health/assessments/route.ts +++ b/src/app/api/mental-health/assessments/route.ts @@ -123,7 +123,9 @@ export const GET = apiHandler(async (request: NextRequest) => { deletedAt: null, ...(instrument ? { instrument } : {}), }, - orderBy: { takenAt: "desc" }, + // Unique tiebreaker — `takenAt` is day-resolution and two instruments + // taken in one sitting tie. + orderBy: [{ takenAt: "desc" }, { id: "desc" }], take: limit ?? 100, skip: offset ?? 0, select: { diff --git a/tests/integration/list-pagination-tiebreaker.test.ts b/tests/integration/list-pagination-tiebreaker.test.ts new file mode 100644 index 000000000..171aa9aad --- /dev/null +++ b/tests/integration/list-pagination-tiebreaker.test.ts @@ -0,0 +1,214 @@ +/** + * Offset pagination over a tied sort key must not repeat or drop a row. + * + * `/api/measurements` and `/api/labs` page with `skip`/`take` over a single + * non-unique column. Ties are the normal case on both: an Apple Health import + * stamps long runs of samples with one instant and the documented + * `stats::YYYY-MM-DD` rollup convention puts every daily total at + * the same one, while a lab panel drawn in one sitting shares `takenAt` across + * every analyte in it. Without a unique secondary key Postgres may order the + * tied block differently for the query that fetches page N and the one that + * fetches page N+1, so a client mirroring its own history silently receives + * one row twice and never receives another. For a health record that is a + * data-integrity bug, and it is invisible to both sides. + * + * The fixtures are seeded with explicit ids in a deliberately scrambled order, + * so heap order and id order disagree. That is what makes this a real gate: + * asserting only that two pages are disjoint would pass on an unordered scan + * that happened to be stable, whereas the id ordering inside a tied block can + * only hold if the secondary sort key is actually there. + */ +import { NextRequest } from "next/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { cookieJar } from "./mock-next-headers"; +import { getPrismaClient, truncateAllTables } from "./setup"; + +const TEST_USER_ID = "user-pagination-tiebreaker"; +const TIED_AT = new Date("2026-03-04T09:00:00.000Z"); + +vi.mock("next/headers", async () => { + const { cookieJar, headerJar } = await import("./mock-next-headers"); + return { + headers: vi.fn(async () => ({ + get: (name: string) => headerJar.get(name.toLowerCase()) ?? null, + })), + cookies: vi.fn(async () => ({ + get: (name: string) => { + const value = cookieJar.get(name); + return value ? { name, value } : undefined; + }, + set: (name: string, value: string) => { + cookieJar.set(name, value); + }, + delete: (name: string) => { + cookieJar.delete(name); + }, + })), + }; +}); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +/** + * Insertion order deliberately unrelated to id order: 04, 09, 01, 06, 00, … + * A scan that ignores the secondary key returns this sequence; one that + * honours it returns 09, 08, 07, … on a descending sort. + */ +const SCRAMBLED = [4, 9, 1, 6, 0, 8, 3, 7, 2, 5]; + +/** + * One type per row. The partial unique index on + * `(user_id, type, measured_at, source, sleep_stage)` forbids two live rows of + * the SAME type at one instant, so the tie that actually occurs in production + * is the cross-type one: a daily `stats:` rollup posts steps, distance, + * flights, active energy and the rest at the identical timestamp, and an + * unfiltered history list pages straight through the block. + */ +const TYPES = [ + "ACTIVITY_STEPS", + "FLIGHTS_CLIMBED", + "WALKING_RUNNING_DISTANCE", + "ACTIVE_ENERGY_BURNED", + "RESTING_HEART_RATE", + "HEART_RATE_VARIABILITY", + "OXYGEN_SATURATION", + "RESPIRATORY_RATE", + "BODY_TEMPERATURE", + "WEIGHT", +] as const; + +beforeEach(async () => { + const prisma = getPrismaClient(); + await truncateAllTables(prisma); + cookieJar.clear(); + await prisma.user.create({ + data: { + id: TEST_USER_ID, + username: "pagination-tiebreaker", + email: "pagination-tiebreaker@example.test", + }, + }); + const session = await prisma.session.create({ + data: { + userId: TEST_USER_ID, + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + }, + }); + cookieJar.set("healthlog_session", session.id); + + // Every row shares one `measuredAt` / `takenAt` — the whole page is one + // tied block, which is the worst case and the realistic one. + for (const n of SCRAMBLED) { + const suffix = String(n).padStart(2, "0"); + await prisma.measurement.create({ + data: { + id: `tie-measurement-${suffix}`, + userId: TEST_USER_ID, + type: TYPES[n], + value: 80 + n, + unit: "count", + measuredAt: TIED_AT, + source: "APPLE_HEALTH", + }, + }); + await prisma.labResult.create({ + data: { + id: `tie-lab-${suffix}`, + userId: TEST_USER_ID, + analyte: `Analyte ${suffix}`, + value: 1 + n, + unit: "mg/dl", + takenAt: TIED_AT, + }, + }); + } +}); + +function get(path: string): NextRequest { + return new NextRequest(`http://localhost${path}`); +} + +async function idsOf( + res: Response, + key: "measurements" | "results", +): Promise { + const body = (await res.json()) as { + data: Record>; + }; + return body.data[key].map((row) => row.id); +} + +describe("offset pagination over a tied sort key (real Postgres)", () => { + it("pages /api/measurements with no duplicate and no gap", async () => { + const { GET } = await import("@/app/api/measurements/route"); + + const first = await idsOf( + await GET(get("/api/measurements?limit=4&offset=0") as never), + "measurements", + ); + const second = await idsOf( + await GET(get("/api/measurements?limit=4&offset=4") as never), + "measurements", + ); + const third = await idsOf( + await GET(get("/api/measurements?limit=4&offset=8") as never), + "measurements", + ); + + const seen = [...first, ...second, ...third]; + expect(seen).toHaveLength(10); + expect(new Set(seen).size).toBe(10); + expect([...seen].sort()).toEqual( + SCRAMBLED.map((n) => `tie-measurement-${String(n).padStart(2, "0")}`) + .slice() + .sort(), + ); + + // The default sort is `measuredAt desc`; with every row tied, the + // tiebreaker is the only thing deciding the order, so it must be + // strictly descending by id across the page boundaries. + expect(seen).toEqual([...seen].sort().reverse()); + }); + + it("pages /api/labs with no duplicate and no gap", async () => { + const { GET } = await import("@/app/api/labs/route"); + + const first = await idsOf( + await GET(get("/api/labs?limit=4&offset=0") as never), + "results", + ); + const second = await idsOf( + await GET(get("/api/labs?limit=4&offset=4") as never), + "results", + ); + const third = await idsOf( + await GET(get("/api/labs?limit=4&offset=8") as never), + "results", + ); + + const seen = [...first, ...second, ...third]; + expect(seen).toHaveLength(10); + expect(new Set(seen).size).toBe(10); + expect(seen).toEqual([...seen].sort().reverse()); + }); + + it("keeps the ascending direction stable too", async () => { + const { GET } = await import("@/app/api/measurements/route"); + + const first = await idsOf( + await GET(get("/api/measurements?limit=5&offset=0&sortDir=asc") as never), + "measurements", + ); + const second = await idsOf( + await GET(get("/api/measurements?limit=5&offset=5&sortDir=asc") as never), + "measurements", + ); + + const seen = [...first, ...second]; + expect(new Set(seen).size).toBe(10); + expect(seen).toEqual([...seen].sort()); + }); +});