diff --git a/client/src/features/book/components/detail/tabs/EditMetadataTab.vue b/client/src/features/book/components/detail/tabs/EditMetadataTab.vue index af69a127e..b632c0e04 100644 --- a/client/src/features/book/components/detail/tabs/EditMetadataTab.vue +++ b/client/src/features/book/components/detail/tabs/EditMetadataTab.vue @@ -252,7 +252,18 @@ watch( { immediate: true }, ) -const combinedError = computed(() => lockError.value ?? error.value) +// The server names the field it rejected ("amazonId must be shorter than..."), which is the only +// text that tells the user what to fix. It is untranslated, so it is preferred over the generic +// catalog message rather than replacing it. +const saveErrorMessage = computed(() => { + const failure = error.value + if (!failure) return null + if (failure.detail) return failure.detail + if (failure.status !== null) return t('book.detail.editMetadata.saveFailedWithStatus', { status: failure.status }) + return t('book.detail.editMetadata.saveFailed') +}) + +const combinedError = computed(() => lockError.value ?? saveErrorMessage.value) const hasLockedFields = computed(() => lockedFields.value.length > 0) const hasPendingChanges = computed(() => isDirty.value || locksDirty.value) const isSeriesLocked = computed(() => isLocked('seriesName') || isLocked('seriesIndex')) diff --git a/client/src/features/book/composables/__tests__/useMetadataEditor.spec.ts b/client/src/features/book/composables/__tests__/useMetadataEditor.spec.ts index 88cee9812..832ea2081 100644 --- a/client/src/features/book/composables/__tests__/useMetadataEditor.spec.ts +++ b/client/src/features/book/composables/__tests__/useMetadataEditor.spec.ts @@ -409,15 +409,81 @@ describe('useMetadataEditor', () => { expect(isDirty.value).toBe(false) }) - it('captures an error message when the save request fails', async () => { + // Regression: a rejected save used to surface as a bare "HTTP 400", so the only text naming the + // field the server refused was left in the response body. See issue #1015. + it('keeps the server validation detail when a save is rejected', async () => { const book = makeBook() - apiMock.mockResolvedValue({ ok: false, status: 400 }) + apiMock.mockResolvedValue({ + ok: false, + status: 400, + json: async () => ({ + statusCode: 400, + message: ['amazonId must be shorter than or equal to 20 characters'], + error: 'Bad Request', + }), + }) const { load, save, error } = useMetadataEditor() load(book) const result = await save(book.id, []) expect(result).toBeNull() - expect(error.value).toBe('HTTP 400') + expect(error.value).toEqual({ detail: 'amazonId must be shorter than or equal to 20 characters', status: 400 }) + }) + + it('reports the status alone when the failure body carries no message', async () => { + const book = makeBook() + apiMock.mockResolvedValue({ ok: false, status: 500, json: async () => ({ statusCode: 500 }) }) + + const { load, save, error } = useMetadataEditor() + load(book) + + expect(await save(book.id, [])).toBeNull() + expect(error.value).toEqual({ detail: null, status: 500 }) + }) + + it('reports neither detail nor status when the request never reached the server', async () => { + const book = makeBook() + apiMock.mockRejectedValue(new Error('Failed to fetch')) + + const { load, save, error } = useMetadataEditor() + load(book) + + expect(await save(book.id, [])).toBeNull() + expect(error.value).toEqual({ detail: null, status: null }) + }) + + it('does not advance the snapshot when a save is rejected, so a retry resends the change', async () => { + const book = makeBook({ title: 'Original Title' }) + apiMock.mockResolvedValue({ ok: false, status: 400, json: async () => ({ message: 'nope' }) }) + + const { form, load, save, isDirty } = useMetadataEditor() + load(book) + form.title = 'Changed Title' + await save(book.id, []) + + expect(isDirty.value).toBe(true) + + apiMock.mockResolvedValue({ ok: true, json: async () => ({ ...book, title: 'Changed Title' }) }) + await save(book.id, []) + + const [, retry] = apiMock.mock.calls[1] as [string, RequestInit] + expect(JSON.parse(String(retry.body)).metadata).toEqual({ title: 'Changed Title' }) + expect(isDirty.value).toBe(false) + }) + + it('clears a previous failure once a save succeeds', async () => { + const book = makeBook() + apiMock.mockResolvedValue({ ok: false, status: 400, json: async () => ({ message: 'nope' }) }) + + const { load, save, error } = useMetadataEditor() + load(book) + await save(book.id, []) + expect(error.value).not.toBeNull() + + apiMock.mockResolvedValue({ ok: true, json: async () => book }) + await save(book.id, []) + + expect(error.value).toBeNull() }) }) diff --git a/client/src/features/book/composables/useMetadataEditor.ts b/client/src/features/book/composables/useMetadataEditor.ts index 408c7bf45..ef48843b5 100644 --- a/client/src/features/book/composables/useMetadataEditor.ts +++ b/client/src/features/book/composables/useMetadataEditor.ts @@ -1,5 +1,6 @@ import { computed, reactive, ref } from 'vue' import { api } from '@/lib/api' +import { readApiErrorDetail } from '@/lib/api-error' import { FORMAT_TO_GROUP, type BookCommunityRating, @@ -110,9 +111,21 @@ function changedCustomMetadataPayload( .map((field) => ({ fieldId: field.fieldId, value: field.value })) } +/** + * A failed save, kept structured rather than pre-rendered: `detail` is the server's own English + * description of what it rejected, which no catalog can translate, while everything else has to + * resolve to a translated message in the component. + */ +export type MetadataSaveFailure = { + /** Server-supplied description, or null when the response carried none. */ + detail: string | null + /** HTTP status, or null when the request failed before producing a response. */ + status: number | null +} + export function useMetadataEditor() { const saving = ref(false) - const error = ref(null) + const error = ref(null) const form = reactive({ title: null as string | null, @@ -308,12 +321,15 @@ export function useMetadataEditor() { headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ metadata, lockedFields }), }) - if (!res.ok) throw new Error(`HTTP ${res.status}`) + if (!res.ok) { + error.value = { detail: await readApiErrorDetail(res), status: res.status } + return null + } const updated = normalizeSaveResult((await res.json()) as BookDetail | BookMetadataSaveResult) snapshot.value = submittedSnapshot return updated - } catch (e) { - error.value = e instanceof Error ? e.message : 'Failed to save' + } catch { + error.value = { detail: null, status: null } return null } finally { saving.value = false diff --git a/client/src/lib/__tests__/api-error.spec.ts b/client/src/lib/__tests__/api-error.spec.ts new file mode 100644 index 000000000..06b750dfb --- /dev/null +++ b/client/src/lib/__tests__/api-error.spec.ts @@ -0,0 +1,67 @@ +// @vitest-environment node +import { describe, expect, it } from 'vitest' +import { readApiErrorDetail } from '../api-error' + +function jsonResponse(body: unknown): Response { + return { json: async () => body } as unknown as Response +} + +describe('readApiErrorDetail', () => { + it('joins the constraint array a ValidationPipe rejection returns', async () => { + const detail = await readApiErrorDetail( + jsonResponse({ + statusCode: 400, + message: ['amazonId must be shorter than or equal to 20 characters', 'goodreadsId must be a string'], + error: 'Bad Request', + }), + ) + + expect(detail).toBe('amazonId must be shorter than or equal to 20 characters, goodreadsId must be a string') + }) + + it('returns the plain string message a thrown HttpException produces', async () => { + expect(await readApiErrorDetail(jsonResponse({ statusCode: 404, message: 'Book 7 not found' }))).toBe('Book 7 not found') + }) + + it('returns null when the body carries no usable message', async () => { + expect(await readApiErrorDetail(jsonResponse({ statusCode: 500 }))).toBeNull() + expect(await readApiErrorDetail(jsonResponse({ message: '' }))).toBeNull() + expect(await readApiErrorDetail(jsonResponse({ message: ' ' }))).toBeNull() + expect(await readApiErrorDetail(jsonResponse({ message: [] }))).toBeNull() + expect(await readApiErrorDetail(jsonResponse({ message: 42 }))).toBeNull() + expect(await readApiErrorDetail(jsonResponse(null))).toBeNull() + expect(await readApiErrorDetail(jsonResponse('plain text'))).toBeNull() + }) + + it('drops non-string entries rather than rendering them as undefined', async () => { + expect(await readApiErrorDetail(jsonResponse({ message: ['title must be a string', null, 7] }))).toBe('title must be a string') + }) + + it('returns null for a body that is not JSON at all', async () => { + const html = { + json: async () => { + throw new SyntaxError('Unexpected token <') + }, + } as unknown as Response + + expect(await readApiErrorDetail(html)).toBeNull() + }) + + it('returns null when the response cannot be read as JSON at all', async () => { + expect(await readApiErrorDetail({} as Response)).toBeNull() + }) + + it('caps a long message so a rejected bulk payload cannot flood the layout', async () => { + const detail = await readApiErrorDetail(jsonResponse({ message: Array.from({ length: 40 }, (_, i) => `field${i} must be a string`) })) + + expect(detail).toHaveLength(303) + expect(detail?.endsWith('...')).toBe(true) + expect(detail?.startsWith('field0 must be a string')).toBe(true) + }) + + it('leaves a message that fits the cap untouched', async () => { + const message = 'x'.repeat(300) + + expect(await readApiErrorDetail(jsonResponse({ message }))).toBe(message) + }) +}) diff --git a/client/src/lib/api-error.ts b/client/src/lib/api-error.ts new file mode 100644 index 000000000..766d1d3ac --- /dev/null +++ b/client/src/lib/api-error.ts @@ -0,0 +1,38 @@ +/** + * A failed response carries the only description of what was actually wrong: the `message` the + * global exception filter serialized. Discarding it leaves the user with a bare status code and no + * way to tell which field the server rejected. + */ + +/** `forbidNonWhitelisted` can name every rejected property at once, and this renders inline. */ +const MAX_DETAIL_LENGTH = 300 + +/** + * Reads the server's own description of a failure. Returns null when the body carried none, so + * callers can fall back to translated copy instead of showing an untranslated status code. + * + * Nest sends a plain string for thrown `HttpException`s and an array of constraint messages for + * `ValidationPipe` rejections; both shapes are flattened here. + */ +export async function readApiErrorDetail(response: Response): Promise { + let body: unknown + try { + body = await response.json() + } catch { + return null + } + + if (typeof body !== 'object' || body === null) return null + const message: unknown = (body as { message?: unknown }).message + + let detail = '' + if (typeof message === 'string') { + detail = message + } else if (Array.isArray(message)) { + detail = message.filter((entry): entry is string => typeof entry === 'string').join(', ') + } + + detail = detail.trim() + if (!detail) return null + return detail.length > MAX_DETAIL_LENGTH ? `${detail.slice(0, MAX_DETAIL_LENGTH)}...` : detail +} diff --git a/client/src/locales/en.json b/client/src/locales/en.json index 1eb5865de..0b9fa580f 100644 --- a/client/src/locales/en.json +++ b/client/src/locales/en.json @@ -3908,6 +3908,8 @@ "savedButWriteFailed": "Metadata saved, but file write failed", "savedWriteSkippedReason": "Metadata saved, file write skipped: {reason}", "savedWriteSkipped": "Metadata saved, file write skipped", + "saveFailed": "Could not save the changes", + "saveFailedWithStatus": "Could not save the changes (HTTP {status})", "loadFromFileFailed": "Failed to load metadata from file", "loadedFieldsFromFile": "{count, plural, =0 {Loaded no fields from file} one {Loaded one field from file} other {Loaded # fields from file}}", "noMetadataInFile": "No metadata found in file", diff --git a/client/src/test-setup.ts b/client/src/test-setup.ts index f89b072f6..91091011c 100644 --- a/client/src/test-setup.ts +++ b/client/src/test-setup.ts @@ -1,6 +1,14 @@ import { config } from '@vue/test-utils' import { i18n } from '@/i18n' +// Fails loudly if `--no-experimental-webstorage` ever stops reaching the workers (see the execArgv +// note in vitest.config.ts). Node's own Web Storage globals shadow jsdom's, and the resulting +// `localStorage` reads as undefined, which otherwise surfaces as hundreds of unrelated "Cannot read +// properties of undefined" failures rather than as the configuration problem it is. +if (typeof window !== 'undefined' && !globalThis.localStorage) { + throw new Error('jsdom localStorage is missing: Node built-in web storage is shadowing it. Check test.execArgv in vitest.config.ts.') +} + // Install vue-i18n globally for all component tests so useI18n()/t() resolve real // English messages (matching existing English text assertions) instead of throwing. config.global.plugins = [...(config.global.plugins ?? []), i18n] diff --git a/client/vitest.config.ts b/client/vitest.config.ts index 79839f0aa..1d47166fa 100644 --- a/client/vitest.config.ts +++ b/client/vitest.config.ts @@ -8,6 +8,12 @@ export default mergeConfig( test: { environment: 'jsdom', setupFiles: ['./src/test-setup.ts'], + // Node 26 defines `localStorage` and `sessionStorage` on globalThis, and jsdom shares that + // object, so its own storage never gets installed. Node's `localStorage` then reads as + // undefined unless the process was started with `--localstorage-file`, and its + // `sessionStorage` is a process-wide store no test file can reset. Standing Node's + // implementation down leaves jsdom's in place, isolated per test file. + execArgv: ['--no-experimental-webstorage'], exclude: [...configDefaults.exclude, 'e2e/**'], root: fileURLToPath(new URL('./', import.meta.url)), coverage: { diff --git a/server/src/common/utils/provider-id.utils.test.ts b/server/src/common/utils/provider-id.utils.test.ts new file mode 100644 index 000000000..793ff2806 --- /dev/null +++ b/server/src/common/utils/provider-id.utils.test.ts @@ -0,0 +1,61 @@ +import { bookMetadata } from '../../db/schema'; +import { boundProviderId, PROVIDER_ID_MAX_LENGTHS, type ProviderIdField } from './provider-id.utils'; + +const FIELDS = Object.keys(PROVIDER_ID_MAX_LENGTHS) as ProviderIdField[]; + +/** `lubimyczytac_id` is a `text` column, so its entry is an API bound with no storage width to match. */ +const UNBOUNDED_COLUMNS: readonly ProviderIdField[] = ['lubimyczytacId']; + +function column(field: ProviderIdField): { columnType: string; length?: number } { + return bookMetadata[field] as unknown as { columnType: string; length?: number }; +} + +describe('provider id bounds', () => { + // This is the guard the original defect needed: the DTO capped amazonId at 20 because the column + // did, and nothing tied the two together. See issue #1015. + it('matches every declared bound to its book_metadata column width', () => { + for (const field of FIELDS) { + if (UNBOUNDED_COLUMNS.includes(field)) continue; + const { columnType, length } = column(field); + expect({ field, columnType, length }).toEqual({ field, columnType: 'PgVarchar', length: PROVIDER_ID_MAX_LENGTHS[field] }); + } + }); + + it('only exempts columns that really are unbounded', () => { + for (const field of UNBOUNDED_COLUMNS) { + expect(column(field).columnType).toBe('PgText'); + } + }); + + it('covers every provider id column the metadata table declares', () => { + const declared = Object.keys(bookMetadata).filter((key) => /(Id)$/.test(key) && key !== 'bookId' && key !== 'seriesId'); + expect(new Set(declared)).toEqual(new Set(FIELDS)); + }); +}); + +describe('boundProviderId', () => { + it('keeps a value that fits, including one sitting exactly on the bound', () => { + expect(boundProviderId('amazonId', 'B0G3YRNY6Y')).toBe('B0G3YRNY6Y'); + expect(boundProviderId('amazonId', 'A'.repeat(20))).toBe('A'.repeat(20)); + }); + + it('drops rather than truncates a value that overflows', () => { + expect(boundProviderId('amazonId', 'A'.repeat(21))).toBeNull(); + expect(boundProviderId('amazonId', 'https://www.amazon.com/dp/0345415000')).toBeNull(); + }); + + it('applies the bound belonging to the named field', () => { + const value = 'C'.repeat(60); + expect(boundProviderId('googleBooksId', value)).toBeNull(); + expect(boundProviderId('hardcoverId', value)).toBe(value); + }); + + it('preserves null and undefined so callers keep cleared distinct from absent', () => { + expect(boundProviderId('amazonId', null)).toBeNull(); + expect(boundProviderId('amazonId', undefined)).toBeUndefined(); + }); + + it('leaves the empty string alone rather than inventing a null', () => { + expect(boundProviderId('amazonId', '')).toBe(''); + }); +}); diff --git a/server/src/common/utils/provider-id.utils.ts b/server/src/common/utils/provider-id.utils.ts new file mode 100644 index 000000000..cc4cc03e9 --- /dev/null +++ b/server/src/common/utils/provider-id.utils.ts @@ -0,0 +1,39 @@ +/** + * Storage bound for each provider identifier, mirroring the `book_metadata` column widths. + * A parsed identifier longer than its column cannot survive: Postgres rejects the write with + * 22001, and the metadata DTO rejects the next manual save with a 400 that names a field the + * user never typed into. Bounding at the point of parse keeps both from happening. + * + * `lubimyczytac_id` is a `text` column, so 512 is the API bound rather than a storage limit. + */ +export const PROVIDER_ID_MAX_LENGTHS = { + googleBooksId: 50, + goodreadsId: 50, + amazonId: 20, + hardcoverId: 255, + hardcoverEditionId: 50, + openLibraryId: 50, + itunesId: 50, + audibleId: 20, + librofmId: 50, + koboId: 255, + comicvineId: 50, + ranobedbId: 50, + lubimyczytacId: 512, + aladinId: 20, +} as const; + +export type ProviderIdField = keyof typeof PROVIDER_ID_MAX_LENGTHS; + +/** + * Drops an identifier that cannot fit its column instead of truncating it. A value that overflows + * is not a long identifier, it is a different kind of value: a Calibre `` holding a product URL is not a 20-character ASIN, and its first 20 + * characters are not one either. Storing nothing beats storing a corrupted prefix. + * + * `undefined` is preserved so callers can keep "field absent" distinct from "field cleared". + */ +export function boundProviderId(field: ProviderIdField, value: string | null | undefined): string | null | undefined { + if (value === null || value === undefined) return value; + return value.length > PROVIDER_ID_MAX_LENGTHS[field] ? null : value; +} diff --git a/server/src/modules/book/dto/book-dto-validation.test.ts b/server/src/modules/book/dto/book-dto-validation.test.ts index e955b1c39..c86f5619b 100644 --- a/server/src/modules/book/dto/book-dto-validation.test.ts +++ b/server/src/modules/book/dto/book-dto-validation.test.ts @@ -4,6 +4,8 @@ import { plainToInstance } from 'class-transformer'; import { validate } from 'class-validator'; import { MetadataProviderKey } from '@bookorbit/types'; +import { PROVIDER_ID_MAX_LENGTHS } from '../../../common/utils/provider-id.utils'; + import { BulkBookIdsDto } from './bulk-book-ids.dto'; import { BulkQuerySelectionDto } from './bulk-query-selection.dto'; import { BulkSelectionDto } from './bulk-selection.dto'; @@ -219,6 +221,22 @@ describe('Book DTO validation', () => { expect((await errorsFor(UpdateBookMetadataDto, { librofmId: 'a'.repeat(51) })).length).toBeGreaterThan(0); }); + it('enforces every provider id bound at exactly its declared length', async () => { + for (const [field, max] of Object.entries(PROVIDER_ID_MAX_LENGTHS)) { + expect({ field, errors: (await errorsFor(UpdateBookMetadataDto, { [field]: 'a'.repeat(max) })).length }).toEqual({ field, errors: 0 }); + expect({ field, errors: (await errorsFor(UpdateBookMetadataDto, { [field]: 'a'.repeat(max + 1) })).length }).toEqual({ field, errors: 1 }); + } + }); + + // The client renders these strings verbatim, because they carry the only description of what the + // server refused. A message that stops naming its field makes the save unfixable. See issue #1015. + it('names the rejected field and its limit in the validation message', async () => { + const [error] = await errorsFor(UpdateBookMetadataDto, { amazonId: 'https://www.amazon.com/dp/0345415000' }); + + expect(error?.property).toBe('amazonId'); + expect(Object.values(error?.constraints ?? {}).join(' ')).toBe('amazonId must be shorter than or equal to 20 characters'); + }); + it('normalizes a zero page count to null and validates other page count values', async () => { // A provider can return a page count of 0 (issue #329); it must normalize to null instead of failing validation. const zeroed = plainToInstance(UpdateBookMetadataDto, { pageCount: 0 }); diff --git a/server/src/modules/book/dto/update-book-metadata.dto.ts b/server/src/modules/book/dto/update-book-metadata.dto.ts index 16cec1721..0fb430e15 100644 --- a/server/src/modules/book/dto/update-book-metadata.dto.ts +++ b/server/src/modules/book/dto/update-book-metadata.dto.ts @@ -2,6 +2,7 @@ import { IsArray, IsBoolean, IsIn, IsInt, IsNumber, IsOptional, IsString, Matche import { Transform, Type } from 'class-transformer'; import { MetadataProviderKey } from '@bookorbit/types'; import { MAX_SERIES_TOTAL_BOOKS } from '../../../common/utils/series-total-books.utils'; +import { PROVIDER_ID_MAX_LENGTHS } from '../../../common/utils/provider-id.utils'; import { CustomMetadataValueDto } from '../../custom-metadata/dto/custom-metadata-value.dto'; export class AudiobookChapterDto { @@ -65,21 +66,21 @@ export class UpdateBookMetadataDto { @IsOptional() @IsArray() @IsString({ each: true }) authors?: string[]; @IsOptional() @IsArray() @IsString({ each: true }) genres?: string[]; @IsOptional() @IsArray() @IsString({ each: true }) tags?: string[]; - @IsOptional() @IsString() @MaxLength(50) googleBooksId?: string | null; - @IsOptional() @IsString() @MaxLength(50) goodreadsId?: string | null; - @IsOptional() @IsString() @MaxLength(20) amazonId?: string | null; - @IsOptional() @IsString() @MaxLength(255) hardcoverId?: string | null; - @IsOptional() @IsString() @MaxLength(50) hardcoverEditionId?: string | null; - @IsOptional() @IsString() @MaxLength(50) openLibraryId?: string | null; - @IsOptional() @IsString() @MaxLength(50) itunesId?: string | null; - @IsOptional() @IsString() @MaxLength(20) audibleId?: string | null; - @IsOptional() @IsString() @MaxLength(50) librofmId?: string | null; - @IsOptional() @IsString() @MaxLength(255) koboId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.googleBooksId) googleBooksId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.goodreadsId) goodreadsId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.amazonId) amazonId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.hardcoverId) hardcoverId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.hardcoverEditionId) hardcoverEditionId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.openLibraryId) openLibraryId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.itunesId) itunesId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.audibleId) audibleId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.librofmId) librofmId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.koboId) koboId?: string | null; @IsOptional() @ValidateNested() @Type(() => AudioMetadataDto) audioMetadata?: AudioMetadataDto; - @IsOptional() @IsString() @MaxLength(50) comicvineId?: string | null; - @IsOptional() @IsString() @MaxLength(50) ranobedbId?: string | null; - @IsOptional() @IsString() @MaxLength(512) lubimyczytacId?: string | null; - @IsOptional() @IsString() @MaxLength(20) aladinId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.comicvineId) comicvineId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.ranobedbId) ranobedbId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.lubimyczytacId) lubimyczytacId?: string | null; + @IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.aladinId) aladinId?: string | null; @IsOptional() @ValidateNested() @Type(() => ComicMetadataDto) comicMetadata?: ComicMetadataDto; @IsOptional() @IsArray() @ValidateNested({ each: true }) @Type(() => CustomMetadataValueDto) customMetadata?: CustomMetadataValueDto[]; } diff --git a/server/src/modules/metadata/lib/opf-parser.test.ts b/server/src/modules/metadata/lib/opf-parser.test.ts index 047fd57b5..2ca09f882 100644 --- a/server/src/modules/metadata/lib/opf-parser.test.ts +++ b/server/src/modules/metadata/lib/opf-parser.test.ts @@ -450,6 +450,55 @@ describe('parseOpf', () => { expect(r.amazonId).toHaveLength(10); }); + // Regression: an identifier wider than its column used to be handed on verbatim, which failed + // the metadata write with a Postgres 22001 and made the next manual save 400 on a field the + // user never touched. See issue #1015. + it('drops a scheme identifier that cannot fit its column instead of truncating it', () => { + const r = parseOpf(epub2Opf(`https://www.amazon.com/dp/0345415000`)); + expect(r.amazonId).toBeNull(); + }); + + it('drops an over-long identifier left by a urn: or Calibre prefix', () => { + expect(parseOpf(epub2Opf(`urn:amazon:${'A'.repeat(21)}`)).amazonId).toBeNull(); + expect(parseOpf(epub2Opf(`asin:${'A'.repeat(21)}`)).amazonId).toBeNull(); + }); + + it('keeps an identifier sitting exactly on the column bound', () => { + expect(parseOpf(epub2Opf(`${'A'.repeat(20)}`)).amazonId).toBe('A'.repeat(20)); + expect(parseOpf(epub2Opf(`${'A'.repeat(21)}`)).amazonId).toBeNull(); + }); + + it('applies each provider its own bound rather than one shared limit', () => { + const wide = 'B'.repeat(60); + const r = parseOpf( + epub2Opf(` + ${wide} + ${wide} + `), + ); + // google_books_id is varchar(50); hardcover_id is varchar(255). + expect(r.googleBooksId).toBeNull(); + expect(r.hardcoverId).toBe(wide); + }); + + it('lets a shorter identifier win when the preferred one is dropped for length', () => { + const xml = epub2Opf(` + https://www.amazon.com/dp/0345415000 + urn:amazon:0345415000 + `); + expect(parseOpf(xml).amazonId).toBe('0345415000'); + }); + + it('does not let an over-long value block the ISBN parsed from the same file', () => { + const xml = epub2Opf(` + 9781635766271 + ${'A'.repeat(64)} + `); + const r = parseOpf(xml); + expect(r.isbn13).toBe('9781635766271'); + expect(r.amazonId).toBeNull(); + }); + it('normalizes provider URN prefixes case-insensitively', () => { const xml = epub2Opf(` URN:GOOGLE:CaseSensitiveGoogleId diff --git a/server/src/modules/metadata/lib/opf-parser.ts b/server/src/modules/metadata/lib/opf-parser.ts index c3440e651..86d656db9 100644 --- a/server/src/modules/metadata/lib/opf-parser.ts +++ b/server/src/modules/metadata/lib/opf-parser.ts @@ -1,5 +1,6 @@ import { XMLParser } from 'fast-xml-parser'; import { parsePublishedDateKey, parsePublishedYear } from '../../../common/utils/published-date.utils'; +import { boundProviderId, type ProviderIdField } from '../../../common/utils/provider-id.utils'; export interface ParsedOpf { title: string | null; @@ -127,6 +128,20 @@ const PROVIDER_PREFIXES: Record = { itunes: ['urn:itunes:', 'itunes:'], }; +const PROVIDER_ID_FIELD: Record = { + google: 'googleBooksId', + amazon: 'amazonId', + goodreads: 'goodreadsId', + hardcover: 'hardcoverId', + hardcoverEdition: 'hardcoverEditionId', + openlibrary: 'openLibraryId', + ranobedb: 'ranobedbId', + kobo: 'koboId', + lubimyczytac: 'lubimyczytacId', + aladin: 'aladinId', + itunes: 'itunesId', +}; + function normalizeProviderId(provider: ProviderKey, raw: string): string | null { const value = raw.trim(); if (!value) return null; @@ -134,11 +149,19 @@ function normalizeProviderId(provider: ProviderKey, raw: string): string | null const lower = value.toLowerCase(); for (const prefix of PROVIDER_PREFIXES[provider]) { if (lower.startsWith(prefix)) { - return value.slice(prefix.length).trim() || null; + return bound(provider, value.slice(prefix.length).trim() || null); } } - return value; + return bound(provider, value); +} + +// An OPF is untrusted input: a scheme attribute only claims what a value is. Anything that cannot +// fit the column is not the identifier it claims to be, so it is dropped here rather than left to +// fail the metadata write or reappear as a 400 the next time the user saves the book by hand. +function bound(provider: ProviderKey, value: string | null): string | null { + if (value === null) return null; + return boundProviderId(PROVIDER_ID_FIELD[provider], value) ?? null; } // Calibre stores custom-column values in a `calibre:user_metadata` JSON blob keyed by column name, diff --git a/server/src/modules/metadata/metadata.service.ts b/server/src/modules/metadata/metadata.service.ts index 8fca1ddfd..d04935f7c 100644 --- a/server/src/modules/metadata/metadata.service.ts +++ b/server/src/modules/metadata/metadata.service.ts @@ -22,6 +22,7 @@ import { normalizeMetadataTextKeySql, } from '../../common/utils/metadata-text-normalize.utils'; import { normalizePublishedDate, publishedYearFromDateKey } from '../../common/utils/published-date.utils'; +import { boundProviderId } from '../../common/utils/provider-id.utils'; import { SeriesIdentityService } from '../../common/services/series-identity.service'; import { SeriesExpectedCountService } from '../../common/services/series-expected-count.service'; import { SeriesMembershipService } from '../../common/services/series-membership.service'; @@ -137,8 +138,8 @@ export class MetadataService { if (!data) return; const { dto: filtered } = await this.bookMetadataLockService.filterAutomatedBookUpdate(bookId, { - audibleId: data.audibleId, - librofmId: data.librofmId, + audibleId: boundProviderId('audibleId', data.audibleId), + librofmId: boundProviderId('librofmId', data.librofmId), audioMetadata: { narrators: data.narrators, chapters: data.chapters && data.chapters.length > 0 ? data.chapters : null, @@ -570,8 +571,8 @@ export class MetadataService { seriesIndex: data.seriesIndex, authors: data.authors.map((author) => author.name), genres: data.genres, - audibleId: data.audibleId, - librofmId: data.librofmId, + audibleId: boundProviderId('audibleId', data.audibleId), + librofmId: boundProviderId('librofmId', data.librofmId), audioMetadata: { durationSeconds: data.durationSeconds ?? null, chapters: data.chapters && data.chapters.length > 0 ? data.chapters : null, @@ -645,18 +646,21 @@ export class MetadataService { tags: data.tags, rating: normalizeImportedRating(data.rating), pageCount: data.pageCount, - googleBooksId: data.googleBooksId, - goodreadsId: data.goodreadsId, - amazonId: data.amazonId, - hardcoverId: data.hardcoverId, - hardcoverEditionId: data.hardcoverEditionId, - openLibraryId: data.openLibraryId, - ranobedbId: data.ranobedbId, - koboId: data.koboId, - comicvineId: data.comicvineId, - lubimyczytacId: data.lubimyczytacId, - aladinId: data.aladinId, - itunesId: data.itunesId, + // Every extractor feeds this path, so the column bound is enforced once here rather than + // trusted to each parser: an identifier that overflows would otherwise fail the write with + // a Postgres 22001 and take the rest of the book's metadata down with it. + googleBooksId: boundProviderId('googleBooksId', data.googleBooksId), + goodreadsId: boundProviderId('goodreadsId', data.goodreadsId), + amazonId: boundProviderId('amazonId', data.amazonId), + hardcoverId: boundProviderId('hardcoverId', data.hardcoverId), + hardcoverEditionId: boundProviderId('hardcoverEditionId', data.hardcoverEditionId), + openLibraryId: boundProviderId('openLibraryId', data.openLibraryId), + ranobedbId: boundProviderId('ranobedbId', data.ranobedbId), + koboId: boundProviderId('koboId', data.koboId), + comicvineId: boundProviderId('comicvineId', data.comicvineId), + lubimyczytacId: boundProviderId('lubimyczytacId', data.lubimyczytacId), + aladinId: boundProviderId('aladinId', data.aladinId), + itunesId: boundProviderId('itunesId', data.itunesId), comicMetadata: data.comicMetadata ?? undefined, });