mirror of
https://github.com/bookorbit/bookorbit.git
synced 2026-11-03 08:21:39 -06:00
fix(metadata): avoid ambiguous ComicVine volume matches
Probe equally ranked comic runs within the existing search budget to prevent reprints from hiding original issues. Require an unambiguous result for automatic matching while preserving manual selection and saved-ID refreshes. Add regression coverage for the reported Generation X match, padded issue numbers, competing runs, provider failures, deadlines, and cache isolation. Fixes #1238
This commit is contained in:
@@ -206,6 +206,71 @@ describe('candidate-relevance', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('ComicVine issue identity', () => {
|
||||
function comic(id: string, title: string | undefined, seriesName = 'Generation X', issueNumber = '1'): MetadataCandidate {
|
||||
return {
|
||||
provider: 'comicvine',
|
||||
providerId: id,
|
||||
title,
|
||||
displayTitle: `${seriesName} #${issueNumber}${title ? ` - ${title}` : ''}`,
|
||||
seriesName,
|
||||
seriesIndex: issueNumber,
|
||||
comicMetadata: { volumeName: seriesName, issueNumber },
|
||||
};
|
||||
}
|
||||
|
||||
it.each(['Generation X #1', 'Generation X #01'])('does not penalize a named original issue for query %s', (title) => {
|
||||
const original = comic('39935', 'Third Genesis');
|
||||
const reprint = comic('325232', undefined);
|
||||
|
||||
expect(filterAndRank([original, reprint], { title })).toEqual([original, reprint]);
|
||||
});
|
||||
|
||||
it('uses stored series and fractional issue context when the title is only an issue name', () => {
|
||||
const match = comic('1', 'The Origin', 'Batman', '12.5');
|
||||
expect(filterAndRank([match], { title: 'The Origin', seriesName: 'Batman', seriesIndex: '12.5' })).toEqual([match]);
|
||||
});
|
||||
|
||||
it('rejects a different issue even when the series name is an exact match', () => {
|
||||
expect(filterAndRank([comic('1', undefined, 'Generation X', '2')], { title: 'Generation X #1' })).toEqual([]);
|
||||
});
|
||||
|
||||
it('ranks exact series ahead of collections without using the issue subtitle', () => {
|
||||
const original = comic('1', 'Third Genesis');
|
||||
const collection = comic('2', undefined, 'Counter X: Generation X: Four Days');
|
||||
expect(filterAndRank([collection, original], { title: 'Generation X #1' })).toEqual([original, collection]);
|
||||
});
|
||||
|
||||
it('honors a typed series over stored series context', () => {
|
||||
const stored = comic('1', 'The Origin', 'Batman');
|
||||
const requested = comic('2', undefined, 'Superman');
|
||||
expect(filterAndRank([stored, requested], { title: 'Superman', titleIsExplicitQuery: true, seriesName: 'Batman', seriesIndex: '1' })).toEqual([
|
||||
requested,
|
||||
]);
|
||||
});
|
||||
|
||||
it('keeps issue zero and a leading article match addressable', () => {
|
||||
const match = comic('1', 'The Beginning', 'The Batman', '0');
|
||||
expect(filterAndRank([match], { title: 'Batman #000' })).toEqual([match]);
|
||||
});
|
||||
|
||||
it.each(['進撃の巨人', 'Élan'])('keeps non-ASCII series identity intact for %s', (seriesName) => {
|
||||
const match = comic('1', 'Issue subtitle', seriesName);
|
||||
expect(filterAndRank([match], { title: `${seriesName} #01` })).toEqual([match]);
|
||||
});
|
||||
|
||||
it('retains author evidence without preferring an unnamed issue', () => {
|
||||
const match = { ...comic('1', 'Third Genesis'), authors: ['Scott Lobdell'] };
|
||||
const other = { ...comic('2', undefined), authors: ['Someone Else'] };
|
||||
expect(filterAndRank([other, match], { title: 'Generation X #1', author: 'Scott Lobdell' })).toEqual([match, other]);
|
||||
});
|
||||
|
||||
it('still matches an issue-name query without stored series context', () => {
|
||||
const match = comic('1', 'Third Genesis');
|
||||
expect(filterAndRank([match], { title: 'Third Genesis' })).toEqual([match]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('scoreTitle', () => {
|
||||
it('gives highest score to exact matches', () => {
|
||||
const params: MetadataSearchParams = { title: 'Foundation' };
|
||||
|
||||
@@ -1,7 +1,8 @@
|
||||
import { MetadataCandidate } from '@bookorbit/types';
|
||||
import { MetadataProviderKey, type MetadataCandidate } from '@bookorbit/types';
|
||||
|
||||
import { candidateHasNormalizedIsbn, normalizeMetadataIsbn } from './isbn-match';
|
||||
import { MetadataSearchParams } from './providers/metadata-search-params';
|
||||
import { scoreComicVineIssue } from './providers/comicvine/comicvine.matching';
|
||||
import { normalizeTitleText, scoreTitleMatch, shareSignificantToken } from '../../common/text-match/title-match';
|
||||
|
||||
const DEFAULT_LIMIT = 5;
|
||||
@@ -56,9 +57,9 @@ export function scoreCandidate(candidate: MetadataCandidate, params: MetadataSea
|
||||
let score = 0;
|
||||
|
||||
const matchable = matchableTitle(candidate);
|
||||
if (params.title && matchable) {
|
||||
score += scoreTitleMatch(params.title, matchable);
|
||||
}
|
||||
const comicScore = candidate.provider === MetadataProviderKey.COMICVINE ? scoreComicVineIssue(candidate, params) : null;
|
||||
if (comicScore !== null) score = comicScore;
|
||||
else if (params.title && matchable) score = scoreTitleMatch(params.title, matchable);
|
||||
|
||||
// When a title was supplied, author remains a boost and cannot rescue an unrelated title.
|
||||
// Without a title, an exact or containing author match is strong enough to stand on its own.
|
||||
|
||||
@@ -1205,6 +1205,11 @@ describe('MetadataFetchPipeline', () => {
|
||||
|
||||
const { resolved } = await pipeline.runWithSources({ title: 'Arkham Asylum #12' }, {});
|
||||
|
||||
expect(fetchService.searchCandidates).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ requireUnambiguousMatch: true }),
|
||||
expect.arrayContaining([MetadataProviderKey.COMICVINE]),
|
||||
);
|
||||
|
||||
expect(resolved.comicMetadata).toEqual({
|
||||
issueNumber: '12',
|
||||
volumeName: 'Arkham Asylum',
|
||||
@@ -1212,6 +1217,33 @@ describe('MetadataFetchPipeline', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('requires unambiguous ComicVine matches even when callers request one candidate or disable the safeguard', async () => {
|
||||
const prefs = createPreferences((fields) => {
|
||||
for (const field of ALL_METADATA_FIELDS) fields[field].providers = [MetadataProviderKey.COMICVINE];
|
||||
});
|
||||
preferencesService.getGlobal.mockResolvedValue(prefs);
|
||||
resolver.resolve.mockReturnValue(prefs);
|
||||
resolver.withForwardCompatibility.mockReturnValue(prefs);
|
||||
registry.all.mockReturnValue([{ key: MetadataProviderKey.COMICVINE }] as never);
|
||||
fetchService.searchCandidates.mockReturnValue(of());
|
||||
|
||||
const { resolved, providerIds } = await pipeline.runWithSources(
|
||||
{
|
||||
title: 'Generation X #01',
|
||||
maxCandidatesPerProvider: 1,
|
||||
requireUnambiguousMatch: false,
|
||||
},
|
||||
{},
|
||||
);
|
||||
|
||||
expect(fetchService.searchCandidates).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ requireUnambiguousMatch: true, maxCandidatesPerProvider: 1 }),
|
||||
[MetadataProviderKey.COMICVINE],
|
||||
);
|
||||
expect(resolved).toEqual({});
|
||||
expect(providerIds).toEqual({});
|
||||
});
|
||||
|
||||
it('merges genres from selected providers when genre merge mode is enabled', async () => {
|
||||
const prefs = createPreferences((fields) => {
|
||||
fields.genres = {
|
||||
|
||||
@@ -220,7 +220,11 @@ export class MetadataFetchPipeline {
|
||||
const searchProviders = [...providerSelection.activeProviders, ...providerSelection.throttledProviders];
|
||||
const searchOptions = this.searchOptions(options?.cacheDirective, providerSelection.throttledProviders);
|
||||
const coverOnlyProviders = this.coverOnlyProviders(preferences, scope.media, searchProviders);
|
||||
const providerSearchParams = existingProviderIdsOnly ? { ...params, existingProviderIdsOnly: true } : params;
|
||||
const providerSearchParams = {
|
||||
...params,
|
||||
...(searchProviders.includes(MetadataProviderKey.COMICVINE) ? { requireUnambiguousMatch: true } : {}),
|
||||
...(existingProviderIdsOnly ? { existingProviderIdsOnly: true } : {}),
|
||||
};
|
||||
const searchParams = searchProviders.some((provider) => this.registry.servesOnlyAudiobooks(provider))
|
||||
? { ...providerSearchParams, includeAudiobookProviders: true }
|
||||
: providerSearchParams;
|
||||
|
||||
@@ -1,8 +1,40 @@
|
||||
import { parseSeriesIndex, type MetadataCandidate } from '@bookorbit/types';
|
||||
|
||||
import { normalizeTitleText, scoreTitleMatch } from '../../../../common/text-match/title-match';
|
||||
import type { MetadataSearchParams } from '../metadata-search-params';
|
||||
import { ComicVineVolume } from './comicvine.types';
|
||||
|
||||
const LEADING_ARTICLE = /^(?:the|a|an)\s+/;
|
||||
const NON_ALPHANUMERIC = /[^a-z0-9]+/g;
|
||||
const LEADING_ZEROS = /^0+(?=\d)/;
|
||||
const ISSUE_PATTERN = /^(.*?)\s*#(\d[\d.]*)(.*)$/;
|
||||
|
||||
interface ParsedIssueTitle {
|
||||
seriesName: string;
|
||||
issueNumber: string;
|
||||
}
|
||||
|
||||
/** An explicit issue target wins; a typed series overrides the stored series but keeps its issue. */
|
||||
export function resolveComicVineIssueQuery(params: MetadataSearchParams): ParsedIssueTitle | null {
|
||||
const match = params.title ? ISSUE_PATTERN.exec(params.title.trim()) : null;
|
||||
if (match?.[1].trim()) return { seriesName: match[1].trim(), issueNumber: normalizeIssueNumber(match[2]) };
|
||||
|
||||
const issueNumber = parseSeriesIndex(params.seriesIndex);
|
||||
const typedSeriesName = params.titleIsExplicitQuery ? params.title?.trim() : undefined;
|
||||
if (typedSeriesName) return issueNumber ? { seriesName: typedSeriesName, issueNumber: normalizeIssueNumber(issueNumber) } : null;
|
||||
|
||||
const seriesName = params.seriesName?.trim();
|
||||
return seriesName && issueNumber ? { seriesName, issueNumber: normalizeIssueNumber(issueNumber) } : null;
|
||||
}
|
||||
|
||||
/** An issue subtitle is not evidence that a different run is a better series/number match. */
|
||||
export function scoreComicVineIssue(candidate: MetadataCandidate, params: MetadataSearchParams): number | null {
|
||||
const query = resolveComicVineIssueQuery(params);
|
||||
const seriesName = candidate.comicMetadata?.volumeName ?? candidate.seriesName;
|
||||
const issueNumber = candidate.comicMetadata?.issueNumber ?? parseSeriesIndex(candidate.seriesIndex);
|
||||
if (!query || !seriesName || !issueNumber) return null;
|
||||
if (normalizeIssueNumber(issueNumber) !== query.issueNumber) return 0;
|
||||
return scoreTitleMatch(normalizeName(query.seriesName), normalizeName(seriesName));
|
||||
}
|
||||
|
||||
/**
|
||||
* ComicVine stores issue numbers unpadded, so a padded number lifted from a filename
|
||||
@@ -14,7 +46,7 @@ export function normalizeIssueNumber(raw: string): string {
|
||||
}
|
||||
|
||||
function normalizeName(name: string): string {
|
||||
return name.toLowerCase().replace(LEADING_ARTICLE, '').replace(NON_ALPHANUMERIC, ' ').trim();
|
||||
return normalizeTitleText(name).replace(LEADING_ARTICLE, '');
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -48,7 +80,7 @@ function startYear(volume: ComicVineVolume): number {
|
||||
return Number.isFinite(year) ? year : 0;
|
||||
}
|
||||
|
||||
function score(volume: ComicVineVolume, seriesName: string, issueNumber: string): number {
|
||||
export function volumeMatchScore(volume: ComicVineVolume, seriesName: string, issueNumber: string): number {
|
||||
return nameScore(volume.name, seriesName) * 2 + (holdsIssuePlausibly(volume, issueNumber) ? 1 : 0);
|
||||
}
|
||||
|
||||
@@ -58,7 +90,7 @@ function score(volume: ComicVineVolume, seriesName: string, issueNumber: string)
|
||||
*/
|
||||
export function rankVolumes(volumes: ComicVineVolume[], seriesName: string, issueNumber: string): ComicVineVolume[] {
|
||||
return volumes
|
||||
.map((volume, index) => ({ volume, index, score: score(volume, seriesName, issueNumber) }))
|
||||
.map((volume, index) => ({ volume, index, score: volumeMatchScore(volume, seriesName, issueNumber) }))
|
||||
.sort((a, b) => b.score - a.score || startYear(b.volume) - startYear(a.volume) || a.index - b.index)
|
||||
.map((entry) => entry.volume);
|
||||
}
|
||||
|
||||
@@ -130,6 +130,129 @@ describe('ComicVineProvider', () => {
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('search - structured path', () => {
|
||||
function sameNamedVolumes(count: number) {
|
||||
return Array.from({ length: count }, (_, index) => ({ ...mockVolume, id: index + 1, start_year: String(2020 - index) }));
|
||||
}
|
||||
|
||||
function mockIssuesForVolumes(volumeIds: number[]) {
|
||||
vi.mocked(client.searchIssuesInVolume).mockImplementation((volumeId) =>
|
||||
Promise.resolve(volumeIds.includes(volumeId) ? [{ ...mockIssue, id: 100 + volumeId, volume: { id: volumeId, name: mockVolume.name } }] : []),
|
||||
);
|
||||
}
|
||||
|
||||
it('offers tied runs across batch boundaries instead of stopping after the first hit', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue(sameNamedVolumes(4));
|
||||
mockIssuesForVolumes([1, 4]);
|
||||
|
||||
const results = await provider.search({ title: 'Batman #01' });
|
||||
|
||||
expect(client.searchIssuesInVolume).toHaveBeenCalledTimes(4);
|
||||
expect(results.map((candidate) => candidate.providerId)).toEqual(['101', '104']);
|
||||
});
|
||||
|
||||
it('does not probe or return lower-ranked collections once matching runs were found', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue([
|
||||
...sameNamedVolumes(2),
|
||||
{ ...mockVolume, id: 3, name: 'Batman Collection', start_year: '2025' },
|
||||
]);
|
||||
mockIssuesForVolumes([1, 2, 3]);
|
||||
|
||||
const results = await provider.search({ title: 'Batman #1' });
|
||||
|
||||
expect(client.searchIssuesInVolume).toHaveBeenCalledTimes(2);
|
||||
expect(results.map((candidate) => candidate.providerId)).toEqual(['101', '102']);
|
||||
});
|
||||
|
||||
it('continues to lower-ranked volumes when the better-ranked volumes have no issue', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue([
|
||||
...sameNamedVolumes(2),
|
||||
{ ...mockVolume, id: 3, name: 'Batman Collection', start_year: '2025' },
|
||||
]);
|
||||
mockIssuesForVolumes([3]);
|
||||
|
||||
expect((await provider.search({ title: 'Batman #1' })).map((candidate) => candidate.providerId)).toEqual(['103']);
|
||||
expect(client.searchIssuesInVolume).toHaveBeenCalledTimes(3);
|
||||
});
|
||||
|
||||
it('does not let a one-result limit hide ambiguity during automatic matching', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue(sameNamedVolumes(4));
|
||||
mockIssuesForVolumes([1, 4]);
|
||||
|
||||
expect(await provider.search({ title: 'Batman #1', maxCandidatesPerProvider: 1, requireUnambiguousMatch: true })).toEqual([]);
|
||||
expect(client.searchIssuesInVolume).toHaveBeenCalledTimes(4);
|
||||
});
|
||||
|
||||
it('accepts a unique issue after checking every tied run', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue(sameNamedVolumes(4));
|
||||
mockIssuesForVolumes([4]);
|
||||
|
||||
const results = await provider.search({ title: 'Batman #1', maxCandidatesPerProvider: 1, requireUnambiguousMatch: true });
|
||||
|
||||
expect(results.map((candidate) => candidate.providerId)).toEqual(['104']);
|
||||
expect(client.searchIssuesInVolume).toHaveBeenCalledTimes(4);
|
||||
});
|
||||
|
||||
it('preserves manual result limits after probing tied volumes', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue(sameNamedVolumes(4));
|
||||
mockIssuesForVolumes([1, 4]);
|
||||
|
||||
expect(await provider.search({ title: 'Batman #1', maxCandidatesPerProvider: 1 })).toHaveLength(1);
|
||||
expect(client.searchIssuesInVolume).toHaveBeenCalledTimes(4);
|
||||
});
|
||||
|
||||
it.each([true, false])('bounds probing to eight volumes when automatic matching is %s', async (automatic) => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue(sameNamedVolumes(20));
|
||||
mockIssuesForVolumes([1]);
|
||||
|
||||
const results = await provider.search({ title: 'Batman #1', requireUnambiguousMatch: automatic });
|
||||
|
||||
expect(client.searchIssuesInVolume).toHaveBeenCalledTimes(8);
|
||||
expect(results).toHaveLength(automatic ? 0 : 1);
|
||||
});
|
||||
|
||||
it('does not accept an automatic match when a tied probe fails', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue(sameNamedVolumes(2));
|
||||
vi.mocked(client.searchIssuesInVolume).mockResolvedValueOnce([mockIssue]).mockRejectedValueOnce(new Error('upstream unavailable'));
|
||||
|
||||
expect(await provider.search({ title: 'Batman #1', requireUnambiguousMatch: true })).toEqual([]);
|
||||
});
|
||||
|
||||
it('does not accept a lower-ranked automatic match after a higher-ranked probe fails', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue([mockVolume, { ...mockVolume, id: 2, name: 'Batman Collection' }]);
|
||||
vi.mocked(client.searchIssuesInVolume).mockRejectedValueOnce(new Error('upstream unavailable')).mockResolvedValueOnce([mockIssue]);
|
||||
|
||||
expect(await provider.search({ title: 'Batman #1', requireUnambiguousMatch: true })).toEqual([]);
|
||||
});
|
||||
|
||||
it('does not accept an automatic match after a tied probe is throttled', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue(sameNamedVolumes(4));
|
||||
vi.mocked(client.searchIssuesInVolume)
|
||||
.mockResolvedValueOnce([mockIssue])
|
||||
.mockRejectedValueOnce(new ProviderThrottleError())
|
||||
.mockResolvedValueOnce([]);
|
||||
|
||||
expect(await provider.search({ title: 'Batman #1', requireUnambiguousMatch: true })).toEqual([]);
|
||||
expect(client.searchIssuesInVolume).toHaveBeenCalledTimes(3);
|
||||
expect(throttleTracker.record).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('deduplicates issue IDs before deciding whether a match is ambiguous', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue([mockVolume]);
|
||||
vi.mocked(client.searchIssuesInVolume).mockResolvedValue([mockIssue, mockIssue]);
|
||||
|
||||
expect(await provider.search({ title: 'Batman #1', requireUnambiguousMatch: true })).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('skips issue detail enrichment when an automatic search is ambiguous', async () => {
|
||||
vi.mocked(client.searchVolumes).mockResolvedValue(sameNamedVolumes(2));
|
||||
vi.mocked(client.searchIssuesInVolume).mockImplementation((volumeId) =>
|
||||
Promise.resolve([{ ...mockIssue, id: 100 + volumeId, volume: { id: volumeId, name: 'Batman' }, person_credits: [] }]),
|
||||
);
|
||||
|
||||
expect(await provider.search({ title: 'Batman #1', requireUnambiguousMatch: true })).toEqual([]);
|
||||
expect(client.getIssueById).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('calls searchVolumes then searchIssuesInVolume for a parsed title', async () => {
|
||||
vi.spyOn(client, 'searchVolumes').mockResolvedValue([mockVolume]);
|
||||
vi.spyOn(client, 'searchIssuesInVolume').mockResolvedValue([mockIssue]);
|
||||
|
||||
@@ -1,8 +1,9 @@
|
||||
import { Injectable, Logger } from '@nestjs/common';
|
||||
import { MetadataCandidate, MetadataProviderKey, parseSeriesIndex } from '@bookorbit/types';
|
||||
import { MetadataCandidate, MetadataProviderKey } from '@bookorbit/types';
|
||||
|
||||
import { ProviderConfigService } from '../../../metadata-preferences/provider-config.service';
|
||||
import { markProviderDegraded } from '../../../../common/provider-cache/provider-call-context';
|
||||
import { sanitizeLogValue } from '../../../../common/utils/log-sanitize.utils';
|
||||
import { currentProviderCallRecord, markProviderDegraded, markProviderTruncated } from '../../../../common/provider-cache/provider-call-context';
|
||||
import type { ProviderCacheSpec } from '../../../../common/provider-cache/provider-cache-spec';
|
||||
import { ProviderThrottleTracker } from '../../provider-throttle.tracker';
|
||||
import { ProviderThrottleError } from '../../provider-throttle.error';
|
||||
@@ -13,22 +14,15 @@ import { PROVIDER_BUDGETS_MS, PROVIDER_LIMITS } from '../provider-constants';
|
||||
import { createSearchDeadline, normalizeMaxCandidates, SearchDeadline } from '../provider-utils';
|
||||
import { ComicVineClient } from './comicvine.client';
|
||||
import { mapIssueToCandidate } from './comicvine.mapper';
|
||||
import { normalizeIssueNumber, rankVolumes } from './comicvine.matching';
|
||||
import { rankVolumes, resolveComicVineIssueQuery, volumeMatchScore } from './comicvine.matching';
|
||||
import { ComicVineIssue, ComicVineVolume } from './comicvine.types';
|
||||
|
||||
const ISSUE_PATTERN = /^(.*?)\s*#(\d[\d.]*)(.*)$/;
|
||||
|
||||
// Every ComicVine call costs a rate-limiter slot, and the volume issue list is slow enough that
|
||||
// probing candidates one at a time exhausts the provider budget before reaching the right run.
|
||||
// Probing a wave at a time keeps recall across all VOLUME_PROBE_LIMIT candidates affordable.
|
||||
const VOLUME_PROBE_LIMIT = 8;
|
||||
const VOLUME_PROBE_BATCH = 3;
|
||||
|
||||
interface ParsedIssueTitle {
|
||||
seriesName: string;
|
||||
issueNumber: string;
|
||||
}
|
||||
|
||||
interface VolumeIssueMatch {
|
||||
issue: ComicVineIssue;
|
||||
volume?: ComicVineVolume;
|
||||
@@ -39,41 +33,6 @@ interface VolumeEnrichmentResult {
|
||||
stopped: boolean;
|
||||
}
|
||||
|
||||
function parseIssueTitle(title: string): ParsedIssueTitle | null {
|
||||
const match = ISSUE_PATTERN.exec(title.trim());
|
||||
if (!match) return null;
|
||||
const seriesName = match[1].trim();
|
||||
const issueNumber = normalizeIssueNumber(match[2]);
|
||||
if (!seriesName || !issueNumber) return null;
|
||||
return { seriesName, issueNumber };
|
||||
}
|
||||
|
||||
function storedIssueNumber(params: MetadataSearchParams): string | null {
|
||||
return parseSeriesIndex(params.seriesIndex);
|
||||
}
|
||||
|
||||
/**
|
||||
* Prefer a "<series> #<issue>" title because that is the user naming an explicit target. Otherwise
|
||||
* the stored series leads, since a comic title holds only the issue name and without that fallback
|
||||
* an already-matched comic would re-search through the far looser general issue search.
|
||||
*
|
||||
* A typed query that is not the book's own title overrides the stored series name even so, or
|
||||
* searching a comic for a different series would silently return the series it is already filed
|
||||
* under. The stored issue number still applies: it is the one thing a bare series query omits.
|
||||
*/
|
||||
function resolveIssueQuery(params: MetadataSearchParams): ParsedIssueTitle | null {
|
||||
const fromTitle = params.title ? parseIssueTitle(params.title) : null;
|
||||
if (fromTitle) return fromTitle;
|
||||
|
||||
const issueNumber = storedIssueNumber(params);
|
||||
const typedSeriesName = params.titleIsExplicitQuery ? params.title?.trim() : undefined;
|
||||
if (typedSeriesName) return issueNumber ? { seriesName: typedSeriesName, issueNumber } : null;
|
||||
|
||||
const seriesName = params.seriesName?.trim();
|
||||
if (!seriesName || !issueNumber) return null;
|
||||
return { seriesName, issueNumber };
|
||||
}
|
||||
|
||||
function isThrottleRejection(result: PromiseSettledResult<unknown>): boolean {
|
||||
return result.status === 'rejected' && result.reason instanceof ProviderThrottleError;
|
||||
}
|
||||
@@ -103,7 +62,7 @@ export class ComicVineProvider implements IdentifiableProvider {
|
||||
private readonly logger = new Logger(ComicVineProvider.name);
|
||||
|
||||
readonly cache: ProviderCacheSpec<MetadataSearchParams> = {
|
||||
version: 1,
|
||||
version: 2,
|
||||
trustEmpty: true,
|
||||
// The issue endpoint answers an unknown id with 404.
|
||||
authoritativeLookupStatuses: [404],
|
||||
@@ -112,7 +71,7 @@ export class ComicVineProvider implements IdentifiableProvider {
|
||||
return providerCacheContext(enabled && Boolean(apiKey));
|
||||
},
|
||||
describeSearch: (params) => ({
|
||||
inputs: pickSearchInputs(params, ['title', 'seriesName', 'seriesIndex', 'titleIsExplicitQuery']),
|
||||
inputs: pickSearchInputs(params, ['title', 'seriesName', 'seriesIndex', 'titleIsExplicitQuery', 'requireUnambiguousMatch']),
|
||||
maxCandidates: requestedMax(params),
|
||||
}),
|
||||
describeLookup: (providerId) => ({ inputs: { id: providerId } }),
|
||||
@@ -134,12 +93,19 @@ export class ComicVineProvider implements IdentifiableProvider {
|
||||
|
||||
try {
|
||||
const maxCandidates = normalizeMaxCandidates(params.maxCandidatesPerProvider, PROVIDER_LIMITS.COMICVINE_MAX_RESULTS);
|
||||
const parsed = resolveIssueQuery(params);
|
||||
const parsed = resolveComicVineIssueQuery(params);
|
||||
const deadline = createSearchDeadline(PROVIDER_BUDGETS_MS.COMICVINE_SEARCH, params.signal);
|
||||
|
||||
try {
|
||||
const matches = parsed
|
||||
? await this.structuredSearch(parsed.seriesName, parsed.issueNumber, apiKey, maxCandidates, deadline)
|
||||
? await this.structuredSearch(
|
||||
parsed.seriesName,
|
||||
parsed.issueNumber,
|
||||
apiKey,
|
||||
maxCandidates,
|
||||
deadline,
|
||||
params.requireUnambiguousMatch ?? false,
|
||||
)
|
||||
: await this.generalSearch(params.title, apiKey, maxCandidates, deadline);
|
||||
|
||||
const volumeEnrichment = await this.enrichVolumesWithinBudget(matches, apiKey, deadline);
|
||||
@@ -204,25 +170,37 @@ export class ComicVineProvider implements IdentifiableProvider {
|
||||
apiKey: string,
|
||||
maxCandidates: number,
|
||||
deadline: SearchDeadline,
|
||||
requireUnambiguousMatch: boolean,
|
||||
): Promise<VolumeIssueMatch[]> {
|
||||
const startedAt = Date.now();
|
||||
const volumes = await this.client.searchVolumes(seriesName, apiKey, deadline.signal);
|
||||
if (volumes.length === 0) {
|
||||
this.logger.debug(`ComicVine: no volumes found for "${seriesName}"`);
|
||||
return [];
|
||||
}
|
||||
|
||||
const ranked = rankVolumes(volumes, seriesName, issueNumber).slice(0, VOLUME_PROBE_LIMIT);
|
||||
const ranked = rankVolumes(volumes, seriesName, issueNumber);
|
||||
const matches: VolumeIssueMatch[] = [];
|
||||
const seenIssueIds = new Set<number>();
|
||||
|
||||
for (let offset = 0; offset < ranked.length; offset += VOLUME_PROBE_BATCH) {
|
||||
const wave = ranked.slice(offset, offset + VOLUME_PROBE_BATCH);
|
||||
let complete = true;
|
||||
for (let offset = 0; offset < Math.min(ranked.length, VOLUME_PROBE_LIMIT);) {
|
||||
if (deadline.expired() || deadline.signal.aborted) {
|
||||
complete = false;
|
||||
break;
|
||||
}
|
||||
const groupScore = volumeMatchScore(ranked[offset], seriesName, issueNumber);
|
||||
let groupEnd = offset + 1;
|
||||
while (groupEnd < ranked.length && volumeMatchScore(ranked[groupEnd], seriesName, issueNumber) === groupScore) groupEnd++;
|
||||
const waveEnd = Math.min(offset + VOLUME_PROBE_BATCH, groupEnd, VOLUME_PROBE_LIMIT);
|
||||
const wave = ranked.slice(offset, waveEnd);
|
||||
// Settled rather than all: one probe being throttled or cut off by the deadline must not
|
||||
// discard the issues its siblings found.
|
||||
const probed = await Promise.allSettled(wave.map((volume) => this.probeVolume(volume, issueNumber, apiKey, deadline.signal)));
|
||||
|
||||
for (const result of probed) {
|
||||
if (result.status !== 'fulfilled') {
|
||||
complete = false;
|
||||
markProviderDegraded('comicvine.search: volume probe failed');
|
||||
continue;
|
||||
}
|
||||
@@ -232,16 +210,36 @@ export class ComicVineProvider implements IdentifiableProvider {
|
||||
matches.push({ issue, volume: result.value.volume });
|
||||
}
|
||||
}
|
||||
|
||||
offset = waveEnd;
|
||||
if (probed.some(isThrottleRejection)) {
|
||||
this.recordThrottle();
|
||||
break;
|
||||
}
|
||||
if (matches.length > 0) break;
|
||||
if (deadline.expired()) {
|
||||
this.logger.debug(`ComicVine: budget spent after ${offset + wave.length} volume probes for "${seriesName}" #${issueNumber}`);
|
||||
if (offset === VOLUME_PROBE_LIMIT && offset < groupEnd) {
|
||||
complete = false;
|
||||
markProviderTruncated();
|
||||
break;
|
||||
}
|
||||
// Equal scores must all be probed, even across waves. A lower-score collection cannot
|
||||
// resolve ambiguity between matching runs and should not consume the remaining budget.
|
||||
if (matches.length > 0 && offset === groupEnd) break;
|
||||
if (deadline.expired()) {
|
||||
complete = false;
|
||||
this.logger.debug(`ComicVine: budget spent after ${offset} volume probes for "${seriesName}" #${issueNumber}`);
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
// The client records unusable responses before returning an empty issue list. That is not
|
||||
// evidence that a competing run lacks the issue, even when the probe promise fulfilled.
|
||||
const record = currentProviderCallRecord();
|
||||
if (record && (record.failureReasons.length > 0 || record.unsuccessfulRequests().length > 0 || record.truncated)) complete = false;
|
||||
|
||||
if (requireUnambiguousMatch && matches.length > 0 && (!complete || matches.length > 1)) {
|
||||
this.logger.log(
|
||||
`[comicvine.search] [end] series="${sanitizeLogValue(seriesName)}" issueNumber="${sanitizeLogValue(issueNumber)}" durationMs=${Date.now() - startedAt} candidateCount=${matches.length} complete=${complete} outcome=ambiguous - volume match requires manual selection`,
|
||||
);
|
||||
return [];
|
||||
}
|
||||
|
||||
if (matches.length === 0) {
|
||||
|
||||
+112
-2
@@ -134,6 +134,8 @@ interface HarnessOptions {
|
||||
issueDetailMs?: number;
|
||||
// Volumes and issue details ComicVine answers with HTTP 420, its throttle response.
|
||||
throttledVolumeIds?: number[];
|
||||
failedVolumeIds?: number[];
|
||||
slowVolumeIds?: number[];
|
||||
throttledDetailIds?: string[];
|
||||
}
|
||||
|
||||
@@ -186,8 +188,12 @@ function buildHarness(options: HarnessOptions = {}): Harness {
|
||||
const volumeId = Number(/volume:(\d+)/.exec(filter)?.[1]);
|
||||
const issueNumber = /issue_number:([^,]+)/.exec(filter)?.[1] ?? '';
|
||||
probedVolumeIds.push(volumeId);
|
||||
return respondAfter(ISSUE_PROBE_MS, () =>
|
||||
options.throttledVolumeIds?.includes(volumeId) ? throttledResponse() : okResponse(issues[`${volumeId}#${issueNumber}`] ?? []),
|
||||
return respondAfter(options.slowVolumeIds?.includes(volumeId) ? 30_000 : ISSUE_PROBE_MS, () =>
|
||||
options.throttledVolumeIds?.includes(volumeId)
|
||||
? throttledResponse()
|
||||
: options.failedVolumeIds?.includes(volumeId)
|
||||
? ({ ok: true, status: 200, json: () => Promise.resolve({ status_code: 100, error: 'Unavailable', results: [] }) } as Response)
|
||||
: okResponse(issues[`${volumeId}#${issueNumber}`] ?? []),
|
||||
);
|
||||
}
|
||||
|
||||
@@ -255,6 +261,110 @@ describe('ComicVine search budget', () => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
const generationXVolumes = [
|
||||
{ ...volume(47374, 'Generation X', '1997', 13), publisher: { id: 1, name: 'Panini France' } },
|
||||
{ ...volume(5300, 'Generation X', '1994', 77), publisher: { id: 2, name: 'Marvel' } },
|
||||
];
|
||||
const generationXIssues = {
|
||||
'47374#1': [{ ...issue(325232, 47374, 'Generation X', '1'), name: null }],
|
||||
'5300#1': [{ ...issue(39935, 5300, 'Generation X', '1'), name: 'Third Genesis' }],
|
||||
};
|
||||
|
||||
it.each(['Generation X #1', 'Generation X #01'])('surfaces both reported runs through the real search service for %s', async (title) => {
|
||||
const harness = buildHarness({ volumes: generationXVolumes, issues: generationXIssues });
|
||||
const { value: candidates } = await runToCompletion(
|
||||
firstValueFrom(harness.service.searchCandidates({ title }, [MetadataProviderKey.COMICVINE]).pipe(toArray())),
|
||||
);
|
||||
|
||||
expect(harness.probedVolumeIds).toEqual([47374, 5300]);
|
||||
expect(candidates.map((candidate) => candidate.providerId)).toEqual(['325232', '39935']);
|
||||
expect(candidates.map((candidate) => candidate.publisher)).toEqual(['Panini France', 'Marvel']);
|
||||
});
|
||||
|
||||
it('does not silently choose the reported reprint in an automatic one-result search', async () => {
|
||||
const harness = buildHarness({ volumes: generationXVolumes, issues: generationXIssues });
|
||||
const { value: candidates } = await runToCompletion(
|
||||
firstValueFrom(
|
||||
harness.service
|
||||
.searchCandidates({ title: 'Generation X #01', maxCandidatesPerProvider: 1, requireUnambiguousMatch: true }, [
|
||||
MetadataProviderKey.COMICVINE,
|
||||
])
|
||||
.pipe(toArray()),
|
||||
),
|
||||
);
|
||||
|
||||
expect(harness.probedVolumeIds).toEqual([47374, 5300]);
|
||||
expect(candidates).toEqual([]);
|
||||
});
|
||||
|
||||
it.each([true, false])('keeps deadline-limited matching safe when automatic matching is %s', async (automatic) => {
|
||||
const volumes = Array.from({ length: 4 }, (_, i) => volume(1000 + i, 'Generation X', '2020', 10));
|
||||
const harness = buildHarness({
|
||||
volumes,
|
||||
issues: { '1000#1': [issue(900001, 1000, 'Generation X', '1')] },
|
||||
slowVolumeIds: [1003],
|
||||
});
|
||||
const { value: candidates, elapsedMs } = await runToCompletion(
|
||||
firstValueFrom(
|
||||
harness.service
|
||||
.searchCandidates({ title: 'Generation X #1', requireUnambiguousMatch: automatic }, [MetadataProviderKey.COMICVINE])
|
||||
.pipe(toArray()),
|
||||
),
|
||||
);
|
||||
|
||||
expect(harness.probedVolumeIds).toEqual([1000, 1001, 1002, 1003]);
|
||||
expect(harness.abortedPaths).toContain('/api/issues/');
|
||||
expect(candidates).toHaveLength(automatic ? 0 : 1);
|
||||
expect(elapsedMs).toBeLessThan(PROVIDER_TIMEOUT_MS.SCRAPE);
|
||||
});
|
||||
|
||||
it('does not mistake a swallowed API failure for a competing run having no issue', async () => {
|
||||
const harness = buildHarness({
|
||||
volumes: generationXVolumes,
|
||||
issues: { '47374#1': generationXIssues['47374#1'] },
|
||||
failedVolumeIds: [5300],
|
||||
});
|
||||
const { value: candidates } = await runToCompletion(
|
||||
firstValueFrom(
|
||||
harness.service
|
||||
.searchCandidates({ title: 'Generation X #1', requireUnambiguousMatch: true }, [MetadataProviderKey.COMICVINE])
|
||||
.pipe(toArray()),
|
||||
),
|
||||
);
|
||||
|
||||
expect(candidates).toEqual([]);
|
||||
expect(harness.probedVolumeIds).toEqual([47374, 5300]);
|
||||
});
|
||||
|
||||
it('refreshes a previously selected original directly despite same-titled reprints', async () => {
|
||||
const harness = buildHarness({
|
||||
volumes: generationXVolumes,
|
||||
issues: generationXIssues,
|
||||
issueDetails: { '39935': generationXIssues['5300#1'][0] },
|
||||
volumeDetails: { 5300: generationXVolumes[1] },
|
||||
});
|
||||
const { value: candidates } = await runToCompletion(
|
||||
firstValueFrom(
|
||||
harness.service
|
||||
.searchCandidates(
|
||||
{
|
||||
title: 'Generation X #01',
|
||||
existingProviderIds: { [MetadataProviderKey.COMICVINE]: '39935' },
|
||||
maxCandidatesPerProvider: 1,
|
||||
requireUnambiguousMatch: true,
|
||||
},
|
||||
[MetadataProviderKey.COMICVINE],
|
||||
)
|
||||
.pipe(toArray()),
|
||||
),
|
||||
);
|
||||
|
||||
expect(candidates.map((candidate) => candidate.providerId)).toEqual(['39935']);
|
||||
expect(candidates[0]?.publisher).toBe('Marvel');
|
||||
expect(harness.probedVolumeIds).toEqual([]);
|
||||
expect(harness.detailIssueIds).toEqual(['39935']);
|
||||
});
|
||||
|
||||
it('keeps its own budget under the timeout that aborts it', () => {
|
||||
expect(PROVIDER_BUDGETS_MS.COMICVINE_SEARCH).toBeLessThan(PROVIDER_TIMEOUT_MS.SCRAPE);
|
||||
});
|
||||
|
||||
@@ -29,6 +29,8 @@ export interface MetadataSearchParams {
|
||||
// Hint for providers to cap deep candidate exploration in non-interactive flows
|
||||
// (e.g. auto-fill/background refresh where there is no manual candidate picking).
|
||||
maxCandidatesPerProvider?: number;
|
||||
// Automatic pipelines must not select a ComicVine run from equally plausible volume matches.
|
||||
requireUnambiguousMatch?: boolean;
|
||||
// Interactive result lists can afford bounded cover checks. Bulk metadata pipelines must not
|
||||
// multiply one provider lookup into many thumbnail requests.
|
||||
validateCoverPlaceholders?: boolean;
|
||||
|
||||
@@ -104,6 +104,7 @@ const BASE: MetadataSearchParams = {
|
||||
isAudiobook: true,
|
||||
includeAudiobookProviders: true,
|
||||
maxCandidatesPerProvider: 2,
|
||||
requireUnambiguousMatch: false,
|
||||
validateCoverPlaceholders: true,
|
||||
};
|
||||
const WITHOUT_ISBN: MetadataSearchParams = { ...BASE, isbn: undefined };
|
||||
@@ -117,6 +118,7 @@ type Mutation =
|
||||
| 'titleIsExplicitQuery'
|
||||
| 'validateCoverPlaceholders'
|
||||
| 'maxCandidatesPerProvider'
|
||||
| 'requireUnambiguousMatch'
|
||||
| 'hardcoverEditionId'
|
||||
| 'isAudiobook'
|
||||
| 'existingProviderIds';
|
||||
@@ -131,6 +133,7 @@ const MUTATIONS: Record<Mutation, (params: MetadataSearchParams) => MetadataSear
|
||||
titleIsExplicitQuery: (params) => ({ ...params, titleIsExplicitQuery: true }),
|
||||
validateCoverPlaceholders: (params) => ({ ...params, validateCoverPlaceholders: false }),
|
||||
maxCandidatesPerProvider: (params) => ({ ...params, maxCandidatesPerProvider: 1 }),
|
||||
requireUnambiguousMatch: (params) => ({ ...params, requireUnambiguousMatch: true }),
|
||||
hardcoverEditionId: (params) => ({ ...params, hardcoverEditionId: '99' }),
|
||||
// Leaves the audiobook gate on, since includeAudiobookProviders is set.
|
||||
isAudiobook: (params) => ({ ...params, isAudiobook: false }),
|
||||
@@ -253,9 +256,9 @@ const CASES: DeclarationCase[] = [
|
||||
},
|
||||
{
|
||||
key: MetadataProviderKey.COMICVINE,
|
||||
withIsbn: { title: 'Dune', seriesName: 'Dune Chronicles', seriesIndex: '1', titleIsExplicitQuery: false },
|
||||
withoutIsbn: { title: 'Dune', seriesName: 'Dune Chronicles', seriesIndex: '1', titleIsExplicitQuery: false },
|
||||
searchReads: ['seriesName', 'seriesIndex', 'titleIsExplicitQuery', 'maxCandidatesPerProvider'],
|
||||
withIsbn: { title: 'Dune', seriesName: 'Dune Chronicles', seriesIndex: '1', titleIsExplicitQuery: false, requireUnambiguousMatch: false },
|
||||
withoutIsbn: { title: 'Dune', seriesName: 'Dune Chronicles', seriesIndex: '1', titleIsExplicitQuery: false, requireUnambiguousMatch: false },
|
||||
searchReads: ['seriesName', 'seriesIndex', 'titleIsExplicitQuery', 'maxCandidatesPerProvider', 'requireUnambiguousMatch'],
|
||||
honoursMax: true,
|
||||
lookup: LOOKUP_ID,
|
||||
fingerprint: {},
|
||||
@@ -322,8 +325,8 @@ describe('provider cache declarations', () => {
|
||||
});
|
||||
|
||||
describe.each(CASES)('$key', (testCase) => {
|
||||
it('declares version 1', () => {
|
||||
expect(specOf(testCase.key).version).toBe(1);
|
||||
it('declares the current mapping version', () => {
|
||||
expect(specOf(testCase.key).version).toBe(testCase.key === MetadataProviderKey.COMICVINE ? 2 : 1);
|
||||
});
|
||||
|
||||
it('keys a search on the fields it reads', () => {
|
||||
|
||||
@@ -12,7 +12,8 @@ export function providerCacheContext(available: boolean, fingerprint: Record<str
|
||||
return { available, fingerprint: canonicalJson(fingerprint) };
|
||||
}
|
||||
|
||||
type SearchField = 'title' | 'author' | 'isbn' | 'seriesName' | 'seriesIndex' | 'titleIsExplicitQuery' | 'hardcoverEditionId';
|
||||
type SearchField =
|
||||
'title' | 'author' | 'isbn' | 'seriesName' | 'seriesIndex' | 'titleIsExplicitQuery' | 'hardcoverEditionId' | 'requireUnambiguousMatch';
|
||||
|
||||
export function pickSearchInputs(params: MetadataSearchParams, fields: readonly SearchField[]): Record<string, unknown> {
|
||||
const inputs: Record<string, unknown> = {};
|
||||
|
||||
Reference in New Issue
Block a user