mirror of
https://github.com/bookorbit/bookorbit.git
synced 2026-11-03 00:11:37 -06:00
fix(kobo): stop logging non-BookOrbit content as annotation sync failures
Kobo devices route every annotation request through us, including content BookOrbit does not own, so a content id that resolves to no book is expected traffic rather than a sync failure. Recording it surfaced a FAILED entry in the device sync history for each such request. Keep returning 404: the device reconciles annotations by omission, so answering with an empty set would make it delete its local annotations for that content. Closes #831
This commit is contained in:
@@ -1,6 +1,7 @@
|
||||
import { HttpStatus } from '@nestjs/common';
|
||||
|
||||
import { KoboReadingServicesController } from './kobo-reading-services.controller';
|
||||
import { KoboContentNotFoundException } from './services/kobo-annotation-exchange.service';
|
||||
|
||||
function makeReply() {
|
||||
return {
|
||||
@@ -101,6 +102,18 @@ describe('KoboReadingServicesController', () => {
|
||||
expect(reply.send).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('does not record a failure when a pulled content id is not a BookOrbit book', async () => {
|
||||
exchangeService.getContentAnnotations.mockRejectedValueOnce(new KoboContentNotFoundException('1894971763'));
|
||||
const reply = makeReply();
|
||||
|
||||
await expect(controller.getAnnotations('1894971763', undefined, { id: 8 } as never, { deviceId: 5 } as never, reply as never)).rejects.toThrow(
|
||||
KoboContentNotFoundException,
|
||||
);
|
||||
|
||||
expect(historyService.recordFailure).not.toHaveBeenCalled();
|
||||
expect(reply.send).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('records successful annotation pushes', async () => {
|
||||
exchangeService.patchContentAnnotations.mockResolvedValue({
|
||||
bookId: 42,
|
||||
@@ -136,6 +149,16 @@ describe('KoboReadingServicesController', () => {
|
||||
expect(historyService.recordFailure).toHaveBeenCalledWith(expect.objectContaining({ userId: 8, deviceId: 5, event: 'annotations_push' }), error);
|
||||
});
|
||||
|
||||
it('does not record a failure when a pushed content id is not a BookOrbit book', async () => {
|
||||
exchangeService.patchContentAnnotations.mockRejectedValueOnce(new KoboContentNotFoundException('1894971763'));
|
||||
|
||||
await expect(controller.patchAnnotations('1894971763', {}, { id: 8 } as never, { deviceId: 5 } as never)).rejects.toThrow(
|
||||
KoboContentNotFoundException,
|
||||
);
|
||||
|
||||
expect(historyService.recordFailure).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('delegates changed content and storage metadata without history rows', async () => {
|
||||
exchangeService.getChangedContentIds.mockResolvedValue(['book-1']);
|
||||
|
||||
|
||||
@@ -7,7 +7,7 @@ import type { RequestUser } from '../../common/types/request-user';
|
||||
import { KoboDevice } from './decorators/kobo-device.decorator';
|
||||
import type { KoboDeviceContext } from './guards/kobo-token.guard';
|
||||
import { KoboTokenGuard } from './guards/kobo-token.guard';
|
||||
import { KoboAnnotationExchangeService } from './services/kobo-annotation-exchange.service';
|
||||
import { KoboAnnotationExchangeService, KoboContentNotFoundException } from './services/kobo-annotation-exchange.service';
|
||||
import { KoboSyncHistoryService } from './services/kobo-sync-history.service';
|
||||
|
||||
@Controller(['kobo/:deviceToken', ''])
|
||||
@@ -43,15 +43,17 @@ export class KoboReadingServicesController {
|
||||
}),
|
||||
});
|
||||
} catch (error: unknown) {
|
||||
await this.historyService.recordFailure(
|
||||
{
|
||||
userId: user.id,
|
||||
deviceId: device.deviceId,
|
||||
event: 'annotations_pull',
|
||||
durationMs: Date.now() - startedAt,
|
||||
},
|
||||
error,
|
||||
);
|
||||
if (!(error instanceof KoboContentNotFoundException)) {
|
||||
await this.historyService.recordFailure(
|
||||
{
|
||||
userId: user.id,
|
||||
deviceId: device.deviceId,
|
||||
event: 'annotations_pull',
|
||||
durationMs: Date.now() - startedAt,
|
||||
},
|
||||
error,
|
||||
);
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
reply.header('ETag', result.etag);
|
||||
@@ -97,15 +99,17 @@ export class KoboReadingServicesController {
|
||||
}),
|
||||
});
|
||||
} catch (error: unknown) {
|
||||
await this.historyService.recordFailure(
|
||||
{
|
||||
userId: user.id,
|
||||
deviceId: device.deviceId,
|
||||
event: 'annotations_push',
|
||||
durationMs: Date.now() - startedAt,
|
||||
},
|
||||
error,
|
||||
);
|
||||
if (!(error instanceof KoboContentNotFoundException)) {
|
||||
await this.historyService.recordFailure(
|
||||
{
|
||||
userId: user.id,
|
||||
deviceId: device.deviceId,
|
||||
event: 'annotations_push',
|
||||
durationMs: Date.now() - startedAt,
|
||||
},
|
||||
error,
|
||||
);
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -23,6 +23,18 @@ const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-
|
||||
const GET_EVENT = 'kobo.annotation_get';
|
||||
const PATCH_EVENT = 'kobo.annotation_patch';
|
||||
|
||||
/**
|
||||
* Raised when a content id the device asks about is not a BookOrbit book. Kobo devices route every
|
||||
* annotation request through us, including their own store content, so this is expected traffic
|
||||
* rather than a sync failure. The 404 matters: the device reconciles by omission (see the deletion
|
||||
* note in getContentAnnotations), so answering with an empty set would wipe its local annotations.
|
||||
*/
|
||||
export class KoboContentNotFoundException extends NotFoundException {
|
||||
constructor(contentId: string) {
|
||||
super(`Kobo content ${contentId} not found`);
|
||||
}
|
||||
}
|
||||
|
||||
export interface ServedAckPlan {
|
||||
entries: { annotationId: number; version: number; externalKey: string; externalCreatedAt: string | null }[];
|
||||
tombstoneStateIds: number[];
|
||||
@@ -68,7 +80,7 @@ export class KoboAnnotationExchangeService {
|
||||
): Promise<KoboContentAnnotationsResult> {
|
||||
const startedAtMs = Date.now();
|
||||
const bookId = await this.resolveBookIdByContentId(userId, contentId);
|
||||
if (bookId === null) throw new NotFoundException(`Kobo content ${contentId} not found`);
|
||||
if (bookId === null) throw new KoboContentNotFoundException(contentId);
|
||||
|
||||
const kepub = await this.kepubContextService.resolveForBook(userId, bookId);
|
||||
const includeAllOrigins = kepub.settings?.syncBookOrbitAnnotationsToKobo ?? false;
|
||||
@@ -167,7 +179,7 @@ export class KoboAnnotationExchangeService {
|
||||
async patchContentAnnotations(userId: number, contentId: string, body: unknown, deviceId: number): Promise<KoboPatchAnnotationsResult> {
|
||||
const startedAtMs = Date.now();
|
||||
const bookId = await this.resolveBookIdByContentId(userId, contentId);
|
||||
if (bookId === null) throw new NotFoundException(`Kobo content ${contentId} not found`);
|
||||
if (bookId === null) throw new KoboContentNotFoundException(contentId);
|
||||
|
||||
const operations = extractKoboAnnotationOperations(body);
|
||||
if (operations.length === 0) return { bookId, created: 0, updated: 0, unchanged: 0, deleted: 0, kepubReady: false };
|
||||
|
||||
Reference in New Issue
Block a user