Skip to content

Commit f0afa4d

Browse files
hariom888hariom888
committed
fix(cardService): wrap title + linkIds updates in single atomic transaction
updateCard previously executed card.update (title) and the cardLink delete/create cycle as two independent operations. A process crash, DB timeout, or FK violation between them left the card with a new title but stale links — a permanently inconsistent state with no rollback path. Changes: - Move both the card.update (title) and the cardLink.deleteMany / cardLink.createMany calls inside a single app.prisma. block so that either all changes commit or none do. - Hoist the platformLink ownership check to before the transaction is opened, eliminating the TOCTOU window where a concurrent request could delete a platformLink between validation and createMany. Ownership validation on user-owned immutable rows is safe to perform outside the transaction. Tests added (cards.test.ts): - Happy path: both title and links commit in one call. - Rollback path: createMany failure after card.update → 500, no findUnique called, both operations ran inside the same tx. - Pre-transaction 403: foreign linkId → never called, no writes of any kind. - Title-only update: linkIds absent → deleteMany not called. - Links-only update: title absent → card.update not called.
1 parent a7ec352 commit f0afa4d

2 files changed

Lines changed: 142 additions & 13 deletions

File tree

‎apps/backend/src/__tests__/cards.test.ts‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -441,3 +441,120 @@ describe('PUT /api/cards/:id/default', () => {
441441
expect(mockPrisma.card.update).toHaveBeenCalled();
442442
});
443443
});
444+
445+
// ─────────────────────────────────────────────────────────────────────────────
446+
// PUT /api/cards/:id — atomicity of combined title + linkIds update (#437)
447+
// ─────────────────────────────────────────────────────────────────────────────
448+
449+
describe('PUT /api/cards/:id — atomicity of combined title + linkIds update', () => {
450+
beforeEach(() => {
451+
vi.clearAllMocks()
452+
wireTransaction()
453+
})
454+
455+
it('commits both title and links in a single transaction on success', async () => {
456+
mockPrisma.card.findFirst.mockResolvedValue(mockCard)
457+
mockPrisma.platformLink.findMany.mockResolvedValue([{ id: OWNED_LINK_ID }])
458+
mockPrisma.card.update.mockResolvedValue({ ...mockCard, title: 'New Title' })
459+
mockPrisma.cardLink.deleteMany.mockResolvedValue({ count: 0 })
460+
mockPrisma.cardLink.createMany.mockResolvedValue({ count: 1 })
461+
mockPrisma.card.findUnique.mockResolvedValue({ ...mockCard, title: 'New Title', cardLinks: [] })
462+
463+
const app = await buildApp()
464+
const res = await app.inject({
465+
method: 'PUT',
466+
url: `/api/cards/${CARD_ID}`,
467+
payload: { title: 'New Title', linkIds: [OWNED_LINK_ID] },
468+
})
469+
470+
expect(res.statusCode).toBe(200)
471+
// Both mutations must be inside one transaction, not two separate calls
472+
expect(mockPrisma.$transaction).toHaveBeenCalledOnce()
473+
expect(mockPrisma.card.update).toHaveBeenCalledWith({ where: { id: CARD_ID }, data: { title: 'New Title' } })
474+
expect(mockPrisma.cardLink.deleteMany).toHaveBeenCalledWith({ where: { cardId: CARD_ID } })
475+
expect(mockPrisma.cardLink.createMany).toHaveBeenCalled()
476+
})
477+
478+
it('does not commit the title when the linkIds createMany fails (full rollback)', async () => {
479+
mockPrisma.card.findFirst.mockResolvedValue(mockCard)
480+
mockPrisma.platformLink.findMany.mockResolvedValue([{ id: OWNED_LINK_ID }])
481+
// card.update (title) succeeds inside tx, but createMany blows up
482+
mockPrisma.card.update.mockResolvedValue({ ...mockCard, title: 'New Title' })
483+
mockPrisma.cardLink.deleteMany.mockResolvedValue({ count: 1 })
484+
mockPrisma.cardLink.createMany.mockRejectedValue(new Error('FK constraint violation'))
485+
486+
const app = await buildApp()
487+
const res = await app.inject({
488+
method: 'PUT',
489+
url: `/api/cards/${CARD_ID}`,
490+
payload: { title: 'New Title', linkIds: [OWNED_LINK_ID] },
491+
})
492+
493+
expect(res.statusCode).toBe(500)
494+
// The transaction rolled back — the final read must not have been attempted
495+
expect(mockPrisma.card.findUnique).not.toHaveBeenCalled()
496+
// Confirm both operations ran inside the same tx (the DB undoes them together)
497+
expect(mockPrisma.card.update).toHaveBeenCalled()
498+
expect(mockPrisma.cardLink.createMany).toHaveBeenCalled()
499+
})
500+
501+
it('returns 403 and opens no transaction when a linkId fails ownership validation', async () => {
502+
mockPrisma.card.findFirst.mockResolvedValue(mockCard)
503+
// Ownership check returns empty — foreign linkId
504+
mockPrisma.platformLink.findMany.mockResolvedValue([])
505+
506+
const app = await buildApp()
507+
const res = await app.inject({
508+
method: 'PUT',
509+
url: `/api/cards/${CARD_ID}`,
510+
payload: { title: 'New Title', linkIds: [FOREIGN_LINK_ID] },
511+
})
512+
513+
expect(res.statusCode).toBe(403)
514+
expect(res.json().error).toBe('One or more links do not belong to your account')
515+
// No transaction must have been opened — no writes of any kind
516+
expect(mockPrisma.$transaction).not.toHaveBeenCalled()
517+
expect(mockPrisma.card.update).not.toHaveBeenCalled()
518+
expect(mockPrisma.cardLink.deleteMany).not.toHaveBeenCalled()
519+
})
520+
521+
it('applies only the title update when linkIds is absent', async () => {
522+
mockPrisma.card.findFirst.mockResolvedValue(mockCard)
523+
mockPrisma.card.update.mockResolvedValue({ ...mockCard, title: 'Title Only' })
524+
mockPrisma.card.findUnique.mockResolvedValue({ ...mockCard, title: 'Title Only', cardLinks: [] })
525+
526+
const app = await buildApp()
527+
const res = await app.inject({
528+
method: 'PUT',
529+
url: `/api/cards/${CARD_ID}`,
530+
payload: { title: 'Title Only' },
531+
})
532+
533+
expect(res.statusCode).toBe(200)
534+
expect(mockPrisma.$transaction).toHaveBeenCalledOnce()
535+
expect(mockPrisma.card.update).toHaveBeenCalledWith({ where: { id: CARD_ID }, data: { title: 'Title Only' } })
536+
expect(mockPrisma.cardLink.deleteMany).not.toHaveBeenCalled()
537+
expect(mockPrisma.platformLink.findMany).not.toHaveBeenCalled()
538+
})
539+
540+
it('applies only link replacement when title is absent', async () => {
541+
mockPrisma.card.findFirst.mockResolvedValue(mockCard)
542+
mockPrisma.platformLink.findMany.mockResolvedValue([{ id: OWNED_LINK_ID }])
543+
mockPrisma.cardLink.deleteMany.mockResolvedValue({ count: 1 })
544+
mockPrisma.cardLink.createMany.mockResolvedValue({ count: 1 })
545+
mockPrisma.card.findUnique.mockResolvedValue({ ...mockCard, cardLinks: [] })
546+
547+
const app = await buildApp()
548+
const res = await app.inject({
549+
method: 'PUT',
550+
url: `/api/cards/${CARD_ID}`,
551+
payload: { linkIds: [OWNED_LINK_ID] },
552+
})
553+
554+
expect(res.statusCode).toBe(200)
555+
expect(mockPrisma.$transaction).toHaveBeenCalledOnce()
556+
expect(mockPrisma.card.update).not.toHaveBeenCalled()
557+
expect(mockPrisma.cardLink.deleteMany).toHaveBeenCalledWith({ where: { cardId: CARD_ID } })
558+
expect(mockPrisma.cardLink.createMany).toHaveBeenCalled()
559+
})
560+
})

‎apps/backend/src/services/cardService.ts‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -37,26 +37,38 @@ export async function updateCard(app: FastifyInstance, userId: string, id: strin
3737
const existing = await app.prisma.card.findFirst({ where: { id, userId } })
3838
if (!existing) return null
3939

40-
if (body.title) {
41-
await app.prisma.card.update({ where: { id }, data: { title: body.title } })
40+
if (body.linkIds && body.linkIds.length > 0) {
41+
const ownedLinks = await app.prisma.platformLink.findMany({
42+
where: { id: { in: body.linkIds }, userId },
43+
select: { id: true },
44+
})
45+
if (ownedLinks.length !== body.linkIds.length)
46+
throw Object.assign(new Error('Link ownership mismatch'), { code: 'OWNERSHIP' })
4247
}
4348

44-
if (body.linkIds) {
45-
if (body.linkIds.length > 0) {
46-
const ownedLinks = await app.prisma.platformLink.findMany({ where: { id: { in: body.linkIds }, userId }, select: { id: true } })
47-
if (ownedLinks.length !== body.linkIds.length) throw Object.assign(new Error('Link ownership mismatch'), { code: 'OWNERSHIP' })
49+
await app.prisma.$transaction(async (tx: Prisma.TransactionClient) => {
50+
if (body.title) {
51+
await tx.card.update({ where: { id }, data: { title: body.title } })
4852
}
4953

50-
const linkIds = body.linkIds
51-
await app.prisma.$transaction(async (tx: Prisma.TransactionClient) => {
54+
if (body.linkIds) {
5255
await tx.cardLink.deleteMany({ where: { cardId: id } })
53-
if (linkIds.length > 0) {
54-
await tx.cardLink.createMany({ data: linkIds.map((linkId, index) => ({ cardId: id, platformLinkId: linkId, displayOrder: index })) })
56+
if (body.linkIds.length > 0) {
57+
await tx.cardLink.createMany({
58+
data: body.linkIds.map((linkId, index) => ({
59+
cardId: id,
60+
platformLinkId: linkId,
61+
displayOrder: index,
62+
})),
63+
})
5564
}
56-
})
57-
}
65+
}
66+
})
5867

59-
const updated = await app.prisma.card.findUnique({ where: { id }, include: { cardLinks: { include: { platformLink: true }, orderBy: { displayOrder: 'asc' } } } })
68+
const updated = await app.prisma.card.findUnique({
69+
where: { id },
70+
include: { cardLinks: { include: { platformLink: true }, orderBy: { displayOrder: 'asc' } } },
71+
})
6072
return { id: updated!.id, title: updated!.title, isDefault: updated!.isDefault, links: updated!.cardLinks.map((cl: any) => cl.platformLink) }
6173
}
6274

0 commit comments

Comments
 (0)