diff --git a/server/src/modules/koreader/koreader.repository.test.ts b/server/src/modules/koreader/koreader.repository.test.ts index ad999fb1c..8b0f3cc1e 100644 --- a/server/src/modules/koreader/koreader.repository.test.ts +++ b/server/src/modules/koreader/koreader.repository.test.ts @@ -20,8 +20,10 @@ function makeQueryChain(result: unknown) { } function makeDb() { + const select = vi.fn(); return { - select: vi.fn(), + select, + selectDistinctOn: vi.fn((_on, fields) => select(fields)), insert: vi.fn(), update: vi.fn(), delete: vi.fn(), @@ -114,6 +116,42 @@ describe('KoreaderRepository', () => { expect(db.select).toHaveBeenCalledTimes(1); }); + it('returns the oldest file when current hash matches stay within one book', async () => { + const firstFile = { id: 10, bookId: 20, libraryId: 1, format: 'epub' }; + const secondFile = { id: 11, bookId: 20, libraryId: 1, format: 'epub' }; + db.select.mockReturnValue(makeQueryChain([firstFile, secondFile])); + + await expect(repo.resolveBookFileByHash('abc123', null)).resolves.toEqual(firstFile); + expect(db.select).toHaveBeenCalledTimes(1); + }); + + it('returns null when a current hash matches different books', async () => { + db.select.mockReturnValue( + makeQueryChain([ + { id: 10, bookId: 20, libraryId: 1, format: 'epub' }, + { id: 11, bookId: 21, libraryId: 1, format: 'epub' }, + ]), + ); + + await expect(repo.resolveBookFileByHash('abc123', null)).resolves.toBeNull(); + expect(db.select).toHaveBeenCalledTimes(1); + }); + + it('uses a user-scoped manual link when a current hash matches different books', async () => { + const manualFile = { id: 11, bookId: 21, libraryId: 1, format: 'epub' }; + db.select + .mockReturnValueOnce( + makeQueryChain([ + { id: 10, bookId: 20, libraryId: 1, format: 'epub' }, + { id: 11, bookId: 21, libraryId: 1, format: 'epub' }, + ]), + ) + .mockReturnValueOnce(makeQueryChain([manualFile])); + + await expect(repo.resolveBookFileByHash('abc123', null, 7)).resolves.toEqual(manualFile); + expect(db.select).toHaveBeenCalledTimes(2); + }); + it('falls back to hash history when current hash lookup returns nothing', async () => { const file = { id: 10, bookId: 20, libraryId: 1, format: 'pdf' }; db.select.mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce(makeQueryChain([file])); @@ -124,6 +162,18 @@ describe('KoreaderRepository', () => { expect(db.select).toHaveBeenCalledTimes(2); }); + it('returns null when a historical hash matches different books', async () => { + db.select.mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce( + makeQueryChain([ + { id: 10, bookId: 20, libraryId: 1, format: 'epub' }, + { id: 11, bookId: 21, libraryId: 1, format: 'epub' }, + ]), + ); + + await expect(repo.resolveBookFileByHash('oldhash', null)).resolves.toBeNull(); + expect(db.select).toHaveBeenCalledTimes(2); + }); + it('falls back to a user-scoped manual link after direct and history lookups miss', async () => { const file = { id: 10, bookId: 20, libraryId: 1, format: 'cbz' }; db.select @@ -200,6 +250,64 @@ describe('KoreaderRepository', () => { expect(db.select).toHaveBeenCalledTimes(1); }); + it('resolves duplicate current hash rows when they belong to one book', async () => { + db.select.mockReturnValueOnce( + makeQueryChain([ + { hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }, + { hash: 'current', bookFileId: 12, bookId: 21, libraryId: 31, format: 'epub' }, + ]), + ); + + const result = await repo.resolveBookFilesByHashes(['current'], null); + + expect(result.get('current')).toEqual({ bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }); + expect(db.select).toHaveBeenCalledTimes(1); + }); + + it('omits a current hash that matches different books', async () => { + db.select.mockReturnValueOnce( + makeQueryChain([ + { hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }, + { hash: 'current', bookFileId: 12, bookId: 22, libraryId: 31, format: 'epub' }, + ]), + ); + + const result = await repo.resolveBookFilesByHashes(['current'], null); + + expect(result.has('current')).toBe(false); + expect(db.select).toHaveBeenCalledTimes(1); + }); + + it('uses a user-scoped manual link for an ambiguous current hash', async () => { + db.select + .mockReturnValueOnce( + makeQueryChain([ + { hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }, + { hash: 'current', bookFileId: 12, bookId: 22, libraryId: 31, format: 'epub' }, + ]), + ) + .mockReturnValueOnce(makeQueryChain([{ hash: 'current', bookFileId: 12, bookId: 22, libraryId: 31, format: 'epub' }])); + + const result = await repo.resolveBookFilesByHashes(['current'], null, 7); + + expect(result.get('current')).toEqual({ bookFileId: 12, bookId: 22, libraryId: 31, format: 'epub' }); + expect(db.select).toHaveBeenCalledTimes(2); + }); + + it('omits a historical hash that matches different books', async () => { + db.select.mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce( + makeQueryChain([ + { hash: 'old', bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }, + { hash: 'old', bookFileId: 12, bookId: 22, libraryId: 31, format: 'epub' }, + ]), + ); + + const result = await repo.resolveBookFilesByHashes(['old'], null); + + expect(result.has('old')).toBe(false); + expect(db.select).toHaveBeenCalledTimes(2); + }); + it('resolves remaining hashes from user-scoped manual links', async () => { db.select .mockReturnValueOnce(makeQueryChain([{ hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }])) diff --git a/server/src/modules/koreader/koreader.repository.ts b/server/src/modules/koreader/koreader.repository.ts index c393d2f7f..4dbfdb91b 100644 --- a/server/src/modules/koreader/koreader.repository.ts +++ b/server/src/modules/koreader/koreader.repository.ts @@ -24,6 +24,39 @@ type KoreaderHashLinkMetadata = { lastOpen?: number | null; }; +type HashedResolvedBookFile = ResolvedBookFileByHashes & { hash: string | null }; + +function resolveUniqueBook(rows: T[]): T | null { + const first = rows[0]; + if (!first) return null; + return rows.every((row) => row.bookId === first.bookId) ? first : null; +} + +function addUnambiguousHashMatches(rows: HashedResolvedBookFile[], result: Map): Set { + const rowsByHash = new Map(); + + for (const row of rows) { + if (!row.hash) continue; + const matches = rowsByHash.get(row.hash); + if (matches) matches.push(row); + else rowsByHash.set(row.hash, [row]); + } + + for (const [hash, matches] of rowsByHash) { + const resolved = resolveUniqueBook(matches); + if (resolved) { + result.set(hash, { + bookFileId: resolved.bookFileId, + bookId: resolved.bookId, + libraryId: resolved.libraryId, + format: resolved.format, + }); + } + } + + return new Set(rowsByHash.keys()); +} + export interface ReadingProgressUpsert { bookFileId: number; userId: number; @@ -78,49 +111,51 @@ export class KoreaderRepository { await this.db.delete(schema.koreaderUsers).where(eq(schema.koreaderUsers.userId, userId)); } - // A file hash is not unique: the same content in two places is two book file rows, and one - // historical hash can belong to several of them. The oldest row wins so a hash always resolves - // to the same target, both across requests and between this and resolveBookFilesByHashes. An - // unordered pick would move a device's sync target under it and strand data on the loser. + // KOReader's partial hash can collide for different files. Multiple matching files are safe only + // when they belong to the same book; otherwise a user-scoped manual link must disambiguate them. async resolveBookFileByHash(hash: string, accessibleLibraryIds: number[] | null, userId?: number): Promise { if (accessibleLibraryIds !== null && accessibleLibraryIds.length === 0) return null; const libraryFilter = accessibleLibraryIds ? inArray(schema.books.libraryId, accessibleLibraryIds) : undefined; - const [byFileHash] = await this.db - .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, format: schema.bookFiles.format }) + const byFileHash = await this.db + .selectDistinctOn([schema.bookFiles.bookId], { + id: schema.bookFiles.id, + bookId: schema.bookFiles.bookId, + libraryId: schema.books.libraryId, + format: schema.bookFiles.format, + }) .from(schema.bookFiles) .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) .where(and(eq(schema.bookFiles.fileHash, hash), libraryFilter)) - .orderBy(asc(schema.bookFiles.id)) - .limit(1); + .orderBy(asc(schema.bookFiles.bookId), asc(schema.bookFiles.id)) + .limit(2); - if (byFileHash) return byFileHash; + if (byFileHash.length > 0) { + return resolveUniqueBook(byFileHash) ?? (userId === undefined ? null : this.resolveManualBookFileByHash(hash, accessibleLibraryIds, userId)); + } - const [byFileHashHistory] = await this.db - .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, format: schema.bookFiles.format }) + const byFileHashHistory = await this.db + .selectDistinctOn([schema.bookFiles.bookId], { + id: schema.bookFiles.id, + bookId: schema.bookFiles.bookId, + libraryId: schema.books.libraryId, + format: schema.bookFiles.format, + }) .from(schema.bookFileHashHistory) .innerJoin(schema.bookFiles, eq(schema.bookFiles.id, schema.bookFileHashHistory.bookFileId)) .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) .where(and(eq(schema.bookFileHashHistory.fileHash, hash), libraryFilter)) - .orderBy(asc(schema.bookFiles.id)) - .limit(1); + .orderBy(asc(schema.bookFiles.bookId), asc(schema.bookFiles.id)) + .limit(2); - if (byFileHashHistory) return byFileHashHistory; - - if (userId !== undefined) { - const [byManualLink] = await this.db - .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, format: schema.bookFiles.format }) - .from(schema.koreaderBookHashLinks) - .innerJoin(schema.bookFiles, eq(schema.bookFiles.id, schema.koreaderBookHashLinks.bookFileId)) - .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) - .where(and(eq(schema.koreaderBookHashLinks.userId, userId), eq(schema.koreaderBookHashLinks.hash, hash), libraryFilter)) - .limit(1); - - if (byManualLink) return byManualLink; + if (byFileHashHistory.length > 0) { + return ( + resolveUniqueBook(byFileHashHistory) ?? (userId === undefined ? null : this.resolveManualBookFileByHash(hash, accessibleLibraryIds, userId)) + ); } - return null; + return userId === undefined ? null : this.resolveManualBookFileByHash(hash, accessibleLibraryIds, userId); } async resolveBookFilesByHashes( @@ -147,36 +182,28 @@ export class KoreaderRepository { .where(and(inArray(schema.bookFiles.fileHash, hashes), libraryFilter)) .orderBy(asc(schema.bookFiles.id)); - for (const row of direct) { - if (row.hash && !result.has(row.hash)) { - result.set(row.hash, { bookFileId: row.bookFileId, bookId: row.bookId, libraryId: row.libraryId, format: row.format }); - } + const directHashes = addUnambiguousHashMatches(direct, result); + + const missing = hashes.filter((hash) => !directHashes.has(hash)); + if (missing.length > 0) { + const history = await this.db + .select({ + hash: schema.bookFileHashHistory.fileHash, + bookFileId: schema.bookFiles.id, + bookId: schema.bookFiles.bookId, + libraryId: schema.books.libraryId, + format: schema.bookFiles.format, + }) + .from(schema.bookFileHashHistory) + .innerJoin(schema.bookFiles, eq(schema.bookFiles.id, schema.bookFileHashHistory.bookFileId)) + .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) + .where(and(inArray(schema.bookFileHashHistory.fileHash, missing), libraryFilter)) + .orderBy(asc(schema.bookFiles.id)); + + addUnambiguousHashMatches(history, result); } - const missing = hashes.filter((hash) => !result.has(hash)); - if (missing.length === 0) return result; - - const history = await this.db - .select({ - hash: schema.bookFileHashHistory.fileHash, - bookFileId: schema.bookFiles.id, - bookId: schema.bookFiles.bookId, - libraryId: schema.books.libraryId, - format: schema.bookFiles.format, - }) - .from(schema.bookFileHashHistory) - .innerJoin(schema.bookFiles, eq(schema.bookFiles.id, schema.bookFileHashHistory.bookFileId)) - .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) - .where(and(inArray(schema.bookFileHashHistory.fileHash, missing), libraryFilter)) - .orderBy(asc(schema.bookFiles.id)); - - for (const row of history) { - if (!result.has(row.hash)) { - result.set(row.hash, { bookFileId: row.bookFileId, bookId: row.bookId, libraryId: row.libraryId, format: row.format }); - } - } - - const stillMissing = missing.filter((hash) => !result.has(hash)); + const stillMissing = hashes.filter((hash) => !result.has(hash)); if (stillMissing.length === 0 || userId === undefined) return result; const manualLinks = await this.db @@ -201,6 +228,23 @@ export class KoreaderRepository { return result; } + private async resolveManualBookFileByHash( + hash: string, + accessibleLibraryIds: number[] | null, + userId: number, + ): Promise { + const libraryFilter = accessibleLibraryIds ? inArray(schema.books.libraryId, accessibleLibraryIds) : undefined; + const [byManualLink] = await this.db + .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, format: schema.bookFiles.format }) + .from(schema.koreaderBookHashLinks) + .innerJoin(schema.bookFiles, eq(schema.bookFiles.id, schema.koreaderBookHashLinks.bookFileId)) + .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) + .where(and(eq(schema.koreaderBookHashLinks.userId, userId), eq(schema.koreaderBookHashLinks.hash, hash), libraryFilter)) + .limit(1); + + return byManualLink ?? null; + } + async upsertUnmatchedBooks(userId: number, candidates: KoreaderUnmatchedCandidate[], deviceId?: string): Promise { if (candidates.length === 0) return; diff --git a/server/test/koreader-progress-position.e2e-spec.ts b/server/test/koreader-progress-position.e2e-spec.ts index c5eea473b..eeda86764 100644 --- a/server/test/koreader-progress-position.e2e-spec.ts +++ b/server/test/koreader-progress-position.e2e-spec.ts @@ -193,4 +193,26 @@ describe('KOReader progress position routing (e2e)', { timeout: 180_000 }, () => expect(stored?.pageNumber).toBeNull(); expect(stored?.percentage).toBeCloseTo(80, 5); }); + + it('does not route progress when the document hash matches different books', async () => { + const comicProgressBefore = await storedProgress(comic.bookFileId); + const epubProgressBefore = await storedProgress(epub.bookFileId); + + await ctx.db.update(schema.bookFiles).set({ fileHash: comicHash }).where(eq(schema.bookFiles.id, epub.bookFileId)); + + try { + const response = await ctx.app.inject({ + method: 'PUT', + url: '/api/v1/koreader/syncs/progress', + headers: deviceHeaders(), + payload: { document: comicHash, percentage: 0.99, progress: '999' }, + }); + + expect(response.statusCode).toBe(404); + await expect(storedProgress(comic.bookFileId)).resolves.toEqual(comicProgressBefore); + await expect(storedProgress(epub.bookFileId)).resolves.toEqual(epubProgressBefore); + } finally { + await ctx.db.update(schema.bookFiles).set({ fileHash: epubHash }).where(eq(schema.bookFiles.id, epub.bookFileId)); + } + }); });