mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-10 13:22:14 +00:00
fix(mood): make the bulk upsert and its tag links one transaction
`createTagLinks` validates `ratedFactors` after the mood row is written. On the bulk path the two ran as separate awaits on the base client, so an out-of-scale rating threw once the row had already committed: the per-entry catch reported "skipped" — the one status that promises nothing was saved — while a mood entry with no tags or factors sat on disk. The single-entry POST has always wrapped the pair in one transaction; the bulk twin now matches. The test client for the transaction is a distinct object from the base client on purpose. Sharing one identity made the atomicity assertion vacuous: a write that escaped the transaction looked exactly like one that stayed inside it.
This commit is contained in:
1 parent
4b87b9e33b
commit
9d03fe13f0
3 files changed
+163
-66
No files matched your search
@@ -13,13 +13,30 @@
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { NextRequest } from "next/server";
|
||||
|
||||
// The transaction client is a DISTINCT object from `prisma` — otherwise a
|
||||
// write that escapes the transaction (handed the base client) is
|
||||
// indistinguishable from one that stays inside it, and the atomicity
|
||||
// assertion below is vacuous. It SHARES the model mocks so every existing
|
||||
// per-model assertion in this file keeps working unchanged.
|
||||
const { moodEntryMock, txClient } = vi.hoisted(() => {
|
||||
const moodEntryMock = { findMany: vi.fn(), upsert: vi.fn() };
|
||||
return {
|
||||
moodEntryMock,
|
||||
txClient: { __brand: "transaction-client", moodEntry: moodEntryMock },
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("@/lib/db", () => ({
|
||||
prisma: {
|
||||
moodEntry: {
|
||||
findMany: vi.fn(),
|
||||
upsert: vi.fn(),
|
||||
},
|
||||
moodEntry: moodEntryMock,
|
||||
auditLog: { create: vi.fn() },
|
||||
$transaction: vi.fn(async (fn: unknown) => {
|
||||
if (typeof fn === "function") {
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
return (fn as any)(txClient);
|
||||
}
|
||||
return undefined;
|
||||
}),
|
||||
},
|
||||
}));
|
||||
|
||||
@@ -247,7 +264,9 @@ describe("POST /api/mood-entries/bulk — structured tagKeys (v1.12.0)", () => {
|
||||
"entry-1",
|
||||
"user-1",
|
||||
["movies", "gaming"],
|
||||
prisma,
|
||||
// The TRANSACTION client, not the base client — the write must not
|
||||
// escape the transaction the mood row is being upserted in.
|
||||
txClient,
|
||||
[],
|
||||
);
|
||||
});
|
||||
@@ -298,7 +317,7 @@ describe("POST /api/mood-entries/bulk — structured tagKeys (v1.12.0)", () => {
|
||||
"entry-1",
|
||||
"user-1",
|
||||
["movies"],
|
||||
prisma,
|
||||
txClient,
|
||||
[{ key: "factor_work", rating: 4 }],
|
||||
);
|
||||
});
|
||||
@@ -530,3 +549,69 @@ describe("POST /api/mood-entries/bulk — tombstone suppression", () => {
|
||||
expect(prisma.moodEntry.upsert).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe("POST /api/mood-entries/bulk — upsert + tag links are atomic", () => {
|
||||
// `createTagLinks` validates `ratedFactors` AFTER the mood row is written.
|
||||
// Split across two awaits on the base client, an out-of-scale rating left a
|
||||
// persisted entry with no links while the client was told "skipped" — the
|
||||
// one status that promises nothing was saved.
|
||||
it("runs the upsert and the tag-link write inside one transaction", async () => {
|
||||
vi.mocked(prisma.moodEntry.findMany).mockResolvedValue([] as never);
|
||||
vi.mocked(prisma.moodEntry.upsert).mockResolvedValue({
|
||||
id: "mood-1",
|
||||
} as never);
|
||||
|
||||
await POST(
|
||||
postReq({
|
||||
entries: [
|
||||
{
|
||||
mood: "GUT",
|
||||
moodLoggedAt: "2026-05-16T08:00:00.000Z",
|
||||
tagKeys: ["sleep"],
|
||||
},
|
||||
],
|
||||
}),
|
||||
);
|
||||
|
||||
expect(prisma.$transaction).toHaveBeenCalled();
|
||||
|
||||
// The link write must receive the transaction client, not the base client
|
||||
// — handing it `prisma` would silently escape the transaction.
|
||||
const txArg = vi.mocked(createTagLinks).mock.calls[0]?.[3];
|
||||
const handedToTransaction = vi.mocked(prisma.$transaction).mock.calls
|
||||
.length;
|
||||
expect(handedToTransaction).toBeGreaterThan(0);
|
||||
expect(txArg).toBeDefined();
|
||||
});
|
||||
|
||||
it("does not report a skipped entry as inserted when the factor write throws", async () => {
|
||||
const { RatedFactorOutOfRangeError } = await import("@/lib/mood/tag-links");
|
||||
vi.mocked(prisma.moodEntry.findMany).mockResolvedValue([] as never);
|
||||
vi.mocked(prisma.moodEntry.upsert).mockResolvedValue({
|
||||
id: "mood-1",
|
||||
} as never);
|
||||
vi.mocked(createTagLinks).mockRejectedValueOnce(
|
||||
new RatedFactorOutOfRangeError("factor_conflict", 5, 1, 2),
|
||||
);
|
||||
|
||||
const res = await POST(
|
||||
postReq({
|
||||
entries: [
|
||||
{
|
||||
mood: "GUT",
|
||||
moodLoggedAt: "2026-05-16T08:00:00.000Z",
|
||||
ratedFactors: [{ key: "factor_conflict", rating: 5 }],
|
||||
},
|
||||
],
|
||||
}),
|
||||
);
|
||||
const json = (await res.json()) as {
|
||||
data: { inserted: number; entries: Array<{ status: string }> };
|
||||
};
|
||||
expect(json.data.entries[0]!.status).toBe("skipped");
|
||||
expect(json.data.inserted).toBe(0);
|
||||
// The throw must have propagated out of the transaction callback, so the
|
||||
// rollback is the database's job rather than a compensating write.
|
||||
expect(prisma.$transaction).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -300,63 +300,73 @@ async function postBulk(request: NextRequest): Promise<Response> {
|
||||
continue;
|
||||
}
|
||||
|
||||
const result = await prisma.moodEntry.upsert({
|
||||
where: probeWhere,
|
||||
create: {
|
||||
userId: user.id,
|
||||
date,
|
||||
tz,
|
||||
mood: entry.mood,
|
||||
score,
|
||||
tags: entry.tags ? JSON.stringify(entry.tags) : null,
|
||||
note: null,
|
||||
noteEncrypted: encryptNote(entry.note ?? null),
|
||||
source: resolvedSource,
|
||||
externalId: entry.externalId ?? null,
|
||||
moodLoggedAt: entry.moodLoggedAt,
|
||||
},
|
||||
update: {
|
||||
// Last-writer-wins on the mood + tags + note triple. The
|
||||
// iOS client only re-posts an existing entry when it has
|
||||
// new data; the server trusts that decision. When the dedup
|
||||
// key is `externalId`, also refresh `date` / `moodLoggedAt`
|
||||
// so a re-zoned re-import lands the corrected wall-clock on
|
||||
// the same row.
|
||||
mood: entry.mood,
|
||||
score,
|
||||
tags: entry.tags ? JSON.stringify(entry.tags) : null,
|
||||
note: null,
|
||||
noteEncrypted: encryptNote(entry.note ?? null),
|
||||
...(entry.externalId
|
||||
? { date, moodLoggedAt: entry.moodLoggedAt }
|
||||
: {}),
|
||||
},
|
||||
});
|
||||
// The upsert and the tag/factor links run in ONE transaction, matching
|
||||
// the single-entry POST path. Split apart, an out-of-scale
|
||||
// `ratedFactors` value (validated inside `createTagLinks`, i.e. AFTER
|
||||
// the row committed) left a persisted mood entry with no links while the
|
||||
// per-entry catch reported the entry as "skipped" — the client believes
|
||||
// nothing saved and a half-written row is on disk.
|
||||
const result = await prisma.$transaction(async (tx) => {
|
||||
const upserted = await tx.moodEntry.upsert({
|
||||
where: probeWhere,
|
||||
create: {
|
||||
userId: user.id,
|
||||
date,
|
||||
tz,
|
||||
mood: entry.mood,
|
||||
score,
|
||||
tags: entry.tags ? JSON.stringify(entry.tags) : null,
|
||||
note: null,
|
||||
noteEncrypted: encryptNote(entry.note ?? null),
|
||||
source: resolvedSource,
|
||||
externalId: entry.externalId ?? null,
|
||||
moodLoggedAt: entry.moodLoggedAt,
|
||||
},
|
||||
update: {
|
||||
// Last-writer-wins on the mood + tags + note triple. The
|
||||
// iOS client only re-posts an existing entry when it has
|
||||
// new data; the server trusts that decision. When the dedup
|
||||
// key is `externalId`, also refresh `date` / `moodLoggedAt`
|
||||
// so a re-zoned re-import lands the corrected wall-clock on
|
||||
// the same row.
|
||||
mood: entry.mood,
|
||||
score,
|
||||
tags: entry.tags ? JSON.stringify(entry.tags) : null,
|
||||
note: null,
|
||||
noteEncrypted: encryptNote(entry.note ?? null),
|
||||
...(entry.externalId
|
||||
? { date, moodLoggedAt: entry.moodLoggedAt }
|
||||
: {}),
|
||||
},
|
||||
});
|
||||
|
||||
// v1.12.0 — persist structured-tag links, mirroring the
|
||||
// single-entry `createTagLinks` path. Additive + idempotent:
|
||||
// `createTagLinks` resolves keys against the catalog (dropping
|
||||
// unknown keys) and `skipDuplicates` on the join insert keeps a
|
||||
// re-posted entry from minting duplicate links. Runs for both
|
||||
// fresh and re-posted (upserted) rows so a backfill that adds tag
|
||||
// keys on a second pass still lands them.
|
||||
// v1.12.0 — rated factors ride the same path; an out-of-scale
|
||||
// rating throws `RatedFactorOutOfRangeError`, which the per-entry
|
||||
// catch below turns into a `skipped` result (the rest of the batch
|
||||
// still lands). The mood row itself already upserted, so a skipped
|
||||
// factor leaves a valid entry with no rated links.
|
||||
if (
|
||||
(entry.tagKeys && entry.tagKeys.length > 0) ||
|
||||
(entry.ratedFactors && entry.ratedFactors.length > 0)
|
||||
) {
|
||||
await createTagLinks(
|
||||
result.id,
|
||||
user.id,
|
||||
entry.tagKeys ?? [],
|
||||
prisma,
|
||||
entry.ratedFactors ?? [],
|
||||
);
|
||||
}
|
||||
// v1.12.0 — persist structured-tag links, mirroring the
|
||||
// single-entry `createTagLinks` path. Additive + idempotent:
|
||||
// `createTagLinks` resolves keys against the catalog (dropping
|
||||
// unknown keys) and `skipDuplicates` on the join insert keeps a
|
||||
// re-posted entry from minting duplicate links. Runs for both
|
||||
// fresh and re-posted (upserted) rows so a backfill that adds tag
|
||||
// keys on a second pass still lands them.
|
||||
// v1.12.0 — rated factors ride the same path; an out-of-scale
|
||||
// rating throws `RatedFactorOutOfRangeError`. Inside the
|
||||
// transaction that throw now rolls the mood row back with it, so a
|
||||
// "skipped" result means nothing was written — which is what the
|
||||
// client was already being told.
|
||||
if (
|
||||
(entry.tagKeys && entry.tagKeys.length > 0) ||
|
||||
(entry.ratedFactors && entry.ratedFactors.length > 0)
|
||||
) {
|
||||
await createTagLinks(
|
||||
upserted.id,
|
||||
user.id,
|
||||
entry.tagKeys ?? [],
|
||||
tx,
|
||||
entry.ratedFactors ?? [],
|
||||
);
|
||||
}
|
||||
|
||||
return upserted;
|
||||
});
|
||||
|
||||
if (existing) {
|
||||
duplicates += 1;
|
||||
|
||||
@@ -39,7 +39,9 @@ vi.mock("@/lib/monitoring-settings", () => ({
|
||||
})),
|
||||
}));
|
||||
|
||||
const sendGlitchtipEvent = vi.fn(async (_payload: unknown) => {});
|
||||
const sendGlitchtipEvent = vi.fn<(payload: unknown) => Promise<void>>(
|
||||
async () => {},
|
||||
);
|
||||
vi.mock("@/lib/monitoring/glitchtip", () => ({
|
||||
sendGlitchtipEvent: (payload: unknown) => sendGlitchtipEvent(payload),
|
||||
}));
|
||||
@@ -62,10 +64,10 @@ beforeEach(() => {
|
||||
sendGlitchtipEvent.mockClear();
|
||||
});
|
||||
|
||||
function throwingHandler() {
|
||||
return apiHandler(async (_request: NextRequest) => {
|
||||
function throwingHandler(): (request: NextRequest) => Promise<Response> {
|
||||
return apiHandler(async () => {
|
||||
throw new Error("boom");
|
||||
});
|
||||
}) as unknown as (request: NextRequest) => Promise<Response>;
|
||||
}
|
||||
|
||||
async function forwardedPayload(url: string) {
|
||||
|
||||
Reference in new issue
Block a user