diff --git a/CLAUDE.md b/CLAUDE.md index cdeae45..00a44ac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -544,10 +544,32 @@ public async Handler(req: IRequest, res: IResponse): Promise { | 200 | Success | | 400 | Bad request / Operation failed | | 403 | Forbidden (auth failed) | +| 404 | `item_not_found` (library routes) | | 409 | Conflict (duplicate) | | 422 | Validation error | | 500 | Server error | +### Error codes (`error`) + +A 4xx that can never succeed as sent carries a stable machine-readable code next to the message: +`{ message, error }` (`src/types/apiError.ts`, and `UploadErrorCode` for `/upload/*`). It's the key the passkey +routes already used, and iOS decodes it into `networkErrorWithCode`. Both apps retry a failed sync task forever, so +the code is what lets them stop and show the failure; a 4xx without one keeps being retried. Throw an `ApiError` +from services and let the controller's `sendLibraryError` answer it. + +| error | HTTP | Meaning | +|---|---|---| +| `not_subscribed` | 400 | `checkSubscription` failed, confirmed by RevenueCat. If RC couldn't be reached, the same 400 goes out without the code | +| `tier_required` | 403 | Subscribed, but not on a tier with this feature, confirmed live by RevenueCat; same rule when RC is unreachable | +| `item_not_found` | 404 | No row, active or deleted, has the uuid (or, without one, the key) the request names. An item the user **deleted** answers success with nothing changed (its intent no longer applies), checked through `LibraryService.confirmDeleted` | +| `uuid_conflict` | 409 | `PUT /`: the uuid already belongs to an item of a different type | +| `invalid_request` | 422 | Failed body validation (`validateBody`) | + +Removals of something already gone answer success: deleting a bookmark, unlinking an external resource, or +`folder_in_out` on a folder that isn't there. A failed DB read on these routes is a 500 (`LibraryLookupError`), never +a "not found" the apps would stop on. The audit log doesn't record `not_subscribed` / `tier_required` rejections: they are +the same stuck task retrying every 5 seconds, and RevenueCat answers the account's state. + ## Logging ```typescript diff --git a/src/__tests__/controllers/LibraryController.test.ts b/src/__tests__/controllers/LibraryController.test.ts index 4ce245f..d900700 100644 --- a/src/__tests__/controllers/LibraryController.test.ts +++ b/src/__tests__/controllers/LibraryController.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach, jest } from '@jest/globals'; import { LibraryController } from '../../controllers/LibraryController'; -import { LibraryLookupError } from '../../services/LibraryService'; +import { ITEM_DELETED, LibraryLookupError } from '../../services/LibraryService'; +import { ApiError, ApiErrorCode } from '../../types/apiError'; import { mockLoggerService } from '../setup'; function makeRes() { @@ -197,3 +198,177 @@ describe('LibraryController.getLastPlayedItem — error mapping mirrors the list expect(res.json).toHaveBeenCalledWith({ lastItemPlayed: null }); }); }); + +// A request naming an item with no active row: deleted → success with nothing +// changed; never existed → 404 `item_not_found`; failed read → 500. +describe('LibraryController — legacy routes naming a missing item', () => { + let libraryService: any; + let libraryDB: any; + let controller: LibraryController; + const uuid = 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa'; + const notFound = new ApiError(ApiErrorCode.ITEM_NOT_FOUND, 404, `Item not found: "${uuid}"`); + + beforeEach(() => { + libraryService = { + confirmDeleted: jest.fn(async () => true), + thumbnailPutRequest: jest.fn(), + renameLibraryObject: jest.fn(), + deleteFolderMoving: jest.fn(), + moveLibraryObjectByUuid: jest.fn(), + putExternalResource: jest.fn(), + deleteExternalResource: jest.fn(), + }; + libraryDB = { + getLibraryByUuid: jest.fn(async () => []), + getLibrary: jest.fn(async () => []), + upsertBookmark: jest.fn(), + deactivateBookmark: jest.fn(), + }; + controller = new LibraryController(libraryService, libraryDB); + (controller as any)._logger = mockLoggerService; + mockLoggerService.log.mockClear(); + }); + + const request = (body: Record) => + ({ body, user: { id_user: 1, email: 'user@example.com' } }) as any; + + describe('PUT /bookmark', () => { + it('answers success, and writes nothing, for a bookmark on a deleted book', async () => { + const res = makeRes(); + await controller.upsertBookmark(request({ uuid, key: 'Book.m4b', time: 120, active: true }), res); + + expect(libraryService.confirmDeleted).toHaveBeenCalledWith(expect.anything(), { uuid, key: 'Book.m4b' }); + expect(res.json).toHaveBeenCalledWith({ bookmark: null }); + expect(libraryDB.upsertBookmark).not.toHaveBeenCalled(); + }); + + it('answers 404 item_not_found for a bookmark on a book that never existed', async () => { + libraryService.confirmDeleted.mockRejectedValue(notFound); + const res = makeRes(); + await controller.upsertBookmark(request({ uuid, key: 'Book.m4b', time: 120, active: true }), res); + + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith({ message: notFound.message, error: 'item_not_found' }); + }); + + it('treats deleting a bookmark on a missing book as done, deleted or not', async () => { + const res = makeRes(); + await controller.upsertBookmark(request({ uuid, key: 'Book.m4b', time: 120, active: false }), res); + + expect(libraryService.confirmDeleted).not.toHaveBeenCalled(); + expect(res.json).toHaveBeenCalledWith({ bookmark: null }); + }); + + it('deletes by deactivating, and answers success for a bookmark the server never had', async () => { + libraryDB.getLibraryByUuid.mockResolvedValue([{ id_library_item: 9, title: 'Book', key: 'Book.m4b' }]); + libraryDB.deactivateBookmark.mockResolvedValue(undefined); + const res = makeRes(); + await controller.upsertBookmark(request({ uuid, key: 'Book.m4b', time: 120, active: false }), res); + + expect(libraryDB.deactivateBookmark).toHaveBeenCalled(); + expect(libraryDB.upsertBookmark).not.toHaveBeenCalled(); + expect(res.json).toHaveBeenCalledWith({ bookmark: null }); + }); + + it('answers 500 when the item lookup fails', async () => { + libraryDB.getLibraryByUuid.mockResolvedValue(null); + const res = makeRes(); + await controller.upsertBookmark(request({ uuid, key: 'Book.m4b', time: 120, active: true }), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ message: 'Internal error' }); + }); + }); + + it('POST /rename answers success for a deleted folder and 404 for one that never existed', async () => { + const res = makeRes(); + await controller.renameLibraryObject(request({ uuid, relativePath: 'Series', newName: 'New' }), res); + expect(res.json).toHaveBeenCalledWith({ content: [] }); + expect(libraryService.renameLibraryObject).not.toHaveBeenCalled(); + + libraryService.confirmDeleted.mockRejectedValue(notFound); + const res404 = makeRes(); + await controller.renameLibraryObject(request({ uuid, relativePath: 'Series', newName: 'New' }), res404); + expect(res404.status).toHaveBeenCalledWith(404); + }); + + describe('DELETE /folder_in_out', () => { + it('treats a missing folder as already removed, deleted or never there', async () => { + const res = makeRes(); + await controller.deleteFolderMoving(request({ uuid, relativePath: 'Series' }), res); + + expect(res.json).toHaveBeenCalledWith({ success: true }); + expect(libraryService.confirmDeleted).not.toHaveBeenCalled(); + expect(libraryService.deleteFolderMoving).not.toHaveBeenCalled(); + }); + + it('answers 500 when the folder lookup fails', async () => { + libraryDB.getLibraryByUuid.mockResolvedValue(null); + const res = makeRes(); + await controller.deleteFolderMoving(request({ uuid, relativePath: 'Series' }), res); + + expect(res.status).toHaveBeenCalledWith(500); + }); + }); + + describe('POST /thumbnail_set', () => { + it('answers a null URL for a deleted book', async () => { + libraryService.thumbnailPutRequest.mockResolvedValue(ITEM_DELETED); + const res = makeRes(); + await controller.itemThumbnailPutRequest(request({ uuid, relativePath: 'Book.m4b', thumbnail_name: 't.jpg' }), res); + + expect(res.json).toHaveBeenCalledWith({ thumbnail_name: 't.jpg', thumbnail_url: null, uploaded: false }); + }); + + it('answers 404 item_not_found for a book that never existed', async () => { + libraryService.thumbnailPutRequest.mockRejectedValue(notFound); + const res = makeRes(); + await controller.itemThumbnailPutRequest(request({ uuid, relativePath: 'Book.m4b', thumbnail_name: 't.jpg' }), res); + + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith({ message: notFound.message, error: 'item_not_found' }); + }); + }); + + describe('external resources', () => { + it('PUT /external answers an empty content for a deleted book, and 404 for one that never existed', async () => { + libraryService.putExternalResource.mockResolvedValueOnce(null); + const res = makeRes(); + await controller.putExternalResource(request({ uuid, providerName: 'jellyfin', providerId: 'p1' }), res); + expect(res.json).toHaveBeenCalledWith({ content: {} }); + + libraryService.putExternalResource.mockRejectedValueOnce(notFound); + const res404 = makeRes(); + await controller.putExternalResource(request({ uuid, providerName: 'jellyfin', providerId: 'p1' }), res404); + expect(res404.status).toHaveBeenCalledWith(404); + }); + + it('DELETE /external answers 500 when the lookup fails', async () => { + libraryService.deleteExternalResource.mockRejectedValue(new LibraryLookupError()); + const res = makeRes(); + await controller.deleteExternalResource(request({ uuid, providerName: 'jellyfin', providerId: 'p1' }), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ message: 'Internal error' }); + }); + }); + + it('POST /move maps a coded error to its status and a failed read to 500', async () => { + libraryService.moveLibraryObjectByUuid.mockRejectedValueOnce(notFound); + const res = makeRes(); + await controller.moveLibraryObject(request({ origin: uuid, destination: '' }), res); + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith({ message: notFound.message, error: 'item_not_found' }); + + libraryService.moveLibraryObjectByUuid.mockRejectedValueOnce(new LibraryLookupError()); + const res500 = makeRes(); + await controller.moveLibraryObject(request({ origin: uuid, destination: '' }), res500); + expect(res500.status).toHaveBeenCalledWith(500); + + libraryService.moveLibraryObjectByUuid.mockRejectedValueOnce(new Error('The destination is invalid')); + const res400 = makeRes(); + await controller.moveLibraryObject(request({ origin: uuid, destination: '' }), res400); + expect(res400.status).toHaveBeenCalledWith(400); + expect(res400.json).toHaveBeenCalledWith({ message: 'The destination is invalid' }); + }); +}); diff --git a/src/__tests__/middlewares/recordSyncOperation.test.ts b/src/__tests__/middlewares/recordSyncOperation.test.ts index b5f4567..c18c2cf 100644 --- a/src/__tests__/middlewares/recordSyncOperation.test.ts +++ b/src/__tests__/middlewares/recordSyncOperation.test.ts @@ -17,6 +17,7 @@ import { jobTypeFor, isProgressOnlyUpdate, extractMessage, + extractErrorCode, sanitizeParams, } from '../../api/middlewares/recordSyncOperation'; import { @@ -131,6 +132,19 @@ describe('recordSyncOperation helpers', () => { }); }); + describe('extractErrorCode', () => { + it('pulls .error from a stringified or object body', () => { + expect(extractErrorCode('{"message":"You are not subscribed","error":"not_subscribed"}')).toBe('not_subscribed'); + expect(extractErrorCode({ message: 'x', error: 'item_not_found' })).toBe('item_not_found'); + }); + + it('returns null without a code, or for a body that is not JSON', () => { + expect(extractErrorCode({ message: 'Invalid key' })).toBeNull(); + expect(extractErrorCode('not json')).toBeNull(); + expect(extractErrorCode(null)).toBeNull(); + }); + }); + describe('sanitizeParams', () => { it('drops note/title for set_bookmark without mutating the original body', () => { const body = { relativePath: 'Book', time: 5, note: 'secret', title: 'chapter' }; @@ -277,6 +291,44 @@ describe('recordSyncOperation middleware', () => { expect(arg.item_uuid).toBe('336453c8-24e3-4298-9e8c-8b41f70ac4e7'); }); + describe('account-level rejections', () => { + const rejected = (id_user: number, path = '/uuids', method = 'POST') => { + const req: any = { method, path, route: { path }, user: { id_user }, body: { items: {} } }; + const res = makeRes(); + recordSyncOperation(req, res, jest.fn()); + res.statusCode = 400; + res.json({ message: 'You are not subscribed', error: 'not_subscribed' }); + res.emitFinish(); + }; + + it('never records not_subscribed or tier_required', () => { + rejected(7); + rejected(8, '/move'); + + const req: any = { method: 'PUT', path: '/', route: { path: '/' }, user: { id_user: 7 }, body: { relativePath: 'Book.m4b' } }; + const res = makeRes(); + recordSyncOperation(req, res, jest.fn()); + res.statusCode = 403; + // Express's res.json goes out through res.send as a JSON string. + res.send('{"message":"Requires one of: pro","error":"tier_required"}'); + res.emitFinish(); + + expect(recordMock()).not.toHaveBeenCalled(); + }); + + it('still records every other error, coded or not', () => { + for (let i = 0; i < 2; i += 1) { + const req: any = { method: 'POST', path: '/move', route: { path: '/move' }, user: { id_user: 7 }, body: { origin: 'a', destination: 'b' } }; + const res = makeRes(); + recordSyncOperation(req, res, jest.fn()); + res.statusCode = 404; + res.json({ message: 'Item not found: "a"', error: 'item_not_found' }); + res.emitFinish(); + } + expect(recordMock()).toHaveBeenCalledTimes(2); + }); + }); + it('does not record reads', () => { const req: any = { method: 'GET', diff --git a/src/__tests__/middlewares/subscription.test.ts b/src/__tests__/middlewares/subscription.test.ts index a328f46..9dd6823 100644 --- a/src/__tests__/middlewares/subscription.test.ts +++ b/src/__tests__/middlewares/subscription.test.ts @@ -61,15 +61,22 @@ describe('checkSubscription middleware', () => { expect(res.status).not.toHaveBeenCalled(); }); - it('returns 400 "not subscribed" when isActive returns false', async () => { - mockIsActive.mockResolvedValue({ active: false, verified: 'local', subscriptions: [] }); + it('returns 400 "not subscribed" with the stop code when RC confirmed it', async () => { + mockIsActive.mockResolvedValue({ active: false, verified: 'rc', subscriptions: [] }); await checkSubscription(req, res, next); expect(mockIsActive).toHaveBeenCalledWith('ext-1'); expect(res.status).toHaveBeenCalledWith(400); - expect(res.json).toHaveBeenCalledWith({ message: 'You are not subscribed' }); + expect(res.json).toHaveBeenCalledWith({ message: 'You are not subscribed', error: 'not_subscribed' }); expect(next).not.toHaveBeenCalled(); }); + it('leaves out the stop code when RC could not be reached', async () => { + mockIsActive.mockResolvedValue({ active: false, verified: 'local', subscriptions: [] }); + await checkSubscription(req, res, next); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith({ message: 'You are not subscribed' }); + }); + it('forwards thrown errors to next()', async () => { const error = new Error('boom'); mockIsActive.mockRejectedValue(error); @@ -154,7 +161,7 @@ describe('requireSubscription middleware', () => { expect(mockFetchLiveEntitlements).toHaveBeenCalledWith('ext-1'); expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ message: 'Requires one of: pro' }); + expect(res.json).toHaveBeenCalledWith({ message: 'Requires one of: pro', error: 'tier_required' }); expect(next).not.toHaveBeenCalled(); }); @@ -165,6 +172,8 @@ describe('requireSubscription middleware', () => { await requireSubscription([SubscriptionTierEnum.PRO])(req, res, next); expect(res.status).toHaveBeenCalledWith(403); + // Not confirmed, so no code telling the apps to stop. + expect(res.json).toHaveBeenCalledWith({ message: 'Requires one of: pro' }); expect(next).not.toHaveBeenCalled(); }); diff --git a/src/__tests__/services/LibraryDBExternalResource.test.ts b/src/__tests__/services/LibraryDBExternalResource.test.ts index 918fba6..7e224dc 100644 --- a/src/__tests__/services/LibraryDBExternalResource.test.ts +++ b/src/__tests__/services/LibraryDBExternalResource.test.ts @@ -204,7 +204,8 @@ describe('LibraryDB — external_resources', () => { trx, ); - expect(deleted).toBeNull(); + // undefined = nothing matched; null is reserved for a failed query. + expect(deleted).toBeUndefined(); }); it('allows re-adding the same resource after a soft delete (partial unique index)', async () => { diff --git a/src/__tests__/services/LibraryServiceExternalResource.test.ts b/src/__tests__/services/LibraryServiceExternalResource.test.ts index 528b949..9cd9c0e 100644 --- a/src/__tests__/services/LibraryServiceExternalResource.test.ts +++ b/src/__tests__/services/LibraryServiceExternalResource.test.ts @@ -141,7 +141,9 @@ describe('LibraryService — external resource flows', () => { expect(row.active).toBe(false); }); - it('throws when the library item is not found', async () => { + // Unlinking asks for "no link", which already holds: success, not an + // error the apps would retry forever. + it('treats a missing library item as already unlinked', async () => { const trx = getTestTransaction(); const user = await createTestUser(trx); @@ -152,10 +154,10 @@ describe('LibraryService — external resource flows', () => { 'id:whatever', 'dropbox', ), - ).rejects.toThrow(); + ).resolves.toBeNull(); }); - it('throws when the resource does not exist on the item', async () => { + it('treats a resource that is not on the item as already unlinked', async () => { const trx = getTestTransaction(); const user = await createTestUser(trx); const uuid = '99999999-9999-9999-9999-999999999999'; @@ -167,7 +169,7 @@ describe('LibraryService — external resource flows', () => { await expect( service.deleteExternalResource(user as any, uuid, 'id:absent', 'dropbox'), - ).rejects.toThrow(); + ).resolves.toBeNull(); }); }); }); diff --git a/src/__tests__/services/LibraryServiceMissingItems.test.ts b/src/__tests__/services/LibraryServiceMissingItems.test.ts new file mode 100644 index 0000000..1b857e8 --- /dev/null +++ b/src/__tests__/services/LibraryServiceMissingItems.test.ts @@ -0,0 +1,268 @@ +import { describe, it, expect, beforeEach, jest } from '@jest/globals'; +import { ITEM_DELETED, LibraryLookupError, LibraryService } from '../../services/LibraryService'; +import { LibraryDB } from '../../services/db/LibraryDB'; +import { ApiError, ApiErrorCode } from '../../types/apiError'; +import { + getTestTransaction, + mockLoggerService, + createTestUser, + createTestLibraryItem, +} from '../setup'; + +/** + * A request that names an item with no active row either targets something + * the user deleted (the intent no longer applies: success, nothing changes) or + * something that never existed on the server (`item_not_found`, which the apps + * stop on and report). A failed read is neither: it's a retryable 500. + */ +describe('LibraryService — requests naming a missing item', () => { + let service: LibraryService; + + const ORIGIN = 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa'; + const FOLDER = 'bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb'; + const NEVER = 'cccccccc-cccc-4ccc-8ccc-cccccccccccc'; + + beforeEach(() => { + service = new LibraryService(); + (service as any).db = getTestTransaction(); + (service as any)._libraryDB.db = getTestTransaction(); + (service as any)._libraryDB._logger = mockLoggerService; + (service as any)._logger = mockLoggerService; + (service as any)._storage = { + moveFile: jest.fn(async () => true), + fileExists: jest.fn(async () => false), + deleteFile: jest.fn(async () => true), + getPresignedUrl: jest.fn(async () => ({ url: 'https://s3.example/put' })), + }; + (service as any)._prefix = { getPrefix: jest.fn(async () => 'test-prefix') }; + mockLoggerService.log.mockClear(); + }); + + const expectNotFound = async (promise: Promise) => { + const err = await promise.then( + () => null, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(ApiError); + expect(err).toMatchObject({ code: ApiErrorCode.ITEM_NOT_FOUND, statusCode: 404 }); + }; + + describe('move by uuid', () => { + it('does nothing when the book was deleted', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Book.m4b', uuid: ORIGIN, active: false }); + + await expect( + service.moveLibraryObjectByUuid(user as any, { origin: ORIGIN, destination: '' }), + ).resolves.toEqual([]); + }); + + it('answers item_not_found for a book that never existed here', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + + await expectNotFound(service.moveLibraryObjectByUuid(user as any, { origin: NEVER, destination: '' })); + }); + + it('leaves the book where it is when its destination folder was deleted', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + const book = await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Book.m4b', uuid: ORIGIN }); + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Series', type: 0, uuid: FOLDER, active: false }); + + await expect( + service.moveLibraryObjectByUuid(user as any, { origin: ORIGIN, destination: FOLDER }), + ).resolves.toEqual([]); + + const after = await trx('library_items').where({ id_library_item: book.id_library_item }).first(); + expect(after.key).toBe('Book.m4b'); + }); + + it('never moves a book to the root because its destination folder is missing', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + const book = await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Series 2/Book.m4b', uuid: ORIGIN }); + + await expectNotFound( + service.moveLibraryObjectByUuid(user as any, { origin: ORIGIN, destination: NEVER }), + ); + + const after = await trx('library_items').where({ id_library_item: book.id_library_item }).first(); + expect(after.key).toBe('Series 2/Book.m4b'); + }); + + it('raises a lookup failure, not a not-found, when the read fails', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + jest.spyOn((service as any)._libraryDB as LibraryDB, 'getLibraryByUuid').mockResolvedValueOnce(null); + + await expect( + service.moveLibraryObjectByUuid(user as any, { origin: ORIGIN, destination: '' }), + ).rejects.toBeInstanceOf(LibraryLookupError); + }); + }); + + it('raises a lookup failure when the deleted-row check itself fails', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + jest.spyOn((service as any)._libraryDB as LibraryDB, 'hasDeletedItem').mockResolvedValueOnce(null); + + await expect(service.confirmDeleted(user as any, { uuid: NEVER })).rejects.toBeInstanceOf(LibraryLookupError); + }); + + it('raises a lookup failure for folder_in_out by path when the read fails', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + jest.spyOn((service as any)._libraryDB as LibraryDB, 'getLibrary').mockResolvedValueOnce(null); + + await expect(service.deleteFolderMoving(user as any, 'Series')).rejects.toBeInstanceOf(LibraryLookupError); + }); + + describe('move by path', () => { + it('does nothing when the book was deleted, and creates no destination folder', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Book.m4b', active: false }); + + await expect( + service.moveLibraryObject(user as any, { origin: 'Book.m4b', destination: 'New Folder' }), + ).resolves.toEqual([]); + + const folder = await trx('library_items').where({ user_id: user.id_user, key: 'New Folder' }).first(); + expect(folder).toBeUndefined(); + }); + + it('answers item_not_found for a book that never existed here', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + + await expectNotFound(service.moveLibraryObject(user as any, { origin: 'Ghost.m4b', destination: '' })); + }); + }); + + describe('external resources', () => { + const resource = { + providerId: 'p1', + providerName: 'jellyfin', + syncStatus: 'stream', + processedFile: false, + lastSyncedAt: 0, + } as any; + + it('links nothing to a deleted book', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Book.m4b', uuid: ORIGIN, active: false }); + + await expect(service.putExternalResource(user as any, ORIGIN, resource)).resolves.toBeNull(); + }); + + it('answers item_not_found when linking to a book that never existed here', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + + await expectNotFound(service.putExternalResource(user as any, NEVER, resource)); + }); + + it('treats unlinking a missing book or a missing link as done', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Book.m4b', uuid: ORIGIN }); + + await expect(service.deleteExternalResource(user as any, NEVER, 'p1', 'jellyfin')).resolves.toBeNull(); + await expect(service.deleteExternalResource(user as any, ORIGIN, 'p1', 'jellyfin')).resolves.toBeNull(); + }); + + it('keeps a failed unlink write retryable instead of reporting it done', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Book.m4b', uuid: ORIGIN }); + jest.spyOn((service as any)._libraryDB as LibraryDB, 'softDeleteExternalResource').mockResolvedValueOnce(null); + + await expect( + service.deleteExternalResource(user as any, ORIGIN, 'p1', 'jellyfin'), + ).rejects.toBeInstanceOf(LibraryLookupError); + }); + }); + + describe('thumbnails', () => { + it('answers ITEM_DELETED for a deleted book, and item_not_found for one that never existed', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Book.m4b', uuid: ORIGIN, active: false }); + + await expect( + service.thumbnailPutRequest(user as any, { relativePath: 'Book.m4b', uuid: ORIGIN, thumbnail_name: 't.jpg' }), + ).resolves.toBe(ITEM_DELETED); + await expectNotFound( + service.thumbnailPutRequest(user as any, { relativePath: 'Ghost.m4b', uuid: NEVER, thumbnail_name: 't.jpg' }), + ); + }); + }); + + it('keeps the uuid conflict on upload as a coded 409', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Folder', type: 0, uuid: ORIGIN }); + + await expect( + service.putObject(user as any, { + relativePath: 'Elsewhere/Book.m4b', + originalFileName: 'Book.m4b', + title: 'Book', + details: 'Author', + currentTime: 0, + duration: 10, + percentCompleted: 0, + isFinished: false, + orderRank: 0, + lastPlayDateTimestamp: 0, + type: 2, + uuid: ORIGIN, + } as any), + ).rejects.toMatchObject({ code: ApiErrorCode.UUID_CONFLICT, statusCode: 409 }); + }); +}); + +describe('LibraryDB — deleted items and bookmark deletes', () => { + let db: LibraryDB; + + beforeEach(() => { + db = new LibraryDB(); + (db as any).db = getTestTransaction(); + (db as any)._logger = mockLoggerService; + }); + + it('finds a soft-deleted row by uuid or key, and ignores active rows', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + const uuid = 'dddddddd-dddd-4ddd-8ddd-dddddddddddd'; + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Gone.m4b', uuid, active: false }); + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Here.m4b' }); + + expect(await db.hasDeletedItem(user.id_user, { uuid })).toBe(true); + expect(await db.hasDeletedItem(user.id_user, { key: 'Gone.m4b' })).toBe(true); + expect(await db.hasDeletedItem(user.id_user, { key: 'Here.m4b' })).toBe(false); + expect(await db.hasDeletedItem(user.id_user, { key: 'Never.m4b' })).toBe(false); + // A folder sent as `Folder/` is the same key. + await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Old Series', type: 0, active: false }); + expect(await db.hasDeletedItem(user.id_user, { key: 'Old Series/' })).toBe(true); + }); + + it('deactivates an existing bookmark and never inserts an unknown one', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx); + const item = await createTestLibraryItem(trx, { user_id: user.id_user, key: 'Book.m4b' }); + await trx('bookmarks').insert({ library_item_id: item.id_library_item, time: 120, note: 'n', active: true }); + + await expect(db.deactivateBookmark({ library_item_id: item.id_library_item, time: 120 })).resolves.toMatchObject({ + time: 120, + active: false, + }); + await expect(db.deactivateBookmark({ library_item_id: item.id_library_item, time: 300 })).resolves.toBeUndefined(); + + const rows = await trx('bookmarks').where({ library_item_id: item.id_library_item }); + expect(rows).toHaveLength(1); + }); +}); diff --git a/src/__tests__/services/SubscriptionService.test.ts b/src/__tests__/services/SubscriptionService.test.ts index 2af9445..47df7b3 100644 --- a/src/__tests__/services/SubscriptionService.test.ts +++ b/src/__tests__/services/SubscriptionService.test.ts @@ -176,6 +176,8 @@ describe('SubscriptionService.isActive', () => { const result = await service.isActive(externalId); expect(result?.active).toBe(false); + // Only the local negative is known: callers must not treat it as confirmed. + expect(result?.verified).toBe('local'); expect(cache.store.has(`sub:v2:${externalId}`)).toBe(false); }); diff --git a/src/api/middlewares/recordSyncOperation.ts b/src/api/middlewares/recordSyncOperation.ts index fd041f3..99653d9 100644 --- a/src/api/middlewares/recordSyncOperation.ts +++ b/src/api/middlewares/recordSyncOperation.ts @@ -3,6 +3,7 @@ import { logger } from '../../services/LoggerService'; import { isValidUUID } from '../../utils'; import { SyncAuditDB } from '../../services/db/SyncAuditDB'; import { SyncOperationJobType } from '../../types/syncOperation'; +import { ApiErrorCode } from '../../types/apiError'; const syncAuditDB = new SyncAuditDB(); @@ -87,6 +88,29 @@ export function extractMessage(payload: unknown): string | null { return typeof message === 'string' ? message.slice(0, 512) : null; } +// The `error` code next to the message, when the response carries one. +export function extractErrorCode(payload: unknown): string | null { + let body: unknown = payload; + if (typeof payload === 'string') { + try { + body = JSON.parse(payload); + } catch { + return null; + } + } + const code = body && typeof body === 'object' ? (body as Record).error : null; + return typeof code === 'string' ? code : null; +} + +// Account-level rejections are not recorded. They come from apps that retry +// the same task every 5 seconds forever (a lapsed subscription whose queue +// never cleared), so each one was a DB write that only bumped a counter, and +// the account's state is RevenueCat's to answer, not this log's. +export const UNRECORDED_ERROR_CODES = new Set([ + ApiErrorCode.NOT_SUBSCRIBED, + ApiErrorCode.TIER_REQUIRED, +]); + // Store the request body for forensics, minus content with no forensic value: // bookmark note/title are user free-text (already persisted in the bookmarks // table), and an oversized body is replaced with a size marker to bound rows. @@ -115,7 +139,8 @@ export function sanitizeParams(jobType: SyncOperationJobType, body: unknown): un * thin wrappers over res.json/res.send (errors go out through res.send in the * global error handler; successes through res.json). * - Logs nothing for reads (routes absent from JOB_TYPE_BY_ROUTE) or for - * playback-only `update`s. + * playback-only `update`s, or for account-level rejections + * (`not_subscribed`, `tier_required`). * - Gated by SYNC_AUDIT_ENABLED=true. */ export const recordSyncOperation = ( @@ -147,6 +172,12 @@ export const recordSyncOperation = ( const status = res.statusCode; const outcome = status >= 200 && status < 400 ? 'applied' : 'error'; + if ( + outcome === 'error' && + UNRECORDED_ERROR_CODES.has(extractErrorCode(res.locals.__syncAuditPayload) ?? '') + ) { + return; + } const body = req.body ?? {}; const rawPath = pickString(body.relativePath) ?? diff --git a/src/api/middlewares/subscription.ts b/src/api/middlewares/subscription.ts index b17cd49..a58d595 100644 --- a/src/api/middlewares/subscription.ts +++ b/src/api/middlewares/subscription.ts @@ -2,6 +2,7 @@ import { IRequest, IResponse, INext } from '../../types/http'; import { SubscriptionService } from '../../services/SubscriptionService'; import { UserDB } from '../../services/db/UserDB'; import { SubscriptionTier } from '../../types/user'; +import { ApiErrorCode } from '../../types/apiError'; const subscriptionService = new SubscriptionService(); const userDB = new UserDB(); @@ -21,7 +22,13 @@ export const checkSubscription = async ( const externalId = user.external_id || (await userDB.getExternalIdByUserId(user.id_user)); const subState = await subscriptionService.isActive(externalId); if (!subState?.active) { - return res.status(400).json({ message: 'You are not subscribed' }); + // The code tells the apps to stop, so it goes only on a negative RC + // confirmed. When RC couldn't be reached the answer is the same 400 + // without it, which the apps keep retrying. + return res.status(400).json({ + message: 'You are not subscribed', + ...(subState?.verified === 'rc' ? { error: ApiErrorCode.NOT_SUBSCRIBED } : {}), + }); } req.user.subscriptions = subState.subscriptions next(); @@ -57,8 +64,11 @@ export const requireSubscription = (allowedTypes: SubscriptionTier[]) => { return; } + // `live` is null when RC couldn't be reached: same 403, but without the + // code that tells the apps to stop. res.status(403).json({ message: `Requires one of: ${allowedTypes.join(', ')}`, + ...(live ? { error: ApiErrorCode.TIER_REQUIRED } : {}), }); }; }; diff --git a/src/controllers/LibraryController.ts b/src/controllers/LibraryController.ts index 8eae2f3..fda1954 100644 --- a/src/controllers/LibraryController.ts +++ b/src/controllers/LibraryController.ts @@ -1,5 +1,5 @@ import { IRequest, IResponse } from '../types/http'; -import { LibraryService } from '../services/LibraryService'; +import { ITEM_DELETED, LibraryLookupError, LibraryService } from '../services/LibraryService'; import { logger } from '../services/LoggerService'; import { LibraryDB } from '../services/db/LibraryDB'; import { Bookmark, LibraryItem } from '../types/user'; @@ -10,6 +10,7 @@ import { } from '../validation/externalResource'; import { MultipartUploadService } from '../services/MultipartUploadService'; import { UploadError } from '../types/multipartUpload'; +import { ApiError } from '../types/apiError'; import { AbortUploadBody, CompleteUploadBody, @@ -182,9 +183,7 @@ export class LibraryController { const content = (await this._libraryService.putObject(user, params)) ?? {}; return res.json({ content }); } catch (err) { - this._logger.log({ origin: 'LibraryController.putLibraryObject', message: err.message, data: { user: req.user, body: req.body } }, 'error'); - res.status(err.statusCode || 400).json({ message: err.message }); - return; + return this.sendLibraryError(res, err, 'LibraryController.putLibraryObject', req); } } @@ -200,9 +199,7 @@ export class LibraryController { const content = (await this._libraryService.putExternalResource(user, uuid, externalResource)) ?? {}; return res.json({ content }); } catch (err) { - this._logger.log({ origin: 'LibraryController.putExternalResource', message: err.message, data: { id_user: req.user?.id_user, uuid: req.body?.uuid } }, 'error'); - res.status(400).json({ message: err.message }); - return; + return this.sendLibraryError(res, err, 'LibraryController.putExternalResource', req); } } @@ -218,9 +215,7 @@ export class LibraryController { const content = (await this._libraryService.deleteExternalResource(user, uuid, providerId, providerName)) ?? {}; return res.json({ content }); } catch (err) { - this._logger.log({ origin: 'LibraryController.deleteExternalResource', message: err.message, data: { id_user: req.user?.id_user, uuid: req.body?.uuid } }, 'error'); - res.status(400).json({ message: err.message }); - return; + return this.sendLibraryError(res, err, 'LibraryController.deleteExternalResource', req); } } @@ -256,9 +251,7 @@ export class LibraryController { : await this._libraryService.moveLibraryObject(user, params); return res.json({ content }); } catch (err) { - this._logger.log({ origin: 'LibraryController.moveLibraryObject', message: err.message, data: { user: req.user, body: req.body } }, 'error'); - res.status(400).json({ message: err.message }); - return; + return this.sendLibraryError(res, err, 'LibraryController.moveLibraryObject', req); } } @@ -274,9 +267,12 @@ export class LibraryController { const items = await this._libraryDB.getLibraryByUuid(user.id_user, uuid, { exactly: true, }); - const item = items?.[0]; + if (items === null) throw new LibraryLookupError(); + const item = items[0]; if (!item) { - throw new Error('Invalid folder'); + // Removing a folder that isn't there is already done, deleted or + // not, as LibraryService.deleteFolderMoving answers for a path. + return res.json({ success: true }); } folderPath = item.key; } else if (!relativePath) { @@ -285,9 +281,7 @@ export class LibraryController { const success = await this._libraryService.deleteFolderMoving(user, folderPath); return res.json({ success }); } catch (err) { - this._logger.log({ origin: 'LibraryController.deleteFolderMoving', message: err.message, data: { user: req.user, body: req.body } }, 'error'); - res.status(400).json({ message: err.message }); - return; + return this.sendLibraryError(res, err, 'LibraryController.deleteFolderMoving', req); } } @@ -334,11 +328,23 @@ export class LibraryController { : await this._libraryDB.getLibrary(user.id_user, bookmark.key, { exactly: true, }); - if (!itemDB || !itemDB[0]) { - throw new Error('Invalid key'); + if (itemDB === null) throw new LibraryLookupError(); + // A delete (`active: false`) asks for "no bookmark": with no item, or no + // such bookmark, that already holds. + const isDelete = bookmark.active === false; + if (!itemDB[0]) { + if (!isDelete) { + await this._libraryService.confirmDeleted(user, { uuid: bookmark.uuid, key: bookmark.key }); + } + return res.json({ bookmark: null }); } bookmark.library_item_id = itemDB[0].id_library_item; - const inserted = await this._libraryDB.upsertBookmark(bookmark); + const inserted = isDelete + ? await this._libraryDB.deactivateBookmark(bookmark) + : await this._libraryDB.upsertBookmark(bookmark); + if (inserted === undefined) { + return res.json({ bookmark: null }); + } if (!inserted) { throw new Error('problem creating the bookmark'); } @@ -350,9 +356,7 @@ export class LibraryController { }, }); } catch (err) { - this._logger.log({ origin: 'LibraryController.upsertBookmark', message: err.message, data: { user: req.user, body: req.body } }, 'error'); - res.status(400).json({ message: err.message }); - return; + return this.sendLibraryError(res, err, 'LibraryController.upsertBookmark', req); } } @@ -372,6 +376,16 @@ export class LibraryController { throw new Error('Invalid parameters'); } const url = await this._libraryService.thumbnailPutRequest(user, thumbnailData); + if (url === ITEM_DELETED) { + // Nothing to set on a deleted item. The apps shipped so far decode + // `thumbnail_url` as a required URL and keep retrying, as they did on + // the old 400; new clients must decode it as optional and stop. + return res.json({ + thumbnail_name: thumbnailData.thumbnail_name, + thumbnail_url: null, + uploaded: false, + }); + } if (!url) { throw new Error('problem creating the request url'); } @@ -381,9 +395,7 @@ export class LibraryController { uploaded: thumbnailData.uploaded && url, }); } catch (err) { - this._logger.log({ origin: 'LibraryController.itemThumbnailPutRequest', message: err.message, data: { user: req.user, body: req.body } }, 'error'); - res.status(400).json({ message: err.message }); - return; + return this.sendLibraryError(res, err, 'LibraryController.itemThumbnailPutRequest', req); } } @@ -403,9 +415,11 @@ export class LibraryController { : await this._libraryDB.getLibrary(user.id_user, cleanPath, { exactly: true, }); + if (objectDB === null) throw new LibraryLookupError(); const itemDb = objectDB[0]; if (!itemDb) { - throw Error('Item not found'); + await this._libraryService.confirmDeleted(user, { uuid, key: cleanPath }); + return res.json({ content: [] }); } const content = await this._libraryService.renameLibraryObject(user, { item: itemDb, @@ -413,9 +427,7 @@ export class LibraryController { }); return res.json({ content }); } catch (err) { - this._logger.log({ origin: 'LibraryController.renameLibraryObject', message: err.message, data: { user: req.user, body: req.body } }, 'error'); - res.status(400).json({ message: err.message }); - return; + return this.sendLibraryError(res, err, 'LibraryController.renameLibraryObject', req); } } @@ -503,6 +515,39 @@ export class LibraryController { } } + /** + * The legacy library routes answer every failure `400 { message }`. Two + * kinds now get their own answer: an ApiError carries the stable `error` + * code the apps stop on, and a failed DB read (LibraryLookupError) is a 500 + * they retry, where it used to read as the item not existing. + */ + private sendLibraryError( + res: IResponse, + err: Error, + origin: string, + req: IRequest, + ): IResponse { + if (err instanceof ApiError) { + this._logger.log( + { origin, message: err.message, data: { id_user: req.user?.id_user, body: req.body, code: err.code } }, + 'warn', + ); + return res.status(err.statusCode).json({ message: err.message, error: err.code }); + } + if (err instanceof LibraryLookupError) { + this._logger.log( + { origin, message: err.message, data: { id_user: req.user?.id_user, body: req.body } }, + 'error', + ); + return res.status(500).json({ message: 'Internal error' }); + } + this._logger.log( + { origin, message: err.message, data: { id_user: req.user?.id_user, body: req.body } }, + 'error', + ); + return res.status(400).json({ message: err.message }); + } + private sendUploadError( res: IResponse, err: Error, diff --git a/src/database/migrations/20260924120000_library_items_uuid_user_inactive.ts b/src/database/migrations/20260924120000_library_items_uuid_user_inactive.ts new file mode 100644 index 0000000..3258abe --- /dev/null +++ b/src/database/migrations/20260924120000_library_items_uuid_user_inactive.ts @@ -0,0 +1,43 @@ +import type { Knex } from 'knex'; + +// CREATE INDEX CONCURRENTLY can't run inside a transaction, and it keeps the +// table writable while the index builds (library_items is ~1.6M rows). +export const config = { transaction: false }; + +// `library_items_uuid_user_unique` only covers active rows, so asking "did this +// user delete the item with this uuid?" was a sequential scan of the table. +// The library routes ask it whenever a request names a uuid that has no active +// row, which old clients repeat every 5 seconds. +export async function up(knex: Knex): Promise { + // A concurrent build that fails (a cancelled deploy, a timeout) leaves an + // INVALID index the planner never uses; IF NOT EXISTS would then skip it for + // good. Drop any leftover and build it fresh. + await knex.raw('DROP INDEX CONCURRENTLY IF EXISTS library_items_uuid_user_inactive'); + await knex.raw(` + CREATE INDEX CONCURRENTLY library_items_uuid_user_inactive + ON library_items (uuid, user_id) + WHERE active = false + `); + + // The same question by key (requests without a uuid) uses the plain key + // index. Production has it but no migration ever created it, so fresh + // databases went without; declare it here. A no-op where it exists. Only an + // INVALID leftover is dropped first: production's valid copy predates this. + const { rows } = await knex.raw(` + SELECT i.indisvalid + FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid + WHERE c.relname = 'library_items_key_index' + `); + if (rows[0] && !rows[0].indisvalid) { + await knex.raw('DROP INDEX CONCURRENTLY library_items_key_index'); + } + await knex.raw(` + CREATE INDEX CONCURRENTLY IF NOT EXISTS library_items_key_index + ON library_items (key) + `); +} + +// Leaves library_items_key_index alone: production had it before this migration. +export async function down(knex: Knex): Promise { + await knex.raw('DROP INDEX CONCURRENTLY IF EXISTS library_items_uuid_user_inactive'); +} diff --git a/src/services/LibraryService.ts b/src/services/LibraryService.ts index 3053971..2af2b6b 100644 --- a/src/services/LibraryService.ts +++ b/src/services/LibraryService.ts @@ -26,6 +26,7 @@ import { } from '../utils'; import { LibraryDB, externalResourceRowToApi } from './db/LibraryDB'; import { StoragePrefixService } from './StoragePrefixService'; +import { ApiError, ApiErrorCode } from '../types/apiError'; /** * A library read that could not be answered because the DB layer failed (it @@ -39,6 +40,9 @@ export class LibraryLookupError extends Error { } } +/** `thumbnailPutRequest`'s answer for an item the user deleted: nothing to set. */ +export const ITEM_DELETED = Symbol('ITEM_DELETED'); + export class LibraryService { private readonly _logger = logger; private db = database; @@ -306,6 +310,31 @@ export class LibraryService { return rows; } + /** + * A request named an item that has no active row. If the user deleted it + * (on another device, say), returns true: the request's intent no longer + * applies, so the caller answers success without changing anything, and the + * client's next sync removes the item locally. Otherwise no row, active or + * deleted, has that uuid (or, without a valid uuid, that key): throws + * `item_not_found`, which the apps stop on and report, instead of retrying + * something that can never succeed as sent. The key is deliberately not a + * fallback for a uuid: an old deleted row at the same path can be a + * different item. + */ + async confirmDeleted( + user: User, + ref: { uuid?: string; key?: string }, + trx?: Knex.Transaction, + ): Promise { + const name = isValidUUID(ref.uuid) ? ref.uuid : ref.key; + if (name) { + const deleted = await this._libraryDB.hasDeletedItem(user.id_user, ref, trx); + if (deleted === null) throw new LibraryLookupError(); + if (deleted) return true; + } + throw new ApiError(ApiErrorCode.ITEM_NOT_FOUND, 404, `Item not found: "${name}"`); + } + async getObject( user: User, path: string, @@ -486,11 +515,10 @@ export class LibraryService { if (parseInt(`${existing.type}`) !== parseInt(`${incoming.type}`)) { // Same uuid on a different item type is corrupted client state — don't // guess at a move; surface it instead of the opaque insert failure. - throw Object.assign( - new Error( - `Upload uuid ${incoming.uuid} belongs to an existing item of a different type at key=${existing.key}`, - ), - { statusCode: 409 }, + throw new ApiError( + ApiErrorCode.UUID_CONFLICT, + 409, + `Upload uuid ${incoming.uuid} belongs to an existing item of a different type at key=${existing.key}`, ); } @@ -699,11 +727,13 @@ export class LibraryService { /// Verify destination folder if not moving to the library if (destinationPathFolder !== '') { - let destinationDB = await this._libraryDB.getLibrary( - user.id_user, - destinationPathFolder, - { exactly: true }, - trx, + let destinationDB = this.requireLookup( + await this._libraryDB.getLibrary( + user.id_user, + destinationPathFolder, + { exactly: true }, + trx, + ), ); if (destinationDB.length === 0) { @@ -743,11 +773,13 @@ export class LibraryService { throw Error('The destination is invalid'); } } - const originObj = await this._libraryDB.getLibrary( - user.id_user, - origin, - { exactly: true }, - trx, + const originObj = this.requireLookup( + await this._libraryDB.getLibrary( + user.id_user, + origin, + { exactly: true }, + trx, + ), ); if (originObj.length !== 1) { // Check if item already exists at destination (already moved) @@ -757,11 +789,13 @@ export class LibraryService { ? originFilename : `${destinationPathFolder}/${originFilename}`; - const destinationObj = await this._libraryDB.getLibrary( - user.id_user, - expectedDestinationPath, - { exactly: true }, - trx, + const destinationObj = this.requireLookup( + await this._libraryDB.getLibrary( + user.id_user, + expectedDestinationPath, + { exactly: true }, + trx, + ), ); if (destinationObj.length === 1) { @@ -770,10 +804,12 @@ export class LibraryService { return []; } - // Item doesn't exist at origin or destination - throw Error( - `Item not found at origin "${origin}" or destination "${expectedDestinationPath}"`, - ); + // Neither at the origin nor at the destination: deleted (nothing to + // move), or it never existed here (throws item_not_found). Roll back: + // the destination folder created above was only for this move. + await this.confirmDeleted(user, { key: origin }, trx); + await trx.rollback(); + return []; } const dbMoved = await this._libraryDB.moveFiles( user.id_user, @@ -793,6 +829,7 @@ export class LibraryService { message: err.message, data: { user, params }, }); + if (err instanceof ApiError || err instanceof LibraryLookupError) throw err; throw Error(err); } } @@ -803,23 +840,36 @@ export class LibraryService { ): Promise { const trx = await this.db.transaction(); try { - const [originDB] = await this._libraryDB.getLibraryByUuid( - user.id_user, - params.origin, - null, - trx, + const [originDB] = this.requireLookup( + await this._libraryDB.getLibraryByUuid( + user.id_user, + params.origin, + null, + trx, + ), ); const [destinationDB] = params.destination - ? await this._libraryDB.getLibraryByUuid( - user.id_user, - params.destination, - null, - trx, + ? this.requireLookup( + await this._libraryDB.getLibraryByUuid( + user.id_user, + params.destination, + null, + trx, + ), ) : [null]; - if (!originDB) { - throw Error(`Item not found: "${params.origin}"`); + // A missing origin or destination folder: deleted, so there is nothing + // to move or nowhere to move it; or never here (throws item_not_found). + // A named destination that is missing must not fall back to the root. + if (!originDB || (params.destination && !destinationDB)) { + await this.confirmDeleted( + user, + { uuid: !originDB ? params.origin : params.destination }, + trx, + ); + await trx.commit(); + return []; } if (destinationDB) { @@ -861,6 +911,7 @@ export class LibraryService { message: err.message, data: { user, params }, }); + if (err instanceof ApiError || err instanceof LibraryLookupError) throw err; throw Error(err); } } @@ -871,14 +922,16 @@ export class LibraryService { const sanitizedFolderPath = sanitizeLibraryPath(folderPath); try { const storagePrefix = await this._prefix.getPrefix(user); - const folderDB = await this._libraryDB.getLibrary( - user.id_user, - sanitizedFolderPath, - { exactly: true }, - trx, + const folderDB = this.requireLookup( + await this._libraryDB.getLibrary( + user.id_user, + sanitizedFolderPath, + { exactly: true }, + trx, + ), ); if (!folderDB[0]) { - // Folder no longer exists + // Folder no longer exists: removing it again is already done await trx.commit(); return true; } @@ -919,19 +972,23 @@ export class LibraryService { message: err.message, data: { user, folderPath }, }); + if (err instanceof LibraryLookupError) throw err; throw Error(err.message); } } - async putExternalResource(user: User, libraryItemUuid: string, externalResource: ExternalResource): Promise { + /** `null` when the item was deleted: there is nothing left to link. */ + async putExternalResource(user: User, libraryItemUuid: string, externalResource: ExternalResource): Promise { const trx = await this.db.transaction(); try { - const [libraryItem] = await this._libraryDB.getLibraryByUuid(user.id_user, libraryItemUuid, null, trx); + const [libraryItem] = this.requireLookup( + await this._libraryDB.getLibraryByUuid(user.id_user, libraryItemUuid, null, trx), + ); if (!libraryItem) { - throw Error( - `Item not found: "${libraryItemUuid}"`, - ); + await this.confirmDeleted(user, { uuid: libraryItemUuid }, trx); + await trx.rollback(); + return null; } const existingExternalResource = await this._libraryDB.getExternalResource(libraryItem.id_library_item, externalResource.providerId, externalResource.providerName, trx) @@ -968,23 +1025,28 @@ export class LibraryService { libraryItemUuid: string, providerId: string, providerName: string, - ): Promise { + ): Promise { const trx = await this.db.transaction(); try { - const [libraryItem] = await this._libraryDB.getLibraryByUuid(user.id_user, libraryItemUuid, null, trx); + const [libraryItem] = this.requireLookup( + await this._libraryDB.getLibraryByUuid(user.id_user, libraryItemUuid, null, trx), + ); + // Unlinking asks for "no link": with no item, or no such link, that + // already holds. Answer success (`null`) instead of an error the apps + // would retry forever. if (!libraryItem) { - throw Error( - `Item not found: "${libraryItemUuid}"`, - ); + await trx.rollback(); + return null; } const deletedRow = await this._libraryDB.softDeleteExternalResource(libraryItem.id_library_item, providerId, providerName, trx); + // A failed write must stay retryable, not read as "already unlinked". + if (deletedRow === null) throw new LibraryLookupError('External resource unlink failed'); if (!deletedRow) { - throw Error( - `ExternalResource not found: "${providerName}/${providerId}"`, - ); + await trx.rollback(); + return null; } await trx.commit(); @@ -1092,20 +1154,23 @@ export class LibraryService { thumbnail_name: string; uploaded?: boolean; }, - ): Promise { + ): Promise { try { const { relativePath, uuid, thumbnail_name, uploaded } = params; const cleanPath = relativePath.replace(`${user.email}/`, ''); - const objectDB = isValidUUID(uuid) - ? await this._libraryDB.getLibraryByUuid(user.id_user, uuid, { - exactly: true, - }) - : await this._libraryDB.getLibrary(user.id_user, cleanPath, { - exactly: true, - }); - const itemDb = objectDB?.[0]; + const objectDB = this.requireLookup( + isValidUUID(uuid) + ? await this._libraryDB.getLibraryByUuid(user.id_user, uuid, { + exactly: true, + }) + : await this._libraryDB.getLibrary(user.id_user, cleanPath, { + exactly: true, + }), + ); + const itemDb = objectDB[0]; if (!itemDb) { - throw new Error('Item not exists'); + await this.confirmDeleted(user, { uuid, key: cleanPath }); + return ITEM_DELETED; } if (uploaded) { const idUpdated = await this._libraryDB.updateThumbnail({ @@ -1126,6 +1191,7 @@ export class LibraryService { message: err.message, data: { user, params }, }); + if (err instanceof ApiError || err instanceof LibraryLookupError) throw err; throw Error(err); } } diff --git a/src/services/SubscriptionService.ts b/src/services/SubscriptionService.ts index 773ddda..25db793 100644 --- a/src/services/SubscriptionService.ts +++ b/src/services/SubscriptionService.ts @@ -214,9 +214,11 @@ export class SubscriptionService { }, 'warn'); } + // 'rc' only when RC answered: an unreachable RC leaves just the local + // negative, which callers must not treat as confirmed. const subState = { active: rcActive, - verified: 'rc', + verified: rcReachable ? 'rc' : 'local', subscriptions: rcEntitlements ?? [] } as SubscriptionState; diff --git a/src/services/db/LibraryDB.ts b/src/services/db/LibraryDB.ts index c5e8fe0..eb57696 100644 --- a/src/services/db/LibraryDB.ts +++ b/src/services/db/LibraryDB.ts @@ -146,6 +146,38 @@ export class LibraryDB { } } + /** + * Whether the user once had this item and deleted it: a soft-deleted row + * with that uuid (or, without one, that key). `null` means the query failed. + * The uuid lookup is served by `library_items_uuid_user_inactive`, the key + * lookup by `library_items_key_index` (both declared in migration + * 20260924120000). + */ + async hasDeletedItem( + user_id: number, + ref: { uuid?: string; key?: string }, + trx?: Knex.Transaction, + ): Promise { + try { + const db = trx || this.db; + const query = db('library_items').where({ user_id, active: false }); + if (isValidUUID(ref.uuid)) { + query.where('uuid', ref.uuid); + } else { + query.where('key', (ref.key ?? '').replace(/\/+$/, '')); + } + const row = await query.first('id_library_item'); + return !!row; + } catch (err) { + this._logger.log({ + origin: 'LibraryDB.hasDeletedItem', + message: err.message, + data: { user_id, ref }, + }); + return null; + } + } + async getItemByThumbnail( user_id: number, thumbnail: string, @@ -769,6 +801,33 @@ export class LibraryDB { } } + /** + * Deletes a bookmark by deactivating its row. Unlike `upsertBookmark`, it + * never inserts: a delete for a bookmark the server never had leaves nothing + * behind (`undefined`), where the upsert's insert would create it active. + * `null` means the query failed. + */ + async deactivateBookmark( + bookmark: Pick, + trx?: Knex.Transaction, + ): Promise { + try { + const db = trx || this.db; + const [row] = await db('bookmarks') + .where({ library_item_id: bookmark.library_item_id, time: bookmark.time }) + .update({ active: false }) + .returning(['note', 'time', 'active']); + return row; + } catch (err) { + this._logger.log({ + origin: 'LibraryDB.deactivateBookmark', + message: err.message, + data: { bookmark }, + }); + return null; + } + } + // Queries for orchestrated (transactional) service methods async updateBySourcePath( @@ -977,12 +1036,13 @@ export class LibraryDB { } } + /** `undefined` when no active link matched; `null` means the query failed. */ async softDeleteExternalResource( libraryItemId: number, providerId: string, providerName: string, trx?: Knex.Transaction, - ): Promise { + ): Promise { try { const db = trx || this.db; const [updatedRow] = await db('external_resources') @@ -994,7 +1054,7 @@ export class LibraryDB { }) .update({ active: false, updated_at: new Date() }) .returning('*'); - return (updatedRow as ExternalResourceDb) || null; + return updatedRow as ExternalResourceDb | undefined; } catch (err) { this._logger.log({ origin: 'LibraryDB.softDeleteExternalResource', diff --git a/src/types/apiError.ts b/src/types/apiError.ts new file mode 100644 index 0000000..5474320 --- /dev/null +++ b/src/types/apiError.ts @@ -0,0 +1,29 @@ +// Machine-readable codes the clients branch on, sent as `error` next to +// `message`, as the passkey routes already do: iOS decodes that key into +// `networkErrorWithCode`. Keep them stable. A code marks a request that can +// never succeed as sent, so the apps stop retrying and show it; anything +// without one is retried as before. +export enum ApiErrorCode { + // The subscription check failed. The apps confirm the lapse against + // RevenueCat before clearing their queues. + NOT_SUBSCRIBED = 'not_subscribed', + // Subscribed, but on a tier without this feature (e.g. LITE asking for S3). + TIER_REQUIRED = 'tier_required', + // No row, active or deleted, has the uuid (or, without one, the key) the + // request names. An item the user deleted answers success instead: the + // request's intent no longer applies. + ITEM_NOT_FOUND = 'item_not_found', + // An upload's uuid already belongs to an item of a different type. + UUID_CONFLICT = 'uuid_conflict', +} + +export class ApiError extends Error { + constructor( + public readonly code: ApiErrorCode, + public readonly statusCode: number, + message: string, + ) { + super(message); + this.name = 'ApiError'; + } +}