mirror of
https://github.com/bookorbit/bookorbit.git
synced 2026-11-03 08:21:39 -06:00
fix(metadata): bound provider ids and surface why a save was rejected
A provider identifier parsed out of an OPF was passed on verbatim, so a Calibre `opf:scheme="AMAZON"` holding a product URL reached a varchar(20) column as a 36-character value. It could not be stored, and the next manual save of that book was rejected for a field the user never typed into. Drop an identifier that cannot fit its column rather than truncating it: an overflowing value is not a long identifier, it is a different kind of value, and its first 20 characters are not an ASIN either. Enforce the same bound where every extractor converges, so no parser can fail a metadata write with a Postgres 22001. Keep the server's description of a rejected save instead of discarding the response body. Only that text names the field at fault, so the editor showed a bare "HTTP 400" and left the reason in the network tab. It stays untranslated, so it is preferred over the catalog message rather than replacing it, and the failure is carried as a structured value so the component still translates everything else. Tie the DTO limits to the column widths they were copied from, and guard the pair with a test, since nothing connected them when they drifted apart. Stand Node's built-in web storage down in the client test run. Node 26 defines localStorage on globalThis, jsdom shares that object, and its getter reads as undefined without --localstorage-file, which failed 229 tests across 28 unrelated suites on CI. Closes #1015
This commit is contained in:
@@ -252,7 +252,18 @@ watch(
|
|||||||
{ immediate: true },
|
{ 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 hasLockedFields = computed(() => lockedFields.value.length > 0)
|
||||||
const hasPendingChanges = computed(() => isDirty.value || locksDirty.value)
|
const hasPendingChanges = computed(() => isDirty.value || locksDirty.value)
|
||||||
const isSeriesLocked = computed(() => isLocked('seriesName') || isLocked('seriesIndex'))
|
const isSeriesLocked = computed(() => isLocked('seriesName') || isLocked('seriesIndex'))
|
||||||
|
|||||||
@@ -409,15 +409,81 @@ describe('useMetadataEditor', () => {
|
|||||||
expect(isDirty.value).toBe(false)
|
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()
|
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()
|
const { load, save, error } = useMetadataEditor()
|
||||||
load(book)
|
load(book)
|
||||||
const result = await save(book.id, [])
|
const result = await save(book.id, [])
|
||||||
|
|
||||||
expect(result).toBeNull()
|
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()
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import { computed, reactive, ref } from 'vue'
|
import { computed, reactive, ref } from 'vue'
|
||||||
import { api } from '@/lib/api'
|
import { api } from '@/lib/api'
|
||||||
|
import { readApiErrorDetail } from '@/lib/api-error'
|
||||||
import {
|
import {
|
||||||
FORMAT_TO_GROUP,
|
FORMAT_TO_GROUP,
|
||||||
type BookCommunityRating,
|
type BookCommunityRating,
|
||||||
@@ -110,9 +111,21 @@ function changedCustomMetadataPayload(
|
|||||||
.map((field) => ({ fieldId: field.fieldId, value: field.value }))
|
.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() {
|
export function useMetadataEditor() {
|
||||||
const saving = ref(false)
|
const saving = ref(false)
|
||||||
const error = ref<string | null>(null)
|
const error = ref<MetadataSaveFailure | null>(null)
|
||||||
|
|
||||||
const form = reactive({
|
const form = reactive({
|
||||||
title: null as string | null,
|
title: null as string | null,
|
||||||
@@ -308,12 +321,15 @@ export function useMetadataEditor() {
|
|||||||
headers: { 'Content-Type': 'application/json' },
|
headers: { 'Content-Type': 'application/json' },
|
||||||
body: JSON.stringify({ metadata, lockedFields }),
|
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)
|
const updated = normalizeSaveResult((await res.json()) as BookDetail | BookMetadataSaveResult)
|
||||||
snapshot.value = submittedSnapshot
|
snapshot.value = submittedSnapshot
|
||||||
return updated
|
return updated
|
||||||
} catch (e) {
|
} catch {
|
||||||
error.value = e instanceof Error ? e.message : 'Failed to save'
|
error.value = { detail: null, status: null }
|
||||||
return null
|
return null
|
||||||
} finally {
|
} finally {
|
||||||
saving.value = false
|
saving.value = false
|
||||||
|
|||||||
@@ -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)
|
||||||
|
})
|
||||||
|
})
|
||||||
@@ -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<string | null> {
|
||||||
|
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
|
||||||
|
}
|
||||||
@@ -3908,6 +3908,8 @@
|
|||||||
"savedButWriteFailed": "Metadata saved, but file write failed",
|
"savedButWriteFailed": "Metadata saved, but file write failed",
|
||||||
"savedWriteSkippedReason": "Metadata saved, file write skipped: {reason}",
|
"savedWriteSkippedReason": "Metadata saved, file write skipped: {reason}",
|
||||||
"savedWriteSkipped": "Metadata saved, file write skipped",
|
"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",
|
"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}}",
|
"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",
|
"noMetadataInFile": "No metadata found in file",
|
||||||
|
|||||||
@@ -1,6 +1,14 @@
|
|||||||
import { config } from '@vue/test-utils'
|
import { config } from '@vue/test-utils'
|
||||||
import { i18n } from '@/i18n'
|
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
|
// Install vue-i18n globally for all component tests so useI18n()/t() resolve real
|
||||||
// English messages (matching existing English text assertions) instead of throwing.
|
// English messages (matching existing English text assertions) instead of throwing.
|
||||||
config.global.plugins = [...(config.global.plugins ?? []), i18n]
|
config.global.plugins = [...(config.global.plugins ?? []), i18n]
|
||||||
|
|||||||
@@ -8,6 +8,12 @@ export default mergeConfig(
|
|||||||
test: {
|
test: {
|
||||||
environment: 'jsdom',
|
environment: 'jsdom',
|
||||||
setupFiles: ['./src/test-setup.ts'],
|
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/**'],
|
exclude: [...configDefaults.exclude, 'e2e/**'],
|
||||||
root: fileURLToPath(new URL('./', import.meta.url)),
|
root: fileURLToPath(new URL('./', import.meta.url)),
|
||||||
coverage: {
|
coverage: {
|
||||||
|
|||||||
@@ -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('');
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -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 `<dc:identifier
|
||||||
|
* opf:scheme="AMAZON">` 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;
|
||||||
|
}
|
||||||
@@ -4,6 +4,8 @@ import { plainToInstance } from 'class-transformer';
|
|||||||
import { validate } from 'class-validator';
|
import { validate } from 'class-validator';
|
||||||
import { MetadataProviderKey } from '@bookorbit/types';
|
import { MetadataProviderKey } from '@bookorbit/types';
|
||||||
|
|
||||||
|
import { PROVIDER_ID_MAX_LENGTHS } from '../../../common/utils/provider-id.utils';
|
||||||
|
|
||||||
import { BulkBookIdsDto } from './bulk-book-ids.dto';
|
import { BulkBookIdsDto } from './bulk-book-ids.dto';
|
||||||
import { BulkQuerySelectionDto } from './bulk-query-selection.dto';
|
import { BulkQuerySelectionDto } from './bulk-query-selection.dto';
|
||||||
import { BulkSelectionDto } from './bulk-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);
|
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 () => {
|
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.
|
// 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 });
|
const zeroed = plainToInstance(UpdateBookMetadataDto, { pageCount: 0 });
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import { IsArray, IsBoolean, IsIn, IsInt, IsNumber, IsOptional, IsString, Matche
|
|||||||
import { Transform, Type } from 'class-transformer';
|
import { Transform, Type } from 'class-transformer';
|
||||||
import { MetadataProviderKey } from '@bookorbit/types';
|
import { MetadataProviderKey } from '@bookorbit/types';
|
||||||
import { MAX_SERIES_TOTAL_BOOKS } from '../../../common/utils/series-total-books.utils';
|
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';
|
import { CustomMetadataValueDto } from '../../custom-metadata/dto/custom-metadata-value.dto';
|
||||||
|
|
||||||
export class AudiobookChapterDto {
|
export class AudiobookChapterDto {
|
||||||
@@ -65,21 +66,21 @@ export class UpdateBookMetadataDto {
|
|||||||
@IsOptional() @IsArray() @IsString({ each: true }) authors?: string[];
|
@IsOptional() @IsArray() @IsString({ each: true }) authors?: string[];
|
||||||
@IsOptional() @IsArray() @IsString({ each: true }) genres?: string[];
|
@IsOptional() @IsArray() @IsString({ each: true }) genres?: string[];
|
||||||
@IsOptional() @IsArray() @IsString({ each: true }) tags?: string[];
|
@IsOptional() @IsArray() @IsString({ each: true }) tags?: string[];
|
||||||
@IsOptional() @IsString() @MaxLength(50) googleBooksId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.googleBooksId) googleBooksId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(50) goodreadsId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.goodreadsId) goodreadsId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(20) amazonId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.amazonId) amazonId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(255) hardcoverId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.hardcoverId) hardcoverId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(50) hardcoverEditionId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.hardcoverEditionId) hardcoverEditionId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(50) openLibraryId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.openLibraryId) openLibraryId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(50) itunesId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.itunesId) itunesId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(20) audibleId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.audibleId) audibleId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(50) librofmId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.librofmId) librofmId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(255) koboId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.koboId) koboId?: string | null;
|
||||||
@IsOptional() @ValidateNested() @Type(() => AudioMetadataDto) audioMetadata?: AudioMetadataDto;
|
@IsOptional() @ValidateNested() @Type(() => AudioMetadataDto) audioMetadata?: AudioMetadataDto;
|
||||||
@IsOptional() @IsString() @MaxLength(50) comicvineId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.comicvineId) comicvineId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(50) ranobedbId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.ranobedbId) ranobedbId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(512) lubimyczytacId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.lubimyczytacId) lubimyczytacId?: string | null;
|
||||||
@IsOptional() @IsString() @MaxLength(20) aladinId?: string | null;
|
@IsOptional() @IsString() @MaxLength(PROVIDER_ID_MAX_LENGTHS.aladinId) aladinId?: string | null;
|
||||||
@IsOptional() @ValidateNested() @Type(() => ComicMetadataDto) comicMetadata?: ComicMetadataDto;
|
@IsOptional() @ValidateNested() @Type(() => ComicMetadataDto) comicMetadata?: ComicMetadataDto;
|
||||||
@IsOptional() @IsArray() @ValidateNested({ each: true }) @Type(() => CustomMetadataValueDto) customMetadata?: CustomMetadataValueDto[];
|
@IsOptional() @IsArray() @ValidateNested({ each: true }) @Type(() => CustomMetadataValueDto) customMetadata?: CustomMetadataValueDto[];
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -450,6 +450,55 @@ describe('parseOpf', () => {
|
|||||||
expect(r.amazonId).toHaveLength(10);
|
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(`<dc:identifier opf:scheme="AMAZON">https://www.amazon.com/dp/0345415000</dc:identifier>`));
|
||||||
|
expect(r.amazonId).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('drops an over-long identifier left by a urn: or Calibre prefix', () => {
|
||||||
|
expect(parseOpf(epub2Opf(`<dc:identifier>urn:amazon:${'A'.repeat(21)}</dc:identifier>`)).amazonId).toBeNull();
|
||||||
|
expect(parseOpf(epub2Opf(`<dc:identifier>asin:${'A'.repeat(21)}</dc:identifier>`)).amazonId).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('keeps an identifier sitting exactly on the column bound', () => {
|
||||||
|
expect(parseOpf(epub2Opf(`<dc:identifier opf:scheme="AMAZON">${'A'.repeat(20)}</dc:identifier>`)).amazonId).toBe('A'.repeat(20));
|
||||||
|
expect(parseOpf(epub2Opf(`<dc:identifier opf:scheme="AMAZON">${'A'.repeat(21)}</dc:identifier>`)).amazonId).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('applies each provider its own bound rather than one shared limit', () => {
|
||||||
|
const wide = 'B'.repeat(60);
|
||||||
|
const r = parseOpf(
|
||||||
|
epub2Opf(`
|
||||||
|
<dc:identifier opf:scheme="GOOGLE">${wide}</dc:identifier>
|
||||||
|
<dc:identifier opf:scheme="HARDCOVER">${wide}</dc:identifier>
|
||||||
|
`),
|
||||||
|
);
|
||||||
|
// 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(`
|
||||||
|
<dc:identifier opf:scheme="AMAZON">https://www.amazon.com/dp/0345415000</dc:identifier>
|
||||||
|
<dc:identifier>urn:amazon:0345415000</dc:identifier>
|
||||||
|
`);
|
||||||
|
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(`
|
||||||
|
<dc:identifier opf:scheme="ISBN">9781635766271</dc:identifier>
|
||||||
|
<dc:identifier opf:scheme="AMAZON">${'A'.repeat(64)}</dc:identifier>
|
||||||
|
`);
|
||||||
|
const r = parseOpf(xml);
|
||||||
|
expect(r.isbn13).toBe('9781635766271');
|
||||||
|
expect(r.amazonId).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
it('normalizes provider URN prefixes case-insensitively', () => {
|
it('normalizes provider URN prefixes case-insensitively', () => {
|
||||||
const xml = epub2Opf(`
|
const xml = epub2Opf(`
|
||||||
<dc:identifier>URN:GOOGLE:CaseSensitiveGoogleId</dc:identifier>
|
<dc:identifier>URN:GOOGLE:CaseSensitiveGoogleId</dc:identifier>
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import { XMLParser } from 'fast-xml-parser';
|
import { XMLParser } from 'fast-xml-parser';
|
||||||
import { parsePublishedDateKey, parsePublishedYear } from '../../../common/utils/published-date.utils';
|
import { parsePublishedDateKey, parsePublishedYear } from '../../../common/utils/published-date.utils';
|
||||||
|
import { boundProviderId, type ProviderIdField } from '../../../common/utils/provider-id.utils';
|
||||||
|
|
||||||
export interface ParsedOpf {
|
export interface ParsedOpf {
|
||||||
title: string | null;
|
title: string | null;
|
||||||
@@ -127,6 +128,20 @@ const PROVIDER_PREFIXES: Record<ProviderKey, readonly string[]> = {
|
|||||||
itunes: ['urn:itunes:', 'itunes:'],
|
itunes: ['urn:itunes:', 'itunes:'],
|
||||||
};
|
};
|
||||||
|
|
||||||
|
const PROVIDER_ID_FIELD: Record<ProviderKey, ProviderIdField> = {
|
||||||
|
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 {
|
function normalizeProviderId(provider: ProviderKey, raw: string): string | null {
|
||||||
const value = raw.trim();
|
const value = raw.trim();
|
||||||
if (!value) return null;
|
if (!value) return null;
|
||||||
@@ -134,11 +149,19 @@ function normalizeProviderId(provider: ProviderKey, raw: string): string | null
|
|||||||
const lower = value.toLowerCase();
|
const lower = value.toLowerCase();
|
||||||
for (const prefix of PROVIDER_PREFIXES[provider]) {
|
for (const prefix of PROVIDER_PREFIXES[provider]) {
|
||||||
if (lower.startsWith(prefix)) {
|
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,
|
// Calibre stores custom-column values in a `calibre:user_metadata` JSON blob keyed by column name,
|
||||||
|
|||||||
@@ -22,6 +22,7 @@ import {
|
|||||||
normalizeMetadataTextKeySql,
|
normalizeMetadataTextKeySql,
|
||||||
} from '../../common/utils/metadata-text-normalize.utils';
|
} from '../../common/utils/metadata-text-normalize.utils';
|
||||||
import { normalizePublishedDate, publishedYearFromDateKey } from '../../common/utils/published-date.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 { SeriesIdentityService } from '../../common/services/series-identity.service';
|
||||||
import { SeriesExpectedCountService } from '../../common/services/series-expected-count.service';
|
import { SeriesExpectedCountService } from '../../common/services/series-expected-count.service';
|
||||||
import { SeriesMembershipService } from '../../common/services/series-membership.service';
|
import { SeriesMembershipService } from '../../common/services/series-membership.service';
|
||||||
@@ -137,8 +138,8 @@ export class MetadataService {
|
|||||||
if (!data) return;
|
if (!data) return;
|
||||||
|
|
||||||
const { dto: filtered } = await this.bookMetadataLockService.filterAutomatedBookUpdate(bookId, {
|
const { dto: filtered } = await this.bookMetadataLockService.filterAutomatedBookUpdate(bookId, {
|
||||||
audibleId: data.audibleId,
|
audibleId: boundProviderId('audibleId', data.audibleId),
|
||||||
librofmId: data.librofmId,
|
librofmId: boundProviderId('librofmId', data.librofmId),
|
||||||
audioMetadata: {
|
audioMetadata: {
|
||||||
narrators: data.narrators,
|
narrators: data.narrators,
|
||||||
chapters: data.chapters && data.chapters.length > 0 ? data.chapters : null,
|
chapters: data.chapters && data.chapters.length > 0 ? data.chapters : null,
|
||||||
@@ -570,8 +571,8 @@ export class MetadataService {
|
|||||||
seriesIndex: data.seriesIndex,
|
seriesIndex: data.seriesIndex,
|
||||||
authors: data.authors.map((author) => author.name),
|
authors: data.authors.map((author) => author.name),
|
||||||
genres: data.genres,
|
genres: data.genres,
|
||||||
audibleId: data.audibleId,
|
audibleId: boundProviderId('audibleId', data.audibleId),
|
||||||
librofmId: data.librofmId,
|
librofmId: boundProviderId('librofmId', data.librofmId),
|
||||||
audioMetadata: {
|
audioMetadata: {
|
||||||
durationSeconds: data.durationSeconds ?? null,
|
durationSeconds: data.durationSeconds ?? null,
|
||||||
chapters: data.chapters && data.chapters.length > 0 ? data.chapters : null,
|
chapters: data.chapters && data.chapters.length > 0 ? data.chapters : null,
|
||||||
@@ -645,18 +646,21 @@ export class MetadataService {
|
|||||||
tags: data.tags,
|
tags: data.tags,
|
||||||
rating: normalizeImportedRating(data.rating),
|
rating: normalizeImportedRating(data.rating),
|
||||||
pageCount: data.pageCount,
|
pageCount: data.pageCount,
|
||||||
googleBooksId: data.googleBooksId,
|
// Every extractor feeds this path, so the column bound is enforced once here rather than
|
||||||
goodreadsId: data.goodreadsId,
|
// trusted to each parser: an identifier that overflows would otherwise fail the write with
|
||||||
amazonId: data.amazonId,
|
// a Postgres 22001 and take the rest of the book's metadata down with it.
|
||||||
hardcoverId: data.hardcoverId,
|
googleBooksId: boundProviderId('googleBooksId', data.googleBooksId),
|
||||||
hardcoverEditionId: data.hardcoverEditionId,
|
goodreadsId: boundProviderId('goodreadsId', data.goodreadsId),
|
||||||
openLibraryId: data.openLibraryId,
|
amazonId: boundProviderId('amazonId', data.amazonId),
|
||||||
ranobedbId: data.ranobedbId,
|
hardcoverId: boundProviderId('hardcoverId', data.hardcoverId),
|
||||||
koboId: data.koboId,
|
hardcoverEditionId: boundProviderId('hardcoverEditionId', data.hardcoverEditionId),
|
||||||
comicvineId: data.comicvineId,
|
openLibraryId: boundProviderId('openLibraryId', data.openLibraryId),
|
||||||
lubimyczytacId: data.lubimyczytacId,
|
ranobedbId: boundProviderId('ranobedbId', data.ranobedbId),
|
||||||
aladinId: data.aladinId,
|
koboId: boundProviderId('koboId', data.koboId),
|
||||||
itunesId: data.itunesId,
|
comicvineId: boundProviderId('comicvineId', data.comicvineId),
|
||||||
|
lubimyczytacId: boundProviderId('lubimyczytacId', data.lubimyczytacId),
|
||||||
|
aladinId: boundProviderId('aladinId', data.aladinId),
|
||||||
|
itunesId: boundProviderId('itunesId', data.itunesId),
|
||||||
comicMetadata: data.comicMetadata ?? undefined,
|
comicMetadata: data.comicMetadata ?? undefined,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user