From 8c854dada824c10971113be494d894a802744b7d Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Tue, 25 Aug 2026 00:41:12 +0000 Subject: [PATCH] feat(metadata): merge fetched genres with existing values Preserve curated genres while appending unique values returned by metadata providers, and expose merge versus replace behavior in the web interface. Closes #1125 --- .../detail/tabs/MetadataDiffPanel.vue | 33 ++++-- .../detail/tabs/MetadataDiffRow.vue | 41 ++++++- .../__tests__/useMetadataDiff.spec.ts | 34 ++++++ .../book/composables/useMetadataDiff.ts | 35 +++++- .../components/FieldRuleDetail.vue | 10 +- .../components/FieldRuleRow.vue | 11 +- .../components/MergeStrategySegmented.spec.ts | 21 ++++ .../components/MergeStrategySegmented.vue | 24 ++-- client/src/locales/en.json | 12 +- packages/types/src/metadata-preferences.ts | 4 +- .../common/utils/genre-fetch-options.utils.ts | 21 ++++ .../metadata-fetch-pipeline.test.ts | 108 ++++++++++++++++++ .../metadata-fetch/metadata-fetch-pipeline.ts | 49 +++++--- .../dto/update-global-preferences.dto.test.ts | 24 ++++ .../dto/update-global-preferences.dto.ts | 9 +- .../metadata-preference-resolver.test.ts | 17 +++ .../metadata-preference-resolver.ts | 21 ++-- 17 files changed, 413 insertions(+), 61 deletions(-) create mode 100644 client/src/features/settings/metadata-preferences/components/MergeStrategySegmented.spec.ts diff --git a/client/src/features/book/components/detail/tabs/MetadataDiffPanel.vue b/client/src/features/book/components/detail/tabs/MetadataDiffPanel.vue index 6515dcb78..d4b8f14a2 100644 --- a/client/src/features/book/components/detail/tabs/MetadataDiffPanel.vue +++ b/client/src/features/book/components/detail/tabs/MetadataDiffPanel.vue @@ -94,16 +94,27 @@ const activeCandidate = computed( props.initialCandidate, ) -const { fields, picksPerProvider, toggleField, pickFieldFromProvider, clearPicksForProvider, copyAll, copyMissing, buildPatch, hasCopied } = - useMetadataDiff( - props.current, - representativeCandidates, - activeProvider, - computed(() => props.providers), - props.currentCoverUrl, - computed(() => props.providerIds), - computed(() => props.lockedFields ?? []), - ) +const { + fields, + picksPerProvider, + toggleField, + pickFieldFromProvider, + clearPicksForProvider, + copyAll, + copyMissing, + buildPatch, + hasCopied, + genreWriteMode, + setGenreWriteMode, +} = useMetadataDiff( + props.current, + representativeCandidates, + activeProvider, + computed(() => props.providers), + props.currentCoverUrl, + computed(() => props.providerIds), + computed(() => props.lockedFields ?? []), +) const activeProviderLabel = computed(() => getProviderLabel(activeProvider.value, props.providers)) const resultsForActiveProvider = computed(() => props.filteredResults.filter((candidate) => candidate.provider === activeProvider.value)) @@ -374,8 +385,10 @@ onBeforeUnmount(() => { :book-author-line="bookAuthorLine" :candidate-seed="candidateSeed" :candidate-author-line="candidateAuthorLine" + :genre-write-mode="genreWriteMode" @toggle="toggleField" @pick-from-provider="handlePickFromProvider" + @update:genre-write-mode="setGenreWriteMode" />

{{ showUnchanged ? t('book.detail.editMetadata.diffPanel.noMetadata') : t('book.detail.editMetadata.diffPanel.noChangedFields') }} diff --git a/client/src/features/book/components/detail/tabs/MetadataDiffRow.vue b/client/src/features/book/components/detail/tabs/MetadataDiffRow.vue index 7a59c5fe8..482151bb2 100644 --- a/client/src/features/book/components/detail/tabs/MetadataDiffRow.vue +++ b/client/src/features/book/components/detail/tabs/MetadataDiffRow.vue @@ -3,7 +3,7 @@ import { computed, inject, ref, watch } from 'vue' import { useI18n } from 'vue-i18n' import { ArrowLeft, RotateCcw, CheckCircle2, X, ZoomIn, Layers } from '@lucide/vue' import type { MetadataProviderInfo, MetadataProviderKey } from '@bookorbit/types' -import type { DiffField, DiffFieldKey } from '../../../composables/useMetadataDiff' +import type { DiffField, DiffFieldKey, GenreWriteMode } from '../../../composables/useMetadataDiff' import { hideOnError, providerBadgeStyle } from '../../../lib/metadata-fetch' import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' import { COVER_ASPECT_RATIO_KEY, DEFAULT_COVER_ASPECT_RATIO } from '../../../lib/cover-aspect-ratio' @@ -17,11 +17,13 @@ const props = defineProps<{ bookAuthorLine?: string | null candidateSeed?: string candidateAuthorLine?: string | null + genreWriteMode?: GenreWriteMode }>() const emit = defineEmits<{ toggle: [DiffFieldKey] pickFromProvider: [key: DiffFieldKey, provider: MetadataProviderKey] + 'update:genreWriteMode': [mode: GenreWriteMode] }>() const { t } = useI18n() @@ -71,6 +73,14 @@ function handlePickFromProvider(provider: MetadataProviderKey) { emit('pickFromProvider', props.field.key, provider) } +function mergeGenres() { + emit('update:genreWriteMode', 'merge') +} + +function replaceGenres() { + emit('update:genreWriteMode', 'replace') +} + function toggleCurrentExpanded() { isCurrentExpanded.value = !isCurrentExpanded.value } @@ -256,6 +266,35 @@ watch( {{ t('book.detail.editMetadata.diff.locked') }} +

+ + +
diff --git a/client/src/features/book/composables/__tests__/useMetadataDiff.spec.ts b/client/src/features/book/composables/__tests__/useMetadataDiff.spec.ts index c122a60f9..af9b47aaa 100644 --- a/client/src/features/book/composables/__tests__/useMetadataDiff.spec.ts +++ b/client/src/features/book/composables/__tests__/useMetadataDiff.spec.ts @@ -114,6 +114,40 @@ describe('useMetadataDiff', () => { expect(formPatch.goodreadsId).toBe('gr1') }) + it('merges provider genres into the current list by default', () => { + const candidate: MetadataCandidate = { + provider: 'goodreads', + providerId: 'gr1', + title: 'Goodreads Title', + genres: [' genre 1 ', 'Genre 2', 'GENRE 2', ''], + } + const candidates = ref([candidate]) + const activeProvider = ref('goodreads') + const { fields, toggleField, buildPatch } = useMetadataDiff(mockCurrent, candidates, activeProvider, providers) + + toggleField('genres') + + expect(fields.value.find((field) => field.key === 'genres')?.currentDisplay).toBe('Genre 1, Genre 2') + expect(buildPatch().formPatch.genres).toEqual(['Genre 1', 'Genre 2']) + }) + + it('can replace current genres explicitly', () => { + const candidate: MetadataCandidate = { + provider: 'goodreads', + providerId: 'gr1', + title: 'Goodreads Title', + genres: ['Genre 2', 'genre 2', 'Genre 3'], + } + const candidates = ref([candidate]) + const activeProvider = ref('goodreads') + const { toggleField, buildPatch, setGenreWriteMode } = useMetadataDiff(mockCurrent, candidates, activeProvider, providers) + + toggleField('genres') + setGenreWriteMode('replace') + + expect(buildPatch().formPatch.genres).toEqual(['Genre 2', 'Genre 3']) + }) + it('shows and applies the Libro.fm ISBN as a provider ID', () => { const candidate: MetadataCandidate = { provider: 'librofm', diff --git a/client/src/features/book/composables/useMetadataDiff.ts b/client/src/features/book/composables/useMetadataDiff.ts index a312b2eee..d48e03ae3 100644 --- a/client/src/features/book/composables/useMetadataDiff.ts +++ b/client/src/features/book/composables/useMetadataDiff.ts @@ -1,4 +1,4 @@ -import { computed, reactive, toValue, type MaybeRefOrGetter } from 'vue' +import { computed, reactive, ref, toValue, type MaybeRefOrGetter } from 'vue' import type { BookCommunityRating, BookMetadataLockField, @@ -114,6 +114,23 @@ export interface MetadataPatch { customMetadata?: CustomMetadataBookValueInput[] } +export type GenreWriteMode = 'merge' | 'replace' + +export function mergeGenreLists(existing: readonly string[], incoming: readonly string[]): string[] { + const merged: string[] = [] + const seen = new Set() + + for (const raw of [...existing, ...incoming]) { + const genre = raw.trim() + const token = genre.toLowerCase() + if (!genre || seen.has(token)) continue + seen.add(token) + merged.push(genre) + } + + return merged +} + export const FIELD_DEFS: { key: DiffFieldKey; label: string }[] = [ { key: 'coverUrl', label: 'Cover' }, { key: 'title', label: 'Title' }, @@ -266,6 +283,7 @@ export function useMetadataDiff( ) { const pickedSources = reactive(new Map()) const pickedCommunityRatingProviders = reactive(new Set()) + const genreWriteMode = ref('merge') const lockedFieldSet = computed(() => new Set(toValue(lockedFields) ?? [])) function resolveLockField(key: DiffFieldKey): BookMetadataLockField | null { @@ -352,7 +370,11 @@ export function useMetadataDiff( .join(', ') : (() => { const pickedCandidate = isPicked ? allCandidates.find((c) => c.provider === pickedProvider) : null - return pickedCandidate ? getCandidateValueFrom(pickedCandidate, key) : '' + if (!pickedCandidate) return '' + if (key === 'genres' && genreWriteMode.value === 'merge') { + return mergeGenreLists(current.genres, pickedCandidate.genres ?? []).join(', ') + } + return getCandidateValueFrom(pickedCandidate, key) })() const lockField = resolveLockField(key) @@ -522,7 +544,8 @@ export function useMetadataDiff( continue } if (key === 'genres') { - formPatch.genres = candidate.genres ?? [] + formPatch.genres = + genreWriteMode.value === 'merge' ? mergeGenreLists(current.genres, candidate.genres ?? []) : mergeGenreLists([], candidate.genres ?? []) continue } if (key === 'narrators') { @@ -639,6 +662,10 @@ export function useMetadataDiff( const hasCopied = computed(() => pickedSources.size > 0 || pickedCommunityRatingProviders.size > 0) + function setGenreWriteMode(mode: GenreWriteMode) { + genreWriteMode.value = mode + } + return { fields, pickedSources, @@ -650,5 +677,7 @@ export function useMetadataDiff( copyMissing, buildPatch, hasCopied, + genreWriteMode, + setGenreWriteMode, } } diff --git a/client/src/features/settings/metadata-preferences/components/FieldRuleDetail.vue b/client/src/features/settings/metadata-preferences/components/FieldRuleDetail.vue index 87db2265c..e6fa0652f 100644 --- a/client/src/features/settings/metadata-preferences/components/FieldRuleDetail.vue +++ b/client/src/features/settings/metadata-preferences/components/FieldRuleDetail.vue @@ -2,7 +2,7 @@ import { computed } from 'vue' import { useI18n } from 'vue-i18n' import { ArrowRight, ChevronDown, ChevronUp, RotateCcw, TriangleAlert, X } from '@lucide/vue' -import type { FieldPreference, MergeStrategy, MetadataProviderKey, ProviderStatus } from '@bookorbit/types' +import type { FieldPreference, MetadataField, MetadataMergeStrategy, MetadataProviderKey, ProviderStatus } from '@bookorbit/types' import { Button } from '@/components/ui/button' import { providerChipStyle, PROVIDER_SHORT_LABELS } from '@/lib/provider-colors' import ProviderAddMenu from './ProviderAddMenu.vue' @@ -11,6 +11,7 @@ import { resolvedProviders, skippedProviders, skipReasonFor } from '../lib/field const { t } = useI18n() const props = defineProps<{ + field: MetadataField fieldLabel: string preference: FieldPreference statuses: ProviderStatus[] @@ -26,7 +27,8 @@ const emit = defineEmits<{ close: [] }>() -const MERGE_ORDER: MergeStrategy[] = ['fillMissing', 'overwriteIfProvided', 'overwrite'] +const STANDARD_MERGE_ORDER: MetadataMergeStrategy[] = ['fillMissing', 'overwriteIfProvided', 'overwrite'] +const GENRE_MERGE_ORDER: MetadataMergeStrategy[] = ['fillMissing', 'mergeExisting', 'overwriteIfProvided', 'overwrite'] const rows = computed(() => props.preference.providers.map((key, index) => { @@ -46,7 +48,7 @@ const skipped = computed(() => skippedProviders(props.preference.providers, prop const providerLabel = (key: MetadataProviderKey) => props.statuses.find((entry) => entry.key === key)?.label ?? key const mergeOptions = computed(() => - MERGE_ORDER.map((value) => ({ + (props.field === 'genres' ? GENRE_MERGE_ORDER : STANDARD_MERGE_ORDER).map((value) => ({ value, label: t(`settings.metadata.mergeStrategy.${value}.label`), description: t(`settings.metadata.mergeStrategy.${value}.description`), @@ -85,7 +87,7 @@ function addProvider(provider: MetadataProviderKey) { update({ providers: [...props.preference.providers, provider] }) } -function selectMerge(value: MergeStrategy) { +function selectMerge(value: MetadataMergeStrategy) { update({ mergeStrategy: value }) } diff --git a/client/src/features/settings/metadata-preferences/components/FieldRuleRow.vue b/client/src/features/settings/metadata-preferences/components/FieldRuleRow.vue index 1bda1dcfe..55b722ab8 100644 --- a/client/src/features/settings/metadata-preferences/components/FieldRuleRow.vue +++ b/client/src/features/settings/metadata-preferences/components/FieldRuleRow.vue @@ -2,7 +2,7 @@ import { computed } from 'vue' import { useI18n } from 'vue-i18n' import { ChevronDown, ChevronRight, TriangleAlert } from '@lucide/vue' -import type { FieldPreference, MergeStrategy, MetadataField, MetadataProviderKey, ProviderStatus } from '@bookorbit/types' +import type { FieldPreference, MetadataField, MetadataMergeStrategy, MetadataProviderKey, ProviderStatus } from '@bookorbit/types' import ToggleSwitch from '@/components/ui/ToggleSwitch.vue' import { providerChipStyle, PROVIDER_SHORT_LABELS } from '@/lib/provider-colors' import { Badge } from '@/components/ui/badge' @@ -57,7 +57,7 @@ function onProvidersChange(providers: MetadataProviderKey[]) { update({ providers }) } -function onMergeChange(mergeStrategy: MergeStrategy) { +function onMergeChange(mergeStrategy: MetadataMergeStrategy) { update({ mergeStrategy }) } @@ -125,7 +125,12 @@ function collapse() {
- +
diff --git a/client/src/features/settings/metadata-preferences/components/MergeStrategySegmented.spec.ts b/client/src/features/settings/metadata-preferences/components/MergeStrategySegmented.spec.ts new file mode 100644 index 000000000..193ac413a --- /dev/null +++ b/client/src/features/settings/metadata-preferences/components/MergeStrategySegmented.spec.ts @@ -0,0 +1,21 @@ +import { mount } from '@vue/test-utils' +import { describe, expect, it } from 'vitest' + +import MergeStrategySegmented from './MergeStrategySegmented.vue' + +describe('MergeStrategySegmented', () => { + it('offers merge-with-existing only for genre fields', async () => { + const standard = mount(MergeStrategySegmented, { props: { modelValue: 'fillMissing' } }) + const genres = mount(MergeStrategySegmented, { + props: { modelValue: 'mergeExisting', allowMergeExisting: true }, + }) + + expect(standard.findAll('[role="radio"]')).toHaveLength(3) + expect(standard.text()).not.toContain('Merge') + expect(genres.findAll('[role="radio"]')).toHaveLength(4) + expect(genres.text()).toContain('Merge') + + await genres.findAll('[role="radio"]')[2]!.trigger('click') + expect(genres.emitted('update:modelValue')).toEqual([['overwriteIfProvided']]) + }) +}) diff --git a/client/src/features/settings/metadata-preferences/components/MergeStrategySegmented.vue b/client/src/features/settings/metadata-preferences/components/MergeStrategySegmented.vue index 8d3211ed6..d4cb23bf1 100644 --- a/client/src/features/settings/metadata-preferences/components/MergeStrategySegmented.vue +++ b/client/src/features/settings/metadata-preferences/components/MergeStrategySegmented.vue @@ -1,30 +1,32 @@ diff --git a/client/src/locales/en.json b/client/src/locales/en.json index d5f95409a..1ad902717 100644 --- a/client/src/locales/en.json +++ b/client/src/locales/en.json @@ -1433,6 +1433,11 @@ "description": "Only write if field is currently empty", "short": "Fill gaps" }, + "mergeExisting": { + "label": "Merge with existing", + "description": "Keep stored genres and append new provider genres", + "short": "Merge" + }, "overwriteIfProvided": { "label": "Overwrite if provided", "description": "Write if provider returned a value", @@ -1598,7 +1603,7 @@ "overwriteTerm": "Overwrite:", "overwriteBody": "Replace whenever a provider has data." }, - "summary": "Providers are tried top to bottom and the first one that returns a value wins. The merge strategy decides whether that value may replace what the book already has. Set defaults here, then override individual fields per library." + "summary": "Providers are tried top to bottom and the first one that returns a value wins. The merge strategy decides whether that value may replace what the book already has; genres can also append new unique values. Set defaults here, then override individual fields per library." }, "libraryOverrides": { "title": "Library Overrides", @@ -4482,6 +4487,11 @@ "pickCoverSource": "Pick cover source", "pickFromProvider": "Pick from another provider", "pickSource": "Pick source", + "genreWriteMode": { + "label": "How to apply genres", + "merge": "Merge", + "replace": "Replace" + }, "empty": "empty", "showLess": "Show less", "showMore": "Show more", diff --git a/packages/types/src/metadata-preferences.ts b/packages/types/src/metadata-preferences.ts index 2662d8021..34a84138b 100644 --- a/packages/types/src/metadata-preferences.ts +++ b/packages/types/src/metadata-preferences.ts @@ -38,15 +38,17 @@ export const ALL_METADATA_FIELDS: MetadataField[] = [ ]; export type MergeStrategy = "fillMissing" | "overwrite" | "overwriteIfProvided"; +export type MetadataMergeStrategy = MergeStrategy | "mergeExisting"; export type GenreMergeMode = "firstProvider" | "merge"; export const MERGE_STRATEGIES: MergeStrategy[] = ["fillMissing", "overwrite", "overwriteIfProvided"]; +export const GENRE_MERGE_STRATEGIES: MetadataMergeStrategy[] = ["fillMissing", "mergeExisting", "overwriteIfProvided", "overwrite"]; export const GENRE_MERGE_MODES: GenreMergeMode[] = ["firstProvider", "merge"]; export const MAX_METADATA_GENRE_COUNT = 50; export interface FieldPreference { enabled: boolean; providers: MetadataProviderKey[]; - mergeStrategy: MergeStrategy; + mergeStrategy: MetadataMergeStrategy; } export type FieldPreferenceOverrides = Partial>; diff --git a/server/src/common/utils/genre-fetch-options.utils.ts b/server/src/common/utils/genre-fetch-options.utils.ts index cbefb1f71..3fb588030 100644 --- a/server/src/common/utils/genre-fetch-options.utils.ts +++ b/server/src/common/utils/genre-fetch-options.utils.ts @@ -41,6 +41,27 @@ export function applyGenreFetchOptions( return resolved; } +export function mergeExistingGenres( + existingGenres: readonly string[] | undefined, + incomingGenres: readonly string[], + maxCount: number | null | undefined, +): string[] { + const merged = applyGenreFetchOptions(existingGenres, new Set(), null); + const seen = new Set(merged.map((genre) => genre.toLowerCase())); + + for (const raw of incomingGenres) { + const genre = raw.trim(); + const token = genre.toLowerCase(); + if (!genre || seen.has(token)) continue; + if (maxCount !== null && maxCount !== undefined && merged.length >= maxCount) break; + + seen.add(token); + merged.push(genre); + } + + return merged; +} + export function applyGenreFetchOptionsToCandidate( candidate: T, blockedTokens: ReadonlySet, diff --git a/server/src/modules/metadata-fetch/metadata-fetch-pipeline.test.ts b/server/src/modules/metadata-fetch/metadata-fetch-pipeline.test.ts index ebc734f0d..0bc5fb01a 100644 --- a/server/src/modules/metadata-fetch/metadata-fetch-pipeline.test.ts +++ b/server/src/modules/metadata-fetch/metadata-fetch-pipeline.test.ts @@ -978,6 +978,114 @@ describe('MetadataFetchPipeline', () => { expect(sources.genres).toBe(MetadataProviderKey.GOOGLE); }); + it('merges fetched genres into stored genres without changing stored spelling or order', async () => { + const prefs = createPreferences((fields) => { + fields.genres = { + enabled: true, + providers: [MetadataProviderKey.GOOGLE, MetadataProviderKey.OPEN_LIBRARY], + mergeStrategy: 'mergeExisting', + }; + }); + prefs.options = { + genres: { mode: 'merge', blocklist: [], maxCount: null }, + saveProviderIds: false, + }; + + preferencesService.getGlobal.mockResolvedValue(prefs); + resolver.resolve.mockReturnValue(prefs); + resolver.withForwardCompatibility.mockReturnValue(prefs); + registry.all.mockReturnValue([{ key: MetadataProviderKey.GOOGLE }, { key: MetadataProviderKey.OPEN_LIBRARY }] as never); + fetchService.searchCandidates.mockReturnValue( + of( + candidate(MetadataProviderKey.GOOGLE, 'g1', { genres: ['romance', 'Magic'] }), + candidate(MetadataProviderKey.OPEN_LIBRARY, 'ol1', { genres: ['Dark Academia', 'MAGIC'] }), + ), + ); + + const { resolved, sources } = await pipeline.runWithSources( + { title: 'Query' }, + { genres: ['Romance', 'Literary Fiction', 'Contemporary', 'Fantasy'] }, + ); + + expect(resolved.genres).toEqual(['Romance', 'Literary Fiction', 'Contemporary', 'Fantasy', 'Magic', 'Dark Academia']); + expect(sources.genres).toBe(MetadataProviderKey.GOOGLE); + }); + + it('preserves stored blocklisted genres and limits only new merged additions', async () => { + const prefs = createPreferences((fields) => { + fields.genres = { + enabled: true, + providers: [MetadataProviderKey.GOOGLE], + mergeStrategy: 'mergeExisting', + }; + }); + prefs.options = { + genres: { mode: 'firstProvider', blocklist: ['Adult'], maxCount: 3 }, + saveProviderIds: false, + }; + + preferencesService.getGlobal.mockResolvedValue(prefs); + resolver.resolve.mockReturnValue(prefs); + resolver.withForwardCompatibility.mockReturnValue(prefs); + registry.all.mockReturnValue([{ key: MetadataProviderKey.GOOGLE }] as never); + fetchService.searchCandidates.mockReturnValue( + of(candidate(MetadataProviderKey.GOOGLE, 'g1', { genres: ['Adult', 'Fantasy', 'Mystery', 'Classic'] })), + ); + + const { resolved } = await pipeline.runWithSources({ title: 'Query' }, { genres: ['Adult', 'Romance'] }); + + expect(resolved.genres).toEqual(['Adult', 'Romance', 'Fantasy']); + }); + + it('does not write genres when merge-with-existing finds no new values', async () => { + const prefs = createPreferences((fields) => { + fields.genres = { + enabled: true, + providers: [MetadataProviderKey.GOOGLE], + mergeStrategy: 'mergeExisting', + }; + }); + prefs.options = { + genres: { mode: 'firstProvider', blocklist: [], maxCount: null }, + saveProviderIds: false, + }; + + preferencesService.getGlobal.mockResolvedValue(prefs); + resolver.resolve.mockReturnValue(prefs); + resolver.withForwardCompatibility.mockReturnValue(prefs); + registry.all.mockReturnValue([{ key: MetadataProviderKey.GOOGLE }] as never); + fetchService.searchCandidates.mockReturnValue(of(candidate(MetadataProviderKey.GOOGLE, 'g1', { genres: ['romance', 'FANTASY'] }))); + + const { resolved, sources } = await pipeline.runWithSources({ title: 'Query' }, { genres: ['Romance', 'Fantasy'] }); + + expect(resolved.genres).toBeUndefined(); + expect(sources.genres).toBeUndefined(); + }); + + it('does not trim stored genres when they already exceed the configured maximum', async () => { + const prefs = createPreferences((fields) => { + fields.genres = { + enabled: true, + providers: [MetadataProviderKey.GOOGLE], + mergeStrategy: 'mergeExisting', + }; + }); + prefs.options = { + genres: { mode: 'firstProvider', blocklist: [], maxCount: 2 }, + saveProviderIds: false, + }; + + preferencesService.getGlobal.mockResolvedValue(prefs); + resolver.resolve.mockReturnValue(prefs); + resolver.withForwardCompatibility.mockReturnValue(prefs); + registry.all.mockReturnValue([{ key: MetadataProviderKey.GOOGLE }] as never); + fetchService.searchCandidates.mockReturnValue(of(candidate(MetadataProviderKey.GOOGLE, 'g1', { genres: ['New Genre'] }))); + + const { resolved } = await pipeline.runWithSources({ title: 'Query' }, { genres: ['One', 'Two', 'Three'] }); + + expect(resolved.genres).toBeUndefined(); + }); + it('filters blocklisted genres before merging selected providers', async () => { const prefs = createPreferences((fields) => { fields.genres = { diff --git a/server/src/modules/metadata-fetch/metadata-fetch-pipeline.ts b/server/src/modules/metadata-fetch/metadata-fetch-pipeline.ts index c8cc89f0d..96933231b 100644 --- a/server/src/modules/metadata-fetch/metadata-fetch-pipeline.ts +++ b/server/src/modules/metadata-fetch/metadata-fetch-pipeline.ts @@ -8,6 +8,7 @@ import { MetadataFetchDiagnostics, MetadataFetchPreferences, MetadataField, + MetadataMergeStrategy, MetadataProviderKey, MetadataSeriesMembership, ProviderConfigurations, @@ -19,7 +20,7 @@ import { MetadataPreferenceResolver } from '../metadata-preferences/metadata-pre import { ProviderConfigService } from '../metadata-preferences/provider-config.service'; import { MetadataPreferencesService } from '../metadata-preferences/metadata-preferences.service'; import { SeriesExpectedCountService } from '../../common/services/series-expected-count.service'; -import { applyGenreFetchOptions, createGenreBlocklistTokenSet } from '../../common/utils/genre-fetch-options.utils'; +import { applyGenreFetchOptions, createGenreBlocklistTokenSet, mergeExistingGenres } from '../../common/utils/genre-fetch-options.utils'; import { normalizeMetadataText, normalizeMetadataTextKey } from '../../common/utils/metadata-text-normalize.utils'; import { sanitizeLogValue } from '../../common/utils/log-sanitize.utils'; import { resolveCandidateAgreement } from './candidate-agreement'; @@ -330,19 +331,10 @@ export class MetadataFetchPipeline { ); if (!genres.length) continue; - const existingValue = existing[field]; - switch (mergeStrategy) { - case 'fillMissing': - if (this.isMissing(existingValue)) { - result.genres = genres; - if (sourceProvider) sources.genres = sourceProvider; - } - break; - case 'overwrite': - case 'overwriteIfProvided': - result.genres = genres; - if (sourceProvider) sources.genres = sourceProvider; - break; + const resolvedGenres = this.resolveGenreWrite(genres, existing[field], mergeStrategy, genreOptions?.maxCount); + if (resolvedGenres) { + result.genres = resolvedGenres; + if (sourceProvider) sources.genres = sourceProvider; } continue; } @@ -363,7 +355,7 @@ export class MetadataFetchPipeline { const candidate = byProvider.get(providerKey); if (!candidate) continue; - let value = this.extractField(candidate, field); + const value = this.extractField(candidate, field); if (value === undefined || value === null) continue; if (mergeStrategy !== 'overwrite' && this.isEmptyProviderValue(value)) continue; @@ -379,7 +371,12 @@ export class MetadataFetchPipeline { if (!Array.isArray(value)) continue; const genres = applyGenreFetchOptions(value, blockedGenreTokens, genreOptions?.maxCount); if (!genres.length) continue; - value = genres; + const resolvedGenres = this.resolveGenreWrite(genres, existing[field], mergeStrategy, genreOptions?.maxCount); + if (resolvedGenres) { + result.genres = resolvedGenres; + sources.genres = providerKey; + } + break; } const existingValue = existing[field]; @@ -397,6 +394,8 @@ export class MetadataFetchPipeline { this.copyPublishedDateForYear(result, candidate, field); sources[field] = providerKey; break; + case 'mergeExisting': + break; } break; } @@ -560,6 +559,24 @@ export class MetadataFetchPipeline { return { genres: applyGenreFetchOptions(merged, blockedGenreTokens, maxCount), sourceProvider }; } + private resolveGenreWrite( + fetchedGenres: string[], + existingValue: unknown, + mergeStrategy: MetadataMergeStrategy, + maxCount: number | null | undefined, + ): string[] | undefined { + if (mergeStrategy === 'fillMissing') return this.isMissing(existingValue) ? fetchedGenres : undefined; + if (mergeStrategy !== 'mergeExisting') return fetchedGenres; + + const existingGenres = Array.isArray(existingValue) ? existingValue.filter((value): value is string => typeof value === 'string') : []; + const merged = mergeExistingGenres(existingGenres, fetchedGenres, maxCount); + return this.arraysEqual(merged, existingGenres) ? undefined : merged; + } + + private arraysEqual(left: readonly string[], right: readonly string[]): boolean { + return left.length === right.length && left.every((value, index) => value === right[index]); + } + private isMissing(value: unknown): boolean { if (value === null || value === undefined || value === '') return true; if (Array.isArray(value)) return value.length === 0; diff --git a/server/src/modules/metadata-preferences/dto/update-global-preferences.dto.test.ts b/server/src/modules/metadata-preferences/dto/update-global-preferences.dto.test.ts index 4e5542b02..03cc67c7d 100644 --- a/server/src/modules/metadata-preferences/dto/update-global-preferences.dto.test.ts +++ b/server/src/modules/metadata-preferences/dto/update-global-preferences.dto.test.ts @@ -31,6 +31,30 @@ describe('UpdateGlobalPreferencesDto', () => { expect(errors).toHaveLength(0); }); + it('accepts merge-with-existing for genres only', async () => { + const genreResult = await validateInput({ + fields: { + genres: { + enabled: true, + providers: [MetadataProviderKey.GOOGLE], + mergeStrategy: 'mergeExisting', + }, + }, + }); + const titleResult = await validateInput({ + fields: { + title: { + enabled: true, + providers: [MetadataProviderKey.GOOGLE], + mergeStrategy: 'mergeExisting', + }, + }, + }); + + expect(genreResult.errors).toHaveLength(0); + expect(titleResult.errors).not.toHaveLength(0); + }); + it('rejects non-object fields payloads', async () => { const { errors } = await validateInput({ fields: [] as unknown[] }); diff --git a/server/src/modules/metadata-preferences/dto/update-global-preferences.dto.ts b/server/src/modules/metadata-preferences/dto/update-global-preferences.dto.ts index 35b4f97ae..bc17019cc 100644 --- a/server/src/modules/metadata-preferences/dto/update-global-preferences.dto.ts +++ b/server/src/modules/metadata-preferences/dto/update-global-preferences.dto.ts @@ -16,8 +16,8 @@ import { validateSync, } from 'class-validator'; import { plainToInstance, Type } from 'class-transformer'; -import { ALL_METADATA_FIELDS, GENRE_MERGE_MODES, MAX_METADATA_GENRE_COUNT, MERGE_STRATEGIES, MetadataProviderKey } from '@bookorbit/types'; -import type { GenreMergeMode, MergeStrategy, MetadataField } from '@bookorbit/types'; +import { ALL_METADATA_FIELDS, GENRE_MERGE_MODES, GENRE_MERGE_STRATEGIES, MAX_METADATA_GENRE_COUNT, MetadataProviderKey } from '@bookorbit/types'; +import type { GenreMergeMode, MetadataField, MetadataMergeStrategy } from '@bookorbit/types'; const PROVIDER_KEYS = Object.values(MetadataProviderKey); export class FieldPreferenceDto { @@ -29,8 +29,8 @@ export class FieldPreferenceDto { @IsIn(PROVIDER_KEYS, { each: true }) providers!: MetadataProviderKey[]; - @IsIn(MERGE_STRATEGIES) - mergeStrategy!: MergeStrategy; + @IsIn(GENRE_MERGE_STRATEGIES) + mergeStrategy!: MetadataMergeStrategy; } export class GenreOptionsDto { @@ -69,6 +69,7 @@ export class IsFieldPreferencesMapConstraint implements ValidatorConstraintInter if (!knownFields.has(field)) return false; const instance = plainToInstance(FieldPreferenceDto, v); if (validateSync(instance).length > 0) return false; + if (field !== 'genres' && instance.mergeStrategy === 'mergeExisting') return false; } return true; } diff --git a/server/src/modules/metadata-preferences/metadata-preference-resolver.test.ts b/server/src/modules/metadata-preferences/metadata-preference-resolver.test.ts index a8dda1f7c..b426ad41e 100644 --- a/server/src/modules/metadata-preferences/metadata-preference-resolver.test.ts +++ b/server/src/modules/metadata-preferences/metadata-preference-resolver.test.ts @@ -74,6 +74,7 @@ describe('MetadataPreferenceResolver', () => { expect(defaults.fields.title.mergeStrategy).toBe('overwriteIfProvided'); expect(defaults.fields.description.mergeStrategy).toBe('overwriteIfProvided'); + expect(defaults.fields.genres.mergeStrategy).toBe('mergeExisting'); expect(defaults.options).toEqual({ genres: { mode: 'merge', blocklist: [], maxCount: null }, saveProviderIds: true, @@ -127,6 +128,22 @@ describe('MetadataPreferenceResolver', () => { expect(resolved.fields.title).toEqual(defaults.fields.title); }); + it('keeps merge-with-existing scoped to genres', () => { + const defaults = resolver.getDefaultPreferences(); + const preferences = { + fields: { + ...defaults.fields, + title: { ...defaults.fields.title, mergeStrategy: 'mergeExisting' }, + genres: { ...defaults.fields.genres, mergeStrategy: 'mergeExisting' }, + }, + } as MetadataFetchPreferences; + + const resolved = resolver.resolve(preferences, null); + + expect(resolved.fields.title.mergeStrategy).toBe('overwriteIfProvided'); + expect(resolved.fields.genres.mergeStrategy).toBe('mergeExisting'); + }); + it('normalizes malformed options to safe defaults', () => { const defaults = resolver.getDefaultPreferences(); const malformed = { diff --git a/server/src/modules/metadata-preferences/metadata-preference-resolver.ts b/server/src/modules/metadata-preferences/metadata-preference-resolver.ts index c520cf7f1..63c3cb8b1 100644 --- a/server/src/modules/metadata-preferences/metadata-preference-resolver.ts +++ b/server/src/modules/metadata-preferences/metadata-preference-resolver.ts @@ -4,11 +4,13 @@ import { COMMUNITY_RATING_PROVIDER_KEYS, FieldPreference, FieldPreferenceOverrides, + GENRE_MERGE_STRATEGIES, GENRE_MERGE_MODES, MAX_METADATA_GENRE_COUNT, MetadataFetchPreferences, MetadataFetchOptions, MetadataField, + MetadataMergeStrategy, MetadataProviderKey, MERGE_STRATEGIES, MergeStrategy, @@ -25,7 +27,8 @@ const DEFAULT_PROVIDER_ORDER: MetadataProviderKey[] = [ ]; const DEFAULT_MERGE_STRATEGY: MergeStrategy = 'overwriteIfProvided'; -const MERGE_STRATEGY_SET: Set = new Set(MERGE_STRATEGIES); +const MERGE_STRATEGY_SET = new Set(MERGE_STRATEGIES); +const GENRE_MERGE_STRATEGY_SET = new Set(GENRE_MERGE_STRATEGIES); const GENRE_MERGE_MODE_SET = new Set(GENRE_MERGE_MODES); const PROVIDERS_WITH_ITUNES: MetadataProviderKey[] = [ @@ -52,7 +55,10 @@ const FIELD_DEFAULTS: Partial>> = ], }, authors: { providers: PROVIDERS_WITH_ITUNES }, - genres: { providers: [MetadataProviderKey.GOODREADS, MetadataProviderKey.GOOGLE, MetadataProviderKey.ITUNES, MetadataProviderKey.KOBO] }, + genres: { + mergeStrategy: 'mergeExisting', + providers: [MetadataProviderKey.GOODREADS, MetadataProviderKey.GOOGLE, MetadataProviderKey.ITUNES, MetadataProviderKey.KOBO], + }, communityRating: { providers: [...COMMUNITY_RATING_PROVIDER_KEYS] }, }; @@ -85,7 +91,7 @@ export class MetadataPreferenceResolver { const fields = {} as Record; for (const field of ALL_METADATA_FIELDS) { const chosen = (libraryOverrides && libraryOverrides[field]) ?? global?.fields?.[field]; - fields[field] = this.normalizeFieldPreference(chosen, defaults.fields[field]); + fields[field] = this.normalizeFieldPreference(field, chosen, defaults.fields[field]); } const options = this.normalizeOptions(global?.options, defaults.options!); return { fields, options }; @@ -97,7 +103,7 @@ export class MetadataPreferenceResolver { const registered = new Set(registeredKeys); for (const field of ALL_METADATA_FIELDS) { const fallback = defaults.fields[field]; - const fp = this.normalizeFieldPreference(preferences?.fields?.[field], fallback); + const fp = this.normalizeFieldPreference(field, preferences?.fields?.[field], fallback); if (!registeredKeys.length) { fields[field] = fp; continue; @@ -114,7 +120,7 @@ export class MetadataPreferenceResolver { return { fields, options }; } - private normalizeFieldPreference(value: unknown, fallback: FieldPreference): FieldPreference { + private normalizeFieldPreference(field: MetadataField, value: unknown, fallback: FieldPreference): FieldPreference { if (typeof value !== 'object' || value === null || Array.isArray(value)) { return { ...fallback, providers: [...fallback.providers] }; } @@ -125,8 +131,9 @@ export class MetadataPreferenceResolver { Array.isArray(candidate.providers) && candidate.providers.every((p) => typeof p === 'string') ? [...candidate.providers] : [...fallback.providers]; - const mergeStrategy = MERGE_STRATEGY_SET.has(candidate.mergeStrategy as MergeStrategy) - ? (candidate.mergeStrategy as MergeStrategy) + const mergeStrategySet = field === 'genres' ? GENRE_MERGE_STRATEGY_SET : MERGE_STRATEGY_SET; + const mergeStrategy = mergeStrategySet.has(candidate.mergeStrategy as MetadataMergeStrategy) + ? (candidate.mergeStrategy as MetadataMergeStrategy) : fallback.mergeStrategy; return { enabled, providers, mergeStrategy };