mirror of
https://github.com/bookorbit/bookorbit.git
synced 2026-11-03 08:21:39 -06:00
fix(opds): stable sort tiebreaker, unique feed IDs, remove dead code (#94)
Add books.id ASC tiebreaker to all 7 OPDS_SORT_MAP sort orders so pagination is deterministic even when books share identical addedAt timestamps (e.g. after a bulk library scan). Without a tiebreaker, PostgreSQL returns equal-valued rows in an undefined order, causing books to appear on multiple pages or not at all. Make catalog feed IDs context-specific: filtered views (by library, collection, smartScope, author, series, or search query) now produce distinct URNs instead of all sharing urn:bookorbit:catalog. This aligns with the OPDS spec requirement that <id> uniquely identify the logical feed. Remove the dead idQuery.$dynamic() block from paginatedBookQuery that was built and conditionally mutated but never awaited. Closes #93
This commit is contained in:
@@ -214,10 +214,10 @@ describe('OpdsBookService', () => {
|
||||
});
|
||||
|
||||
it('paginates ids and only fetches entries when rows are present', async () => {
|
||||
const empty = makeService([[], [], [{ total: 5 }]]);
|
||||
const empty = makeService([[], [{ total: 5 }]]);
|
||||
await expect((empty.service as never).paginatedBookQuery({ kind: 'where' }, 'recent', 2, 10)).resolves.toEqual({ entries: [], total: 5 });
|
||||
|
||||
const filled = makeService([[], [{ id: 3 }, { id: 1 }], [{ total: 2 }]]);
|
||||
const filled = makeService([[{ id: 3 }, { id: 1 }], [{ total: 2 }]]);
|
||||
const fetchSpy = vi.spyOn(filled.service as never, 'fetchBookEntries').mockResolvedValue([{ id: 3 }, { id: 1 }]);
|
||||
|
||||
await expect((filled.service as never).paginatedBookQuery({ kind: 'where' }, 'author_asc', 1, 25)).resolves.toEqual({
|
||||
@@ -227,6 +227,26 @@ describe('OpdsBookService', () => {
|
||||
expect(fetchSpy).toHaveBeenCalledWith([3, 1]);
|
||||
});
|
||||
|
||||
it('every sort order includes a books.id tiebreaker as its final ORDER BY clause', async () => {
|
||||
const sortOrders = ['recent', 'title_asc', 'title_desc', 'author_asc', 'author_desc', 'series_asc', 'series_desc'] as const;
|
||||
|
||||
for (const sortOrder of sortOrders) {
|
||||
const { service, db } = makeService([[], [{ total: 0 }]]);
|
||||
await (service as never).paginatedBookQuery({ kind: 'where' }, sortOrder, 1, 25);
|
||||
|
||||
const chains = (db.select as ReturnType<typeof vi.fn>).mock.results.map((r: { value: Record<string, unknown> }) => r.value);
|
||||
const orderByArgs = chains.flatMap((chain: Record<string, unknown>) => {
|
||||
const fn = chain['orderBy'] as ReturnType<typeof vi.fn>;
|
||||
return fn.mock.calls.flat() as unknown[];
|
||||
});
|
||||
|
||||
const allValues = orderByArgs.flatMap((arg: unknown) => collectValues(arg));
|
||||
const hasIdTiebreaker = allValues.some((v) => v === 'id');
|
||||
|
||||
expect(hasIdTiebreaker, `sort order "${sortOrder}" is missing books.id tiebreaker`).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it('maps metadata, authors, and files into ordered OPDS entries', async () => {
|
||||
const now = new Date('2026-04-15T00:00:00.000Z');
|
||||
const { service } = makeService([
|
||||
|
||||
@@ -25,13 +25,13 @@ type Db = NodePgDatabase<typeof schema>;
|
||||
type OpdsSortOrder = 'recent' | 'title_asc' | 'title_desc' | 'author_asc' | 'author_desc' | 'series_asc' | 'series_desc';
|
||||
|
||||
const OPDS_SORT_MAP: Record<OpdsSortOrder, SQL[]> = {
|
||||
recent: [sql`${books.addedAt} DESC`],
|
||||
title_asc: [sql`${bookMetadata.title} ASC NULLS LAST`],
|
||||
title_desc: [sql`${bookMetadata.title} DESC NULLS LAST`],
|
||||
author_asc: [sql`min(${authors.sortName}) ASC NULLS LAST`, sql`${bookMetadata.title} ASC NULLS LAST`],
|
||||
author_desc: [sql`min(${authors.sortName}) DESC NULLS LAST`, sql`${bookMetadata.title} ASC NULLS LAST`],
|
||||
series_asc: [sql`${bookMetadata.seriesName} ASC NULLS LAST`, sql`${bookMetadata.seriesIndex} ASC NULLS LAST`],
|
||||
series_desc: [sql`${bookMetadata.seriesName} DESC NULLS LAST`, sql`${bookMetadata.seriesIndex} DESC NULLS LAST`],
|
||||
recent: [sql`${books.addedAt} DESC`, sql`${books.id} ASC`],
|
||||
title_asc: [sql`${bookMetadata.title} ASC NULLS LAST`, sql`${books.id} ASC`],
|
||||
title_desc: [sql`${bookMetadata.title} DESC NULLS LAST`, sql`${books.id} ASC`],
|
||||
author_asc: [sql`min(${authors.sortName}) ASC NULLS LAST`, sql`${bookMetadata.title} ASC NULLS LAST`, sql`${books.id} ASC`],
|
||||
author_desc: [sql`min(${authors.sortName}) DESC NULLS LAST`, sql`${bookMetadata.title} ASC NULLS LAST`, sql`${books.id} ASC`],
|
||||
series_asc: [sql`${bookMetadata.seriesName} ASC NULLS LAST`, sql`${bookMetadata.seriesIndex} ASC NULLS LAST`, sql`${books.id} ASC`],
|
||||
series_desc: [sql`${bookMetadata.seriesName} DESC NULLS LAST`, sql`${bookMetadata.seriesIndex} DESC NULLS LAST`, sql`${books.id} ASC`],
|
||||
};
|
||||
|
||||
const LIKE_SPECIAL_CHARS = /[%_\\]/g;
|
||||
@@ -393,13 +393,6 @@ export class OpdsBookService {
|
||||
): Promise<{ entries: OpdsBookEntry[]; total: number }> {
|
||||
const offset = (page - 1) * size;
|
||||
const needsAuthorJoin = sortOrder === 'author_asc' || sortOrder === 'author_desc';
|
||||
|
||||
const idQuery = this.db.select({ id: books.id }).from(books).leftJoin(bookMetadata, eq(bookMetadata.bookId, books.id)).where(where).$dynamic();
|
||||
|
||||
if (needsAuthorJoin) {
|
||||
idQuery.leftJoin(bookAuthors, eq(bookAuthors.bookId, books.id)).leftJoin(authors, eq(authors.id, bookAuthors.authorId));
|
||||
}
|
||||
|
||||
const orderClauses = OPDS_SORT_MAP[sortOrder];
|
||||
|
||||
const [idRows, [{ total }]] = await Promise.all([
|
||||
|
||||
@@ -134,7 +134,7 @@ describe('OpdsController', () => {
|
||||
);
|
||||
expect(opdsService.generateAcquisitionFeed).toHaveBeenCalledWith(
|
||||
'Search: arrakis',
|
||||
'urn:bookorbit:catalog',
|
||||
'urn:bookorbit:catalog:author:Frank%20Herbert:collectionId:11:libraryId:2:q:arrakis:series:Dune:smartScopeId:15',
|
||||
[{ id: 1 }],
|
||||
1,
|
||||
1,
|
||||
@@ -144,6 +144,62 @@ describe('OpdsController', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('catalog generates unique feed ids per filter context', async () => {
|
||||
const user = { userId: 5, isSuperuser: false, sortOrder: 'recent', coverToken: 'tok' } as never;
|
||||
|
||||
const noFilters = makeController();
|
||||
await noFilters.controller.catalog(user, 1, 50, undefined, undefined, undefined, undefined, undefined, undefined, makeReply());
|
||||
expect(noFilters.opdsService.generateAcquisitionFeed).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
'urn:bookorbit:catalog',
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
);
|
||||
|
||||
const libraryOnly = makeController();
|
||||
await libraryOnly.controller.catalog(user, 1, 50, '1', undefined, undefined, undefined, undefined, undefined, makeReply());
|
||||
expect(libraryOnly.opdsService.generateAcquisitionFeed).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
'urn:bookorbit:catalog:libraryId:1',
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
);
|
||||
|
||||
const searchOnly = makeController();
|
||||
await searchOnly.controller.catalog(user, 1, 50, undefined, undefined, undefined, undefined, undefined, 'dune', makeReply());
|
||||
expect(searchOnly.opdsService.generateAcquisitionFeed).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
'urn:bookorbit:catalog:q:dune',
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
);
|
||||
|
||||
const multiFilter = makeController();
|
||||
await multiFilter.controller.catalog(user, 1, 50, '2', undefined, undefined, 'Frank Herbert', undefined, undefined, makeReply());
|
||||
expect(multiFilter.opdsService.generateAcquisitionFeed).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
'urn:bookorbit:catalog:author:Frank%20Herbert:libraryId:2',
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects invalid catalog filter ids and deep pagination windows', async () => {
|
||||
const { controller } = makeController();
|
||||
const user = { userId: 7, isSuperuser: false, sortOrder: 'recent', coverToken: 'token' } as never;
|
||||
|
||||
@@ -135,17 +135,14 @@ export class OpdsController {
|
||||
selfParams.set('size', String(clampedSize));
|
||||
const selfPath = `/api/v1/opds/catalog?${selfParams.toString()}`;
|
||||
|
||||
const filterSuffix = Object.entries(filters)
|
||||
.sort(([a], [b]) => a.localeCompare(b))
|
||||
.map(([k, v]) => `${k}:${encodeURIComponent(String(v))}`)
|
||||
.join(':');
|
||||
const feedId = filterSuffix ? `urn:bookorbit:catalog:${filterSuffix}` : 'urn:bookorbit:catalog';
|
||||
|
||||
const feedTitle = q ? `Search: ${q}` : 'Catalog';
|
||||
const xml = this.opdsService.generateAcquisitionFeed(
|
||||
feedTitle,
|
||||
'urn:bookorbit:catalog',
|
||||
entries,
|
||||
total,
|
||||
clampedPage,
|
||||
clampedSize,
|
||||
selfPath,
|
||||
user.coverToken,
|
||||
);
|
||||
const xml = this.opdsService.generateAcquisitionFeed(feedTitle, feedId, entries, total, clampedPage, clampedSize, selfPath, user.coverToken);
|
||||
this.sendXml(reply!, xml, OPDS_MIME_ACQ);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user