diff --git a/scripts/e2e/suite-registry.mjs b/scripts/e2e/suite-registry.mjs index 81ade68a2..3a5b5208a 100644 --- a/scripts/e2e/suite-registry.mjs +++ b/scripts/e2e/suite-registry.mjs @@ -285,6 +285,27 @@ export const E2E_SUITES = Object.freeze({ ...SHARED_DB_AND_HELPER_PATHS, ], }, + "reading-attempts": { + id: "reading-attempts", + name: "Reading Attempts", + timeout: 120, + lane: "full", + description: "Reading attempt lifecycle, external read identity, and Hardcover read ownership suite", + vitestTarget: "test/reading-attempts.e2e-spec.ts", + junitOutput: `${TEST_RESULTS_DIR}/reading-attempts-e2e-junit.xml`, + prepareDedicatedDatabase: true, + useDedicatedDatabase: true, + changedPaths: [ + "server/src/modules/user-book-status/**", + "server/src/modules/hardcover/**", + "server/src/modules/dashboard/**", + "server/test/reading-attempts.e2e-spec.ts", + "server/test/e2e/**", + ".github/workflows/e2e-runner.yml", + "packages/types/src/book.ts", + ...SHARED_DB_AND_HELPER_PATHS, + ], + }, "book-api-contract": { id: "book-api-contract", name: "Book API Contract", diff --git a/server/src/modules/hardcover/hardcover-read-selection.test.ts b/server/src/modules/hardcover/hardcover-read-selection.test.ts new file mode 100644 index 000000000..72c267f49 --- /dev/null +++ b/server/src/modules/hardcover/hardcover-read-selection.test.ts @@ -0,0 +1,190 @@ +import { describe, expect, it } from 'vitest'; + +import { + attemptOwnedReadId, + isReadCompatible, + readTargetForAttempt, + readTargetForBook, + selectAdoptableReadId, + selectPrimaryAttempt, +} from './hardcover-read-selection'; + +function read(id: number, started_at: string | null, finished_at: string | null) { + return { id, started_at, finished_at }; +} + +function attempt(overrides: Partial[0]> & { id?: number } = {}) { + return { + id: 1, + startedOn: null, + endedOn: null, + outcome: null, + externalProvider: null, + externalId: null, + ...overrides, + } as { + id: number; + startedOn: string | null; + endedOn: string | null; + outcome: 'completed' | 'skimmed' | 'abandoned' | null; + externalProvider: string | null; + externalId: string | null; + }; +} + +describe('selectPrimaryAttempt', () => { + it('returns null when there are no attempts', () => { + expect(selectPrimaryAttempt([])).toBeNull(); + }); + + it('prefers the open attempt over a higher-id closed one', () => { + const open = attempt({ id: 2, outcome: null }); + const closed = attempt({ id: 9, outcome: 'completed' }); + + expect(selectPrimaryAttempt([closed, open])?.id).toBe(2); + }); + + it('falls back to the highest id when no attempt is open', () => { + const first = attempt({ id: 3, outcome: 'completed' }); + const second = attempt({ id: 7, outcome: 'completed' }); + + expect(selectPrimaryAttempt([second, first])?.id).toBe(7); + }); + + it('lets a skimmed or abandoned attempt be primary, since both map to a Hardcover status', () => { + expect(selectPrimaryAttempt([attempt({ id: 4, outcome: 'skimmed' })])?.id).toBe(4); + expect(selectPrimaryAttempt([attempt({ id: 5, outcome: 'abandoned' })])?.id).toBe(5); + }); + + it('does not depend on input ordering', () => { + const attempts = [attempt({ id: 1, outcome: 'completed' }), attempt({ id: 8, outcome: null }), attempt({ id: 4, outcome: 'abandoned' })]; + + expect(selectPrimaryAttempt(attempts)?.id).toBe(8); + expect(selectPrimaryAttempt([...attempts].reverse())?.id).toBe(8); + }); +}); + +describe('attemptOwnedReadId', () => { + it('reads the id an attempt already owns', () => { + expect(attemptOwnedReadId({ externalProvider: 'hardcover', externalId: '555' })).toBe(555); + }); + + it('ignores stamps from other providers', () => { + expect(attemptOwnedReadId({ externalProvider: 'kobo', externalId: '555' })).toBeNull(); + }); + + it('ignores missing or unusable ids', () => { + expect(attemptOwnedReadId({ externalProvider: 'hardcover', externalId: null })).toBeNull(); + expect(attemptOwnedReadId({ externalProvider: 'hardcover', externalId: 'abc' })).toBeNull(); + expect(attemptOwnedReadId({ externalProvider: 'hardcover', externalId: '0' })).toBeNull(); + expect(attemptOwnedReadId({ externalProvider: 'hardcover', externalId: '-3' })).toBeNull(); + }); +}); + +describe('readTargetForAttempt', () => { + it('treats only a completed attempt as finished', () => { + expect(readTargetForAttempt(attempt({ startedOn: '2024-01-01', endedOn: '2024-01-05', outcome: 'completed' }))).toEqual({ + startedOn: '2024-01-01', + endedOn: '2024-01-05', + finished: true, + }); + }); + + it('keeps a completed attempt with an unknown finish date open remotely', () => { + expect(readTargetForAttempt(attempt({ startedOn: '2024-01-01', endedOn: null, outcome: 'completed' }))).toEqual({ + startedOn: '2024-01-01', + endedOn: null, + finished: false, + }); + }); + + it('drops the end date for skimmed and abandoned attempts, matching what the sync sends', () => { + expect(readTargetForAttempt(attempt({ startedOn: '2024-01-01', endedOn: '2024-01-05', outcome: 'skimmed' }))).toEqual({ + startedOn: '2024-01-01', + endedOn: null, + finished: false, + }); + expect(readTargetForAttempt(attempt({ startedOn: '2024-01-01', endedOn: '2024-01-05', outcome: 'abandoned' }))).toEqual({ + startedOn: '2024-01-01', + endedOn: null, + finished: false, + }); + }); + + it('treats an open attempt as unfinished', () => { + expect(readTargetForAttempt(attempt({ startedOn: '2024-02-01', outcome: null }))).toEqual({ + startedOn: '2024-02-01', + endedOn: null, + finished: false, + }); + }); +}); + +describe('readTargetForBook', () => { + it('is finished exactly when the book has an end date', () => { + expect(readTargetForBook('2024-01-01', '2024-01-05').finished).toBe(true); + expect(readTargetForBook('2024-01-01', null).finished).toBe(false); + }); +}); + +describe('isReadCompatible', () => { + it('requires open and finished state to agree', () => { + const openTarget = readTargetForBook('2024-01-01', null); + + expect(isReadCompatible(read(1, '2024-01-01', null), openTarget)).toBe(true); + expect(isReadCompatible(read(1, '2024-01-01', '2024-01-05'), openTarget)).toBe(false); + }); + + it('rejects a finished read whose end date differs', () => { + const target = readTargetForBook('2024-01-01', '2024-01-05'); + + expect(isReadCompatible(read(1, '2024-01-01', '2024-01-05'), target)).toBe(true); + expect(isReadCompatible(read(1, '2024-01-01', '2024-02-09'), target)).toBe(false); + }); + + it('rejects a read whose start date differs', () => { + const target = readTargetForBook('2024-03-01', null); + + expect(isReadCompatible(read(1, '2024-01-01', null), target)).toBe(false); + }); + + it('tolerates dates missing on either side rather than treating absence as evidence', () => { + expect(isReadCompatible(read(1, null, null), readTargetForBook('2024-01-01', null))).toBe(true); + expect(isReadCompatible(read(1, '2024-01-01', null), readTargetForBook(null, null))).toBe(true); + }); +}); + +describe('selectAdoptableReadId', () => { + const openTarget = readTargetForBook('2024-01-01', null); + + it('returns null when nothing is compatible', () => { + expect(selectAdoptableReadId([read(1, '2024-01-01', '2024-01-05')], openTarget, new Set())).toBeNull(); + }); + + it('skips reads another attempt already owns', () => { + expect(selectAdoptableReadId([read(555, '2024-01-01', null)], openTarget, new Set([555]))).toBeNull(); + }); + + it('adopts a compatible unclaimed read', () => { + expect(selectAdoptableReadId([read(777, '2024-01-01', null)], openTarget, new Set())).toBe(777); + }); + + it('prefers the candidate with more explicitly matching dates', () => { + const reads = [read(900, null, null), read(777, '2024-01-01', null)]; + + expect(selectAdoptableReadId(reads, openTarget, new Set())).toBe(777); + }); + + it('falls back to the highest id when evidence is equal', () => { + const reads = [read(777, '2024-01-01', null), read(778, '2024-01-01', null)]; + + expect(selectAdoptableReadId(reads, openTarget, new Set())).toBe(778); + }); + + it('matches a completed attempt to its finished read', () => { + const target = readTargetForAttempt(attempt({ startedOn: '2024-01-01', endedOn: '2024-01-05', outcome: 'completed' })); + const reads = [read(700, '2024-01-01', '2024-01-05'), read(701, '2024-01-01', null)]; + + expect(selectAdoptableReadId(reads, target, new Set())).toBe(700); + }); +}); diff --git a/server/src/modules/hardcover/hardcover-read-selection.ts b/server/src/modules/hardcover/hardcover-read-selection.ts new file mode 100644 index 000000000..536c0a709 --- /dev/null +++ b/server/src/modules/hardcover/hardcover-read-selection.ts @@ -0,0 +1,106 @@ +import type { ReadingAttemptOutcome } from '@bookorbit/types'; + +export const HARDCOVER_EXTERNAL_PROVIDER = 'hardcover'; + +export interface HardcoverReadCandidate { + id: number; + started_at: string | null; + finished_at: string | null; +} + +export interface ReadingAttemptForSync { + id: number; + startedOn: string | null; + endedOn: string | null; + outcome: ReadingAttemptOutcome | null; + externalProvider: string | null; + externalId: string | null; +} + +/** + * The shape a Hardcover read must have to represent a given local attempt. `finished` mirrors what + * the sync sends as `finished_at`, which is only ever populated for a completed attempt. + */ +export interface HardcoverReadTarget { + startedOn: string | null; + endedOn: string | null; + finished: boolean; +} + +/** + * Mirrors the `active ?? latest` rule that rebuildProjection uses to decide which attempt drives + * user_book_status. The book-level dates the sync pushes to Hardcover are that attempt's dates, so + * both sides must agree on the choice or the sync links a read to the wrong attempt. + */ +export function selectPrimaryAttempt(attempts: readonly T[]): T | null { + let active: T | null = null; + let latest: T | null = null; + for (const attempt of attempts) { + if (attempt.outcome === null && (active === null || attempt.id > active.id)) active = attempt; + if (latest === null || attempt.id > latest.id) latest = attempt; + } + return active ?? latest; +} + +export function attemptOwnedReadId(attempt: Pick): number | null { + if (attempt.externalProvider !== HARDCOVER_EXTERNAL_PROVIDER || !attempt.externalId) return null; + const parsed = Number(attempt.externalId); + return Number.isInteger(parsed) && parsed > 0 ? parsed : null; +} + +export function readTargetForAttempt(attempt: Pick): HardcoverReadTarget { + const finished = attempt.outcome === 'completed' && attempt.endedOn !== null; + return { startedOn: attempt.startedOn, endedOn: finished ? attempt.endedOn : null, finished }; +} + +export function readTargetForBook(startedOn: string | null, endedOn: string | null): HardcoverReadTarget { + return { startedOn, endedOn, finished: endedOn !== null }; +} + +/** + * Adoption is only safe when the remote read plausibly *is* this target: open/finished state must + * agree, and any date both sides state must be equal. Dates absent on either side are not evidence + * against a match, since Hardcover reads routinely omit them. + */ +export function isReadCompatible(read: HardcoverReadCandidate, target: HardcoverReadTarget): boolean { + if (Boolean(read.finished_at) !== target.finished) return false; + if (read.started_at && target.startedOn && read.started_at !== target.startedOn) return false; + if (read.finished_at && target.endedOn && read.finished_at !== target.endedOn) return false; + return true; +} + +function explicitMatchScore(read: HardcoverReadCandidate, target: HardcoverReadTarget): number { + let score = 0; + if (read.started_at && target.startedOn && read.started_at === target.startedOn) score++; + if (read.finished_at && target.endedOn && read.finished_at === target.endedOn) score++; + return score; +} + +/** + * Picks a remote read this target may take ownership of. `unavailable` carries every read id already + * owned by another attempt - including soft-deleted ones, whose external ids stay reserved as + * tombstones - plus everything claimed earlier in the same sync run. + */ +export function selectAdoptableReadId( + reads: readonly HardcoverReadCandidate[], + target: HardcoverReadTarget, + unavailable: ReadonlySet, +): number | null { + let best: HardcoverReadCandidate | null = null; + let bestScore = -1; + for (const read of reads) { + if (unavailable.has(read.id)) continue; + if (!isReadCompatible(read, target)) continue; + const score = explicitMatchScore(read, target); + if (score > bestScore) { + best = read; + bestScore = score; + continue; + } + if (score < bestScore || best === null) continue; + // Equal evidence: keep the highest id, matching the descending order Hardcover returns. The + // book-level cached id is deliberately not consulted because it is not attempt identity. + if (read.id > best.id) best = read; + } + return best?.id ?? null; +} diff --git a/server/src/modules/hardcover/hardcover-sync.service.test.ts b/server/src/modules/hardcover/hardcover-sync.service.test.ts index b2f09907f..86466b58a 100644 --- a/server/src/modules/hardcover/hardcover-sync.service.test.ts +++ b/server/src/modules/hardcover/hardcover-sync.service.test.ts @@ -14,6 +14,7 @@ const mockRepo = { findBookSyncData: vi.fn(), findCurrentReadingBooks: vi.fn(), findReadingAttempts: vi.fn(), + findClaimedHardcoverReadIds: vi.fn(), linkReadingAttempt: vi.fn(), }; @@ -76,7 +77,8 @@ describe('HardcoverSyncService', () => { mockRepo.findBookSyncData.mockResolvedValue(null); mockRepo.findCurrentReadingBooks.mockResolvedValue([]); mockRepo.findReadingAttempts.mockResolvedValue([]); - mockRepo.linkReadingAttempt.mockResolvedValue(undefined); + mockRepo.findClaimedHardcoverReadIds.mockResolvedValue([]); + mockRepo.linkReadingAttempt.mockResolvedValue('linked'); mockRepo.upsertBookState.mockResolvedValue({}); mockRepo.setBookSyncOverride.mockResolvedValue({}); mockRepo.updateLastSyncedAt.mockResolvedValue(undefined); @@ -347,6 +349,63 @@ describe('HardcoverSyncService', () => { logSpy.mockRestore(); }); + it('does not fan primary progress out to an open read reserved by another attempt', async () => { + mockSettingsService.getTokenForUser.mockResolvedValue('tok'); + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue({ hardcoverReadId: 900 }); + mockRepo.findReadingAttempts.mockResolvedValue([ + { + id: 2, + startedOn: '2024-01-01', + endedOn: null, + outcome: null, + externalProvider: 'hardcover', + externalId: '900', + }, + ]); + mockRepo.findClaimedHardcoverReadIds.mockResolvedValue([899, 900]); + mockMatchService.matchBook.mockResolvedValue({ hardcoverBookId: 10, hardcoverEditionId: 20, editionPages: 300, matchMethod: 'cached' }); + mockClient.query + .mockResolvedValueOnce({ insert_user_book: { user_book: { id: 55 }, error: null } }) + .mockResolvedValueOnce({ update_user_book: { user_book: { id: 55 }, error: null } }) + .mockResolvedValueOnce({ + user_book_reads: [ + { id: 900, started_at: '2024-01-01', finished_at: null, progress_pages: null }, + { id: 899, started_at: '2023-01-01', finished_at: null, progress_pages: null }, + ], + }) + .mockResolvedValueOnce({ update_user_book_read: { user_book_read: { id: 900 }, error: null } }); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('synced'); + + expect( + mockClient.query.mock.calls.some( + ([, , query, variables]) => + typeof query === 'string' && query.includes('mutation UpdateUserBookRead') && (variables as { id?: number } | undefined)?.id === 899, + ), + ).toBe(false); + }); + + it('fails without inserting a read when Hardcover read discovery fails', async () => { + const errorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined); + mockSettingsService.getTokenForUser.mockResolvedValue('tok'); + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue(undefined); + mockMatchService.matchBook.mockResolvedValue({ hardcoverBookId: 10, hardcoverEditionId: 20, editionPages: 300, matchMethod: 'cached' }); + mockClient.query + .mockResolvedValueOnce({ insert_user_book: { user_book: { id: 55 }, error: null } }) + .mockResolvedValueOnce({ update_user_book: { user_book: { id: 55 }, error: null } }) + .mockRejectedValueOnce(new Error('read discovery failed')); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('failed'); + + expect(mockClient.query.mock.calls.some(([, , query]) => typeof query === 'string' && query.includes('mutation InsertUserBookRead'))).toBe( + false, + ); + expect(mockRepo.upsertBookState).toHaveBeenCalledWith(expect.objectContaining({ syncError: 'read discovery failed' })); + errorSpy.mockRestore(); + }); + it('stores error when edition pages are unavailable', async () => { mockSettingsService.getTokenForUser.mockResolvedValue('tok'); mockRepo.findBookSyncData.mockResolvedValue(readingBook); @@ -807,4 +866,311 @@ describe('HardcoverSyncService', () => { await expect(makeService().setEdition(1, 1, 200)).resolves.toEqual({ success: true }); }); }); + describe('reading attempt to Hardcover read mapping', () => { + type AttemptRow = { + id: number; + startedOn: string | null; + endedOn: string | null; + outcome: 'completed' | 'skimmed' | 'abandoned' | null; + externalProvider: string | null; + externalId: string | null; + }; + + function attempt(id: number, overrides: Partial = {}): AttemptRow { + return { id, startedOn: null, endedOn: null, outcome: null, externalProvider: null, externalId: null, ...overrides }; + } + + function linked(id: number, readId: number, overrides: Partial = {}): AttemptRow { + return attempt(id, { externalProvider: 'hardcover', externalId: String(readId), ...overrides }); + } + + const rereadingBook = { ...readingBook, status: 'rereading', startedAt: new Date('2024-02-01'), finishedAt: null }; + const match = { hardcoverBookId: 10, hardcoverEditionId: 20, editionPages: 300, matchMethod: 'cached' }; + + function arrange(reads: Array<{ id: number; started_at: string | null; finished_at: string | null; progress_pages: number | null }>) { + mockSettingsService.getTokenForUser.mockResolvedValue('tok'); + mockMatchService.matchBook.mockResolvedValue(match); + mockClient.query + .mockResolvedValueOnce({ insert_user_book: { user_book: { id: 55 }, error: null } }) + .mockResolvedValueOnce({ update_user_book: { user_book: { id: 55 }, error: null } }) + .mockResolvedValueOnce({ user_book_reads: reads }); + } + + function insertRead(id: number) { + mockClient.query.mockResolvedValueOnce({ insert_user_book_read: { user_book_read: { id }, error: null } }); + } + + function updateRead(id: number) { + mockClient.query.mockResolvedValueOnce({ update_user_book_read: { user_book_read: { id }, error: null } }); + } + + function linkCalls() { + return mockRepo.linkReadingAttempt.mock.calls as Array<[number, number, number]>; + } + + it("gives a reread its own read instead of stamping the finished attempt's id onto it", async () => { + mockRepo.findBookSyncData.mockResolvedValue(rereadingBook); + mockRepo.findBookState.mockResolvedValue({ hardcoverReadId: 555 }); + mockRepo.findReadingAttempts.mockResolvedValue([ + linked(1, 555, { startedOn: '2024-01-01', endedOn: '2024-01-05', outcome: 'completed' }), + attempt(2, { startedOn: '2024-02-01' }), + ]); + mockRepo.findClaimedHardcoverReadIds.mockResolvedValue([555]); + arrange([{ id: 555, started_at: '2024-01-01', finished_at: '2024-01-05', progress_pages: 300 }]); + insertRead(556); + updateRead(555); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('synced'); + + expect(linkCalls()).toEqual([[1, 2, 556]]); + expect(mockRepo.upsertBookState).toHaveBeenCalledWith(expect.objectContaining({ hardcoverReadId: 556, syncError: null })); + }); + + it('keeps the finished attempt on its original read rather than merging the two', async () => { + mockRepo.findBookSyncData.mockResolvedValue(rereadingBook); + mockRepo.findBookState.mockResolvedValue({ hardcoverReadId: 555 }); + mockRepo.findReadingAttempts.mockResolvedValue([ + linked(1, 555, { startedOn: '2024-01-01', endedOn: '2024-01-05', outcome: 'completed' }), + attempt(2, { startedOn: '2024-02-01' }), + ]); + mockRepo.findClaimedHardcoverReadIds.mockResolvedValue([555]); + arrange([{ id: 555, started_at: '2024-01-01', finished_at: '2024-01-05', progress_pages: 300 }]); + insertRead(556); + updateRead(555); + + await makeService().syncBook(1, 1); + + expect(mockClient.query).toHaveBeenNthCalledWith( + 5, + 1, + 'tok', + expect.stringContaining('mutation UpdateUserBookRead'), + expect.objectContaining({ id: 555 }), + ); + expect(linkCalls().some(([, attemptId]) => attemptId === 1)).toBe(false); + }); + + it("does not let a stale cached read id override an attempt's own mapping", async () => { + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue({ hardcoverReadId: 900 }); + mockRepo.findReadingAttempts.mockResolvedValue([linked(1, 777, { startedOn: '2024-01-01' })]); + mockRepo.findClaimedHardcoverReadIds.mockResolvedValue([777]); + arrange([ + { id: 900, started_at: '2024-01-01', finished_at: null, progress_pages: null }, + { id: 777, started_at: '2024-01-01', finished_at: null, progress_pages: null }, + ]); + updateRead(777); + updateRead(900); + + await makeService().syncBook(1, 1); + + expect(mockClient.query).toHaveBeenNthCalledWith( + 4, + 1, + 'tok', + expect.stringContaining('mutation UpdateUserBookRead'), + expect.objectContaining({ id: 777 }), + ); + expect(mockRepo.upsertBookState).toHaveBeenCalledWith(expect.objectContaining({ hardcoverReadId: 777 })); + }); + + it('leaves a read reserved by a soft-deleted attempt alone', async () => { + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue(undefined); + mockRepo.findReadingAttempts.mockResolvedValue([attempt(2, { startedOn: '2024-01-01' })]); + mockRepo.findClaimedHardcoverReadIds.mockResolvedValue([555]); + arrange([{ id: 555, started_at: '2024-01-01', finished_at: null, progress_pages: null }]); + insertRead(601); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('synced'); + + expect(linkCalls()).toEqual([[1, 2, 601]]); + }); + + it('adopts an unclaimed matching read instead of creating a duplicate', async () => { + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue(undefined); + mockRepo.findReadingAttempts.mockResolvedValue([attempt(2, { startedOn: '2024-01-01' })]); + arrange([{ id: 777, started_at: '2024-01-01', finished_at: null, progress_pages: null }]); + updateRead(777); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('synced'); + + expect(linkCalls()).toEqual([[1, 2, 777]]); + expect(mockClient.query).not.toHaveBeenCalledWith(1, 'tok', expect.stringContaining('mutation InsertUserBookRead'), expect.anything()); + }); + + it('claims a candidate locally before editing it on Hardcover', async () => { + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue(undefined); + mockRepo.findReadingAttempts.mockResolvedValue([attempt(2, { startedOn: '2024-01-01' })]); + arrange([{ id: 777, started_at: '2024-01-01', finished_at: null, progress_pages: null }]); + const order: string[] = []; + mockRepo.linkReadingAttempt.mockImplementation(() => { + order.push('link'); + return Promise.resolve('linked'); + }); + mockClient.query.mockImplementationOnce(() => { + order.push('remote-write'); + return Promise.resolve({ update_user_book_read: { user_book_read: { id: 777 }, error: null } }); + }); + + await makeService().syncBook(1, 1); + + expect(order).toEqual(['link', 'remote-write']); + }); + + it('gives competing unlinked attempts distinct reads', async () => { + mockRepo.findBookSyncData.mockResolvedValue({ ...rereadingBook, startedAt: new Date('2024-03-01') }); + mockRepo.findBookState.mockResolvedValue(undefined); + mockRepo.findReadingAttempts.mockResolvedValue([ + attempt(1, { startedOn: '2024-01-01', endedOn: '2024-01-05', outcome: 'completed' }), + attempt(2, { startedOn: '2024-01-01', endedOn: '2024-01-05', outcome: 'completed' }), + attempt(3, { startedOn: '2024-03-01' }), + ]); + arrange([{ id: 700, started_at: '2024-01-01', finished_at: '2024-01-05', progress_pages: 300 }]); + insertRead(800); + updateRead(700); + insertRead(801); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('synced'); + + const readIds = linkCalls().map(([, , readId]) => readId); + expect(readIds).toEqual([800, 700, 801]); + expect(new Set(readIds).size).toBe(readIds.length); + }); + + it('writes no link when every attempt already owns its read, so repeat syncs do not churn', async () => { + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue({ hardcoverReadId: 777 }); + mockRepo.findReadingAttempts.mockResolvedValue([ + linked(1, 700, { startedOn: '2023-05-01', endedOn: '2023-05-09', outcome: 'completed' }), + linked(2, 777, { startedOn: '2024-01-01' }), + ]); + mockRepo.findClaimedHardcoverReadIds.mockResolvedValue([700, 777]); + arrange([ + { id: 777, started_at: '2024-01-01', finished_at: null, progress_pages: null }, + { id: 700, started_at: '2023-05-01', finished_at: '2023-05-09', progress_pages: 300 }, + ]); + updateRead(777); + updateRead(700); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('synced'); + + expect(mockRepo.linkReadingAttempt).not.toHaveBeenCalled(); + }); + + it('reselects another read when a claim loses the race', async () => { + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue(undefined); + mockRepo.findReadingAttempts.mockResolvedValue([attempt(2, { startedOn: '2024-01-01' })]); + mockRepo.linkReadingAttempt.mockResolvedValueOnce('conflict').mockResolvedValueOnce('linked'); + arrange([ + { id: 778, started_at: '2024-01-01', finished_at: null, progress_pages: null }, + { id: 777, started_at: '2024-01-01', finished_at: null, progress_pages: null }, + ]); + updateRead(777); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('synced'); + + expect(linkCalls()).toEqual([ + [1, 2, 778], + [1, 2, 777], + ]); + expect(mockClient.query).toHaveBeenNthCalledWith( + 4, + 1, + 'tok', + expect.stringContaining('mutation UpdateUserBookRead'), + expect.objectContaining({ id: 777 }), + ); + }); + + it('creates a fresh read once reselection is exhausted rather than sharing one', async () => { + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue(undefined); + mockRepo.findReadingAttempts.mockResolvedValue([attempt(2, { startedOn: '2024-01-01' })]); + mockRepo.linkReadingAttempt.mockResolvedValueOnce('conflict').mockResolvedValueOnce('conflict').mockResolvedValueOnce('conflict'); + arrange([ + { id: 778, started_at: '2024-01-01', finished_at: null, progress_pages: null }, + { id: 777, started_at: '2024-01-01', finished_at: null, progress_pages: null }, + { id: 776, started_at: '2024-01-01', finished_at: null, progress_pages: null }, + ]); + insertRead(900); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('synced'); + + expect(mockClient.query).toHaveBeenNthCalledWith(4, 1, 'tok', expect.stringContaining('mutation InsertUserBookRead'), expect.anything()); + expect(mockRepo.upsertBookState).toHaveBeenCalledWith(expect.objectContaining({ hardcoverReadId: 900 })); + }); + + it('fails the sync instead of recording success when a brand new read cannot be claimed', async () => { + const errorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined); + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue(undefined); + mockRepo.findReadingAttempts.mockResolvedValue([attempt(2, { startedOn: '2024-01-01' })]); + mockRepo.linkReadingAttempt.mockResolvedValue('conflict'); + arrange([]); + insertRead(900); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('failed'); + + expect(mockRepo.upsertBookState).toHaveBeenCalledWith(expect.objectContaining({ syncError: 'read_link_conflict' })); + expect(mockRepo.upsertBookState).not.toHaveBeenCalledWith(expect.objectContaining({ syncError: null })); + errorSpy.mockRestore(); + }); + + it('logs the driver cause so a constraint failure names its constraint', async () => { + const errorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined); + mockRepo.findBookSyncData.mockResolvedValue(readingBook); + mockRepo.findBookState.mockResolvedValue(undefined); + mockRepo.findReadingAttempts.mockResolvedValue([attempt(2, { startedOn: '2024-01-01' })]); + mockRepo.linkReadingAttempt.mockRejectedValue( + Object.assign(new Error('Failed query: update "reading_attempts"'), { + cause: { code: '23505', constraint: 'reading_attempts_external_uidx', message: 'duplicate key value' }, + }), + ); + arrange([{ id: 777, started_at: '2024-01-01', finished_at: null, progress_pages: null }]); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('failed'); + + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining('cause="23505 reading_attempts_external_uidx duplicate key value"')); + errorSpy.mockRestore(); + }); + }); + describe('change detection', () => { + const syncedState = { + hardcoverBookId: 10, + hardcoverReadId: 700, + lastSyncedAt: new Date('2026-08-21T10:00:00Z'), + lastSyncedStatus: 'reading', + lastSyncedProgress: 42, + lastSyncedRating: null, + lastSyncedStartedAt: '2024-01-01', + lastSyncedFinishedAt: null, + }; + + it('skips a book whose watched fields all match the last sync', async () => { + mockSettingsService.getTokenForUser.mockResolvedValue('tok'); + mockRepo.findBookSyncData.mockResolvedValue({ ...readingBook, attemptsUpdatedAt: new Date('2026-08-21T09:00:00Z') }); + mockRepo.findBookState.mockResolvedValue(syncedState); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('skipped'); + expect(mockMatchService.matchBook).not.toHaveBeenCalled(); + }); + + it('re-syncs when only the attempt history changed', async () => { + mockSettingsService.getTokenForUser.mockResolvedValue('tok'); + mockRepo.findBookSyncData.mockResolvedValue({ ...readingBook, attemptsUpdatedAt: new Date('2026-08-21T11:00:00Z') }); + mockRepo.findBookState.mockResolvedValue(syncedState); + mockMatchService.matchBook.mockResolvedValue({ hardcoverBookId: 10, hardcoverEditionId: 20, editionPages: 300, matchMethod: 'cached' }); + mockClient.query + .mockResolvedValueOnce({ insert_user_book: { user_book: { id: 55 }, error: null } }) + .mockResolvedValueOnce({ update_user_book: { user_book: { id: 55 }, error: null } }) + .mockResolvedValueOnce({ user_book_reads: [{ id: 700, started_at: '2024-01-01', finished_at: null, progress_pages: 120 }] }) + .mockResolvedValueOnce({ update_user_book_read: { user_book_read: { id: 700 }, error: null } }); + + await expect(makeService().syncBook(1, 1)).resolves.toBe('synced'); + }); + }); }); diff --git a/server/src/modules/hardcover/hardcover-sync.service.ts b/server/src/modules/hardcover/hardcover-sync.service.ts index 30361afa3..2980cd567 100644 --- a/server/src/modules/hardcover/hardcover-sync.service.ts +++ b/server/src/modules/hardcover/hardcover-sync.service.ts @@ -18,6 +18,16 @@ import { BookService } from '../book/book.service'; import { HARDCOVER_STATUS } from './hardcover.constants'; import { HardcoverBookMatchService } from './hardcover-book-match.service'; import { HardcoverClientService } from './hardcover-client.service'; +import { + attemptOwnedReadId, + type HardcoverReadCandidate, + type HardcoverReadTarget, + readTargetForAttempt, + readTargetForBook, + type ReadingAttemptForSync, + selectAdoptableReadId, + selectPrimaryAttempt, +} from './hardcover-read-selection'; import { type BookSyncData, HardcoverRepository } from './hardcover.repository'; import { HardcoverSettingsService } from './hardcover-settings.service'; import { @@ -28,6 +38,9 @@ import { const LINKED_BOOKS_LIMIT = 200; +// Bounds how many times a lost ownership race may force reselection before the sync mints a new read. +const MAX_READ_ADOPTION_ROUNDS = 3; + const STATUS_MAP: Partial> = { want_to_read: HARDCOVER_STATUS.WANT_TO_READ, reading: HARDCOVER_STATUS.CURRENTLY_READING, @@ -109,6 +122,20 @@ type FindUserBookReadsResult = { export type HardcoverSyncBookResult = 'synced' | 'skipped' | 'failed'; type HardcoverBookStateSnapshot = Awaited>; +/** Drizzle hides the driver error - and with it the constraint name - behind `cause`. */ +function describeErrorCause(error: unknown): string { + let current: unknown = error instanceof Error ? error.cause : undefined; + const seen = new Set(); + while (current && typeof current === 'object' && !seen.has(current)) { + seen.add(current); + const entry = current as { code?: unknown; constraint?: unknown; message?: unknown; cause?: unknown }; + const parts = [entry.code, entry.constraint, entry.message].filter((part): part is string => typeof part === 'string' && part.length > 0); + if (parts.length > 0) return parts.join(' '); + current = entry.cause; + } + return ''; +} + function toDateString(d: Date | null | undefined): string | null { if (!d) return null; return d instanceof Date ? d.toISOString().split('T')[0]! : null; @@ -476,20 +503,34 @@ export class HardcoverSyncService { let hardcoverReadId: number | null = syncState?.hardcoverReadId ?? null; let progressSynced = true; - if (startDate || endDate || book.progress != null) { - if (book.progress != null && !match.editionPages) { - throw new Error('missing_edition_pages'); - } + const localAttempts = typeof this.repo.findReadingAttempts === 'function' ? await this.repo.findReadingAttempts(userId, book.bookId) : []; + const primaryAttempt = selectPrimaryAttempt(localAttempts); + // Every read id already owned on this book, tombstones included, so no attempt adopts one that + // reading_attempts_external_uidx would refuse. Grows as this run claims further ids. + const reserved = new Set( + typeof this.repo.findClaimedHardcoverReadIds === 'function' ? await this.repo.findClaimedHardcoverReadIds(userId, book.bookId) : [], + ); + const historyAttempts = localAttempts.filter( + (attempt) => attempt.id !== primaryAttempt?.id && attempt.outcome !== 'skimmed' && attempt.outcome !== 'abandoned', + ); + const syncsPrimaryRead = startDate !== null || endDate !== null || book.progress != null; + if (syncsPrimaryRead && book.progress != null && !match.editionPages) { + throw new Error('missing_edition_pages'); + } + const reads = syncsPrimaryRead || historyAttempts.length > 0 ? await this.findUserBookReads(userId, token, userBookId) : []; + if (syncsPrimaryRead) { const progressPages = book.progress != null && match.editionPages ? Math.round((book.progress / 100) * match.editionPages) : undefined; progressSynced = book.progress == null || progressPages != null; - const reads = await this.findUserBookReads(userId, token, userBookId); - const targetReadId = this.resolveTargetReadId(hardcoverReadId, reads); - - hardcoverReadId = await this.upsertUserBookRead(userId, token, { + hardcoverReadId = await this.syncAttemptRead(userId, token, { userBookId, - existingReadId: targetReadId, + attempt: primaryAttempt, + // The projection writes the primary attempt's dates onto the book, so the book-level dates + // this sync sends are the same dates the read must carry. + target: readTargetForBook(startDate, endDate), + reads, + reserved, startedAt: startDate, finishedAt: endDate, progressPages, @@ -500,6 +541,7 @@ export class HardcoverSyncService { await this.updateAdditionalOpenReads(userId, token, { reads, primaryReadId: hardcoverReadId, + reservedReadIds: reserved, startedAt: startDate, finishedAt: endDate, progressPages, @@ -512,26 +554,17 @@ export class HardcoverSyncService { } } - const localAttempts = typeof this.repo.findReadingAttempts === 'function' ? await this.repo.findReadingAttempts(userId, book.bookId) : []; - const primaryAttempt = [...localAttempts].reverse().find((attempt) => { - const attemptStarted = attempt.startedOn ?? null; - const attemptEnded = attempt.outcome === 'completed' ? (attempt.endedOn ?? null) : null; - return attemptStarted === startDate && attemptEnded === endDate; - }); - if (hardcoverReadId != null && primaryAttempt) { - await this.repo.linkReadingAttempt(userId, primaryAttempt.id, hardcoverReadId); - } - for (const attempt of localAttempts) { - if (attempt.id === primaryAttempt?.id || attempt.outcome === 'skimmed' || attempt.outcome === 'abandoned') continue; - const existingReadId = attempt.externalProvider === 'hardcover' && attempt.externalId ? Number(attempt.externalId) : null; - const readId = await this.upsertUserBookRead(userId, token, { + for (const attempt of historyAttempts) { + await this.syncAttemptRead(userId, token, { userBookId, - existingReadId: Number.isInteger(existingReadId) ? existingReadId : null, + attempt, + target: readTargetForAttempt(attempt), + reads, + reserved, startedAt: attempt.startedOn, finishedAt: attempt.outcome === 'completed' ? attempt.endedOn : null, editionId: match.hardcoverEditionId ?? undefined, }); - if (readId != null) await this.repo.linkReadingAttempt(userId, attempt.id, readId); } await this.repo.upsertBookState({ @@ -559,8 +592,10 @@ export class HardcoverSyncService { } catch (err) { const errorClass = err instanceof Error ? err.constructor.name : 'Error'; const error = sanitizeLogValue(err instanceof Error ? err.message : String(err)); + const cause = describeErrorCause(err); + const causeField = cause ? ` cause="${sanitizeLogValue(cause)}"` : ''; this.logger.error( - `[hardcover.sync_book] [fail] userId=${userId} bookId=${book.bookId} durationMs=${Date.now() - startedAt} errorClass=${errorClass} error="${error}" - sync failed`, + `[hardcover.sync_book] [fail] userId=${userId} bookId=${book.bookId} durationMs=${Date.now() - startedAt} errorClass=${errorClass} error="${error}"${causeField} - sync failed`, ); await this.repo.upsertBookState({ userId, @@ -675,19 +710,76 @@ export class HardcoverSyncService { } private async findUserBookReads(userId: number, token: string, userBookId: number): Promise { - try { - const result = await this.client.query(userId, token, FIND_USER_BOOK_READS_QUERY, { userBookId }); - return result.user_book_reads ?? []; - } catch { - return []; - } + const result = await this.client.query(userId, token, FIND_USER_BOOK_READS_QUERY, { userBookId }); + return result.user_book_reads ?? []; } - private resolveTargetReadId(existingReadId: number | null, reads: FindUserBookReadsResult['user_book_reads']): number | null { - const openRead = reads.find((read) => !read.finished_at); - if (openRead) return openRead.id; - if (existingReadId) return existingReadId; - return reads[0]?.id ?? null; + /** + * Resolves the one Hardcover read that represents `attempt` and writes this sync's values to it. + * + * Ownership lives on reading_attempts.external_id, never on the book-level cache. An attempt that + * already owns a read keeps it; an unlinked attempt may only adopt a read nothing else owns, and + * claims it locally before the remote write so a lost race cannot edit another attempt's read. + * When nothing is adoptable the sync mints a fresh read rather than sharing an existing one. + */ + private async syncAttemptRead( + userId: number, + token: string, + input: { + userBookId: number; + attempt: ReadingAttemptForSync | null | undefined; + target: HardcoverReadTarget; + reads: readonly HardcoverReadCandidate[]; + reserved: Set; + startedAt?: string | null; + finishedAt?: string | null; + progressPages?: number; + editionId?: number; + }, + ): Promise { + const attempt = input.attempt ?? null; + const write = (existingReadId: number | null): Promise => + this.upsertUserBookRead(userId, token, { + userBookId: input.userBookId, + existingReadId, + startedAt: input.startedAt, + finishedAt: input.finishedAt, + progressPages: input.progressPages, + editionId: input.editionId, + }); + + const owned = attempt ? attemptOwnedReadId(attempt) : null; + if (owned != null && attempt) { + input.reserved.add(owned); + const readId = await write(owned); + if (readId == null || readId === owned) return readId; + // Hardcover answered with a different read, so move the stamp rather than leave it stale. + input.reserved.add(readId); + if ((await this.repo.linkReadingAttempt(userId, attempt.id, readId)) === 'conflict') { + throw new Error('read_link_conflict'); + } + return readId; + } + + const excluded = new Set(input.reserved); + for (let round = 0; round < MAX_READ_ADOPTION_ROUNDS; round++) { + const candidate = selectAdoptableReadId(input.reads, input.target, excluded); + if (candidate == null) break; + if (attempt && (await this.repo.linkReadingAttempt(userId, attempt.id, candidate)) === 'conflict') { + excluded.add(candidate); + continue; + } + input.reserved.add(candidate); + return await write(candidate); + } + + const created = await write(null); + if (created == null) return null; + input.reserved.add(created); + if (attempt && (await this.repo.linkReadingAttempt(userId, attempt.id, created)) === 'conflict') { + throw new Error('read_link_conflict'); + } + return created; } private async updateAdditionalOpenReads( @@ -696,13 +788,16 @@ export class HardcoverSyncService { input: { reads: FindUserBookReadsResult['user_book_reads']; primaryReadId: number; + reservedReadIds: ReadonlySet; startedAt?: string | null; finishedAt?: string | null; progressPages: number; editionId?: number; }, ): Promise { - const siblingReadIds = input.reads.filter((read) => read.id !== input.primaryReadId && !read.finished_at).map((read) => read.id); + const siblingReadIds = input.reads + .filter((read) => read.id !== input.primaryReadId && !read.finished_at && !input.reservedReadIds.has(read.id)) + .map((read) => read.id); if (siblingReadIds.length === 0) return; diff --git a/server/src/modules/hardcover/hardcover.repository.test.ts b/server/src/modules/hardcover/hardcover.repository.test.ts index e4034c6cb..caee5c170 100644 --- a/server/src/modules/hardcover/hardcover.repository.test.ts +++ b/server/src/modules/hardcover/hardcover.repository.test.ts @@ -35,7 +35,9 @@ function makeRepository() { const settingsInsert = makeReturningChain(settingsRow); const bookStateInsert = makeReturningChain(bookStateRow); const deleteChain = makeWhereChain(undefined); - const updateChain = { set: vi.fn().mockReturnValue(makeWhereChain(undefined)) }; + const updateReturning = vi.fn().mockResolvedValue([{ id: 3 }]); + const updateWhere = vi.fn().mockReturnValue({ returning: updateReturning }); + const updateChain = { set: vi.fn().mockReturnValue({ where: updateWhere }) }; const bookIdLimit = vi.fn().mockResolvedValue([{ bookId: 42 }]); const bookIdWhere = vi.fn().mockReturnValue({ limit: bookIdLimit }); const bookIdFrom = vi.fn().mockReturnValue({ where: bookIdWhere }); @@ -64,6 +66,8 @@ function makeRepository() { bookStateInsert, deleteChain, updateChain, + updateReturning, + updateWhere, bookIdLimit, bookIdWhere, permissionLimit, @@ -440,4 +444,101 @@ describe('HardcoverRepository', () => { expect(progressInsert.values).toHaveBeenCalledWith(expect.objectContaining({ percentage: 0 })); expect(progressInsert.onConflictDoUpdate).toHaveBeenCalledWith(expect.objectContaining({ set: expect.objectContaining({ percentage: 0 }) })); }); + describe('linkReadingAttempt', () => { + function pgError(fields: Record) { + return Object.assign(new Error('Failed query: update "reading_attempts"'), { cause: fields }); + } + + function rejectUpdateWith(updateChain: { set: ReturnType }, error: unknown) { + updateChain.set.mockReturnValue({ where: vi.fn().mockReturnValue({ returning: vi.fn().mockRejectedValue(error) }) }); + } + + it('stamps the attempt and reports it linked', async () => { + const { repo, updateChain } = makeRepository(); + + await expect(repo.linkReadingAttempt(7, 3, 555)).resolves.toBe('linked'); + expect(updateChain.set).toHaveBeenCalledWith(expect.objectContaining({ externalProvider: 'hardcover', externalId: '555' })); + }); + + it('reports a conflict when another attempt already owns the read', async () => { + const { repo, updateChain } = makeRepository(); + rejectUpdateWith(updateChain, pgError({ code: '23505', constraint: 'reading_attempts_external_uidx' })); + + await expect(repo.linkReadingAttempt(7, 3, 555)).resolves.toBe('conflict'); + }); + + it('reports a conflict when the attempt was already claimed by a concurrent sync', async () => { + const { repo, updateReturning } = makeRepository(); + updateReturning.mockResolvedValue([]); + + await expect(repo.linkReadingAttempt(7, 3, 555)).resolves.toBe('conflict'); + }); + + it('recognises the conflict when the driver only names the index in the message', async () => { + const { repo, updateChain } = makeRepository(); + rejectUpdateWith( + updateChain, + pgError({ code: '23505', message: 'duplicate key value violates unique constraint "reading_attempts_external_uidx"' }), + ); + + await expect(repo.linkReadingAttempt(7, 3, 555)).resolves.toBe('conflict'); + }); + + it('recognises a conflict reported directly rather than through a cause', async () => { + const { repo, updateChain } = makeRepository(); + rejectUpdateWith(updateChain, Object.assign(new Error('duplicate key'), { code: '23505', constraint: 'reading_attempts_external_uidx' })); + + await expect(repo.linkReadingAttempt(7, 3, 555)).resolves.toBe('conflict'); + }); + + it('rethrows a unique violation on a different constraint', async () => { + const { repo, updateChain } = makeRepository(); + rejectUpdateWith(updateChain, pgError({ code: '23505', constraint: 'reading_attempts_one_active_uidx' })); + + await expect(repo.linkReadingAttempt(7, 3, 555)).rejects.toThrow('Failed query'); + }); + + it('rethrows errors that are not unique violations', async () => { + const { repo, updateChain } = makeRepository(); + rejectUpdateWith(updateChain, pgError({ code: '40001', message: 'could not serialize access' })); + + await expect(repo.linkReadingAttempt(7, 3, 555)).rejects.toThrow('Failed query'); + }); + + it('does not loop on a self-referencing cause chain', async () => { + const { repo, updateChain } = makeRepository(); + const error = new Error('boom') as Error & { cause?: unknown }; + error.cause = error; + rejectUpdateWith(updateChain, error); + + await expect(repo.linkReadingAttempt(7, 3, 555)).rejects.toThrow('boom'); + }); + }); + + describe('findClaimedHardcoverReadIds', () => { + function withRows(db: { select: ReturnType }, rows: Array<{ externalId: string | null }>) { + db.select.mockReturnValue({ from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue(rows) }) }); + } + + it('returns the read ids already claimed on the book', async () => { + const { repo, db } = makeRepository(); + withRows(db, [{ externalId: '555' }, { externalId: '777' }]); + + await expect(repo.findClaimedHardcoverReadIds(7, 42)).resolves.toEqual([555, 777]); + }); + + it('drops ids that are not usable read identifiers', async () => { + const { repo, db } = makeRepository(); + withRows(db, [{ externalId: 'abc' }, { externalId: '0' }, { externalId: null }, { externalId: '-1' }, { externalId: '12' }]); + + await expect(repo.findClaimedHardcoverReadIds(7, 42)).resolves.toEqual([12]); + }); + + it('returns an empty list when nothing is claimed', async () => { + const { repo, db } = makeRepository(); + withRows(db, []); + + await expect(repo.findClaimedHardcoverReadIds(7, 42)).resolves.toEqual([]); + }); + }); }); diff --git a/server/src/modules/hardcover/hardcover.repository.ts b/server/src/modules/hardcover/hardcover.repository.ts index 7053392fb..21839d430 100644 --- a/server/src/modules/hardcover/hardcover.repository.ts +++ b/server/src/modules/hardcover/hardcover.repository.ts @@ -1,16 +1,44 @@ import { Inject, Injectable } from '@nestjs/common'; import { Permission } from '@bookorbit/types'; import type { ContentFilterRules, ReadStatus } from '@bookorbit/types'; -import { and, asc, eq, inArray, ne, sql } from 'drizzle-orm'; +import { and, asc, eq, inArray, isNotNull, isNull, ne, or, sql } from 'drizzle-orm'; import type { NodePgDatabase } from 'drizzle-orm/node-postgres'; import { buildContentFilterClauses } from '../../common/utils/content-filter-sql.utils'; import { DB } from '../../db'; import * as schema from '../../db/schema'; import type { HardcoverBookState, HardcoverUserSetting, NewHardcoverBookState, NewHardcoverUserSetting } from '../../db/schema'; +import { HARDCOVER_EXTERNAL_PROVIDER } from './hardcover-read-selection'; type Db = NodePgDatabase; +const READING_ATTEMPT_EXTERNAL_CONSTRAINT = 'reading_attempts_external_uidx'; + +export type HardcoverLinkOutcome = 'linked' | 'conflict'; + +function* iterateErrorChain(error: unknown): Generator> { + let current: unknown = error; + const seen = new Set(); + while (current && typeof current === 'object' && !seen.has(current)) { + seen.add(current); + yield current as Record; + current = (current as { cause?: unknown }).cause; + } +} + +function asString(value: unknown): string { + return typeof value === 'string' ? value : ''; +} + +function isExternalIdConflict(error: unknown): boolean { + for (const entry of iterateErrorChain(error)) { + if (entry['code'] !== '23505') continue; + if (asString(entry['constraint']) === READING_ATTEMPT_EXTERNAL_CONSTRAINT) return true; + if (asString(entry['message']).includes(READING_ATTEMPT_EXTERNAL_CONSTRAINT)) return true; + } + return false; +} + export interface BookSyncData { bookId: number; isbn13: string | null; @@ -208,7 +236,9 @@ export class HardcoverRepository { const attemptsSq = this.db .select({ bookId: schema.readingAttempts.bookId, - updatedAt: sql`max(${schema.readingAttempts.updatedAt})`.as('attempts_updated_at'), + // mapWith keeps the driver's raw timestamp string from reaching callers: hasChanges compares + // this against a mapped Date, and a string/Date comparison is silently always false. + updatedAt: sql`max(${schema.readingAttempts.updatedAt})`.mapWith(schema.readingAttempts.updatedAt).as('attempts_updated_at'), }) .from(schema.readingAttempts) .where(and(eq(schema.readingAttempts.userId, userId), sql`${schema.readingAttempts.deletedAt} is null`)) @@ -263,11 +293,59 @@ export class HardcoverRepository { .orderBy(schema.readingAttempts.id); } - async linkReadingAttempt(userId: number, attemptId: number, hardcoverReadId: number): Promise { - await this.db - .update(schema.readingAttempts) - .set({ externalProvider: 'hardcover', externalId: String(hardcoverReadId), updatedAt: new Date() }) - .where(and(eq(schema.readingAttempts.userId, userId), eq(schema.readingAttempts.id, attemptId))); + /** + * Reports an ownership conflict instead of throwing. Another attempt - possibly a soft-deleted + * tombstone - can already own this read, and the caller recovers by selecting a different read + * rather than by merging or reassigning attempts. + */ + async linkReadingAttempt(userId: number, attemptId: number, hardcoverReadId: number): Promise { + const externalId = String(hardcoverReadId); + try { + const rows = await this.db + .update(schema.readingAttempts) + .set({ externalProvider: HARDCOVER_EXTERNAL_PROVIDER, externalId, updatedAt: new Date() }) + .where( + and( + eq(schema.readingAttempts.userId, userId), + eq(schema.readingAttempts.id, attemptId), + isNull(schema.readingAttempts.deletedAt), + or( + and(isNull(schema.readingAttempts.externalProvider), isNull(schema.readingAttempts.externalId)), + and(eq(schema.readingAttempts.externalProvider, HARDCOVER_EXTERNAL_PROVIDER), eq(schema.readingAttempts.externalId, externalId)), + ), + ), + ) + .returning({ id: schema.readingAttempts.id }); + return rows.length > 0 ? 'linked' : 'conflict'; + } catch (err) { + if (isExternalIdConflict(err)) return 'conflict'; + throw err; + } + } + + /** + * Read ids already spoken for on this book. Soft-deleted attempts are included on purpose: + * reading_attempts_external_uidx has no deleted_at predicate, and a deleted attempt keeping its + * external id is the tombstone importExternalRead relies on to avoid re-importing a removed read. + */ + async findClaimedHardcoverReadIds(userId: number, bookId: number): Promise { + const rows = await this.db + .select({ externalId: schema.readingAttempts.externalId }) + .from(schema.readingAttempts) + .where( + and( + eq(schema.readingAttempts.userId, userId), + eq(schema.readingAttempts.bookId, bookId), + eq(schema.readingAttempts.externalProvider, HARDCOVER_EXTERNAL_PROVIDER), + isNotNull(schema.readingAttempts.externalId), + ), + ); + const ids: number[] = []; + for (const row of rows) { + const parsed = Number(row.externalId); + if (Number.isInteger(parsed) && parsed > 0) ids.push(parsed); + } + return ids; } async findBookIdByFileId(bookFileId: number): Promise { diff --git a/server/test/reading-attempts.e2e-spec.ts b/server/test/reading-attempts.e2e-spec.ts index 8bd6e8fd0..3903249a5 100644 --- a/server/test/reading-attempts.e2e-spec.ts +++ b/server/test/reading-attempts.e2e-spec.ts @@ -494,4 +494,137 @@ describe('Reading attempts main-flow simulation (docker e2e)', { timeout: TIMEOU }), ).rejects.toThrow(); }); + it('19. reports an ownership conflict instead of throwing when another attempt holds the read', async () => { + const book = await createBook(); + const attempts = ctx.app.get(ReadingAttemptService); + const hardcover = ctx.app.get(HardcoverRepository); + const owner = await attempts.createHistorical(adminUserId, book.bookId, { + startedOn: '2022-01-01', + endedOn: '2022-01-05', + outcome: 'completed', + }); + const rival = await attempts.createHistorical(adminUserId, book.bookId, { + startedOn: '2022-03-01', + endedOn: '2022-03-05', + outcome: 'completed', + }); + + await expect(hardcover.linkReadingAttempt(adminUserId, owner.id, 9101)).resolves.toBe('linked'); + await expect(hardcover.linkReadingAttempt(adminUserId, rival.id, 9101)).resolves.toBe('conflict'); + + const [rivalRow] = await ctx.db.select().from(schema.readingAttempts).where(eq(schema.readingAttempts.id, rival.id)); + expect(rivalRow?.externalId).toBeNull(); + const [ownerRow] = await ctx.db.select().from(schema.readingAttempts).where(eq(schema.readingAttempts.id, owner.id)); + expect(ownerRow).toMatchObject({ externalId: '9101', deletedAt: null }); + }); + + it('20. keeps a soft-deleted attempt tombstone reserved against reuse', async () => { + const book = await createBook(); + const attempts = ctx.app.get(ReadingAttemptService); + const hardcover = ctx.app.get(HardcoverRepository); + const removed = await attempts.createHistorical(adminUserId, book.bookId, { + startedOn: '2022-06-01', + endedOn: '2022-06-05', + outcome: 'completed', + }); + await hardcover.linkReadingAttempt(adminUserId, removed.id, 9202); + await attempts.delete(adminUserId, book.bookId, removed.id); + const survivor = await attempts.createHistorical(adminUserId, book.bookId, { + startedOn: '2022-09-01', + endedOn: '2022-09-05', + outcome: 'completed', + }); + + await expect(hardcover.linkReadingAttempt(adminUserId, survivor.id, 9202)).resolves.toBe('conflict'); + await expect(hardcover.findClaimedHardcoverReadIds(adminUserId, book.bookId)).resolves.toContain(9202); + }); + + it('21. lists claimed read ids across live and soft-deleted attempts', async () => { + const book = await createBook(); + const attempts = ctx.app.get(ReadingAttemptService); + const hardcover = ctx.app.get(HardcoverRepository); + const live = await attempts.createHistorical(adminUserId, book.bookId, { + startedOn: '2023-01-01', + endedOn: '2023-01-05', + outcome: 'completed', + }); + const deleted = await attempts.createHistorical(adminUserId, book.bookId, { + startedOn: '2023-04-01', + endedOn: '2023-04-05', + outcome: 'completed', + }); + await hardcover.linkReadingAttempt(adminUserId, live.id, 9301); + await hardcover.linkReadingAttempt(adminUserId, deleted.id, 9302); + await attempts.delete(adminUserId, book.bookId, deleted.id); + + const claimed = await hardcover.findClaimedHardcoverReadIds(adminUserId, book.bookId); + + expect(new Set(claimed)).toEqual(new Set([9301, 9302])); + }); + + it('22. lets an attempt restamp the same read but rejects reassignment', async () => { + const book = await createBook(); + const attempts = ctx.app.get(ReadingAttemptService); + const hardcover = ctx.app.get(HardcoverRepository); + const attempt = await attempts.createHistorical(adminUserId, book.bookId, { + startedOn: '2023-07-01', + endedOn: '2023-07-05', + outcome: 'completed', + }); + + await expect(hardcover.linkReadingAttempt(adminUserId, attempt.id, 9401)).resolves.toBe('linked'); + await expect(hardcover.linkReadingAttempt(adminUserId, attempt.id, 9401)).resolves.toBe('linked'); + await expect(hardcover.linkReadingAttempt(adminUserId, attempt.id, 9402)).resolves.toBe('conflict'); + + await expect(hardcover.findClaimedHardcoverReadIds(adminUserId, book.bookId)).resolves.toEqual([9401]); + }); + + it('23. lets exactly one concurrent claim establish attempt ownership', async () => { + const book = await createBook(); + const attempts = ctx.app.get(ReadingAttemptService); + const hardcover = ctx.app.get(HardcoverRepository); + const attempt = await attempts.createHistorical(adminUserId, book.bookId, { + startedOn: '2023-08-01', + endedOn: '2023-08-05', + outcome: 'completed', + }); + + const outcomes = await Promise.all([ + hardcover.linkReadingAttempt(adminUserId, attempt.id, 9501), + hardcover.linkReadingAttempt(adminUserId, attempt.id, 9502), + ]); + + expect(outcomes.sort()).toEqual(['conflict', 'linked']); + const claimed = await hardcover.findClaimedHardcoverReadIds(adminUserId, book.bookId); + expect(claimed).toHaveLength(1); + expect([9501, 9502]).toContain(claimed[0]); + }); + it('24. surfaces attempt timestamps as Dates so sync change detection can compare them', async () => { + const book = await createBook(); + const attempts = ctx.app.get(ReadingAttemptService); + const hardcover = ctx.app.get(HardcoverRepository); + const before = new Date(Date.now() - 60_000); + await attempts.createHistorical(adminUserId, book.bookId, { + startedOn: '2024-02-01', + endedOn: '2024-02-05', + outcome: 'completed', + }); + + const sync = await hardcover.findBookSyncData(adminUserId, book.bookId); + const updatedAt = sync?.attemptsUpdatedAt ?? null; + + // A raw driver timestamp string compares false against a Date, which silently disables the + // attempt-history branch of hasChanges and stops attempt edits from ever re-triggering a sync. + expect(updatedAt).toBeInstanceOf(Date); + expect(updatedAt instanceof Date && updatedAt > before).toBe(true); + }); + + it('25. leaves the attempt timestamp null for a book with no attempts', async () => { + const book = await createBook(); + const hardcover = ctx.app.get(HardcoverRepository); + + const sync = await hardcover.findBookSyncData(adminUserId, book.bookId); + + expect(sync?.attemptsUpdatedAt ?? null).toBeNull(); + }); });