From 790dd3b728cd9f63a8ed64d1eaf9569435a077e2 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 8 Aug 2026 12:18:40 +0200 Subject: [PATCH] refactor(server): break the memories import cycle immich -> unified -> photoResolver -> immich was a real cycle, every edge a value import; journeyService only got past it with a dynamic import(). Nothing in that cluster can become a provider while it stands, because Nest would need a forwardRef through three modules. It had exactly one cause: syncAlbumAssets and syncSynologyAlbumLink called addTripPhotos, which lives in unifiedService. Both functions are two things stapled together - fetch the album from the provider, then write it to the trip - and only the first half is provider-specific. So the providers keep the half they own (collectAlbumSelection, collectSynologyAlbumSelection) and unifiedService keeps the half it owns (syncImmichAlbum, syncSynologyAlbum). Every error string, status code and return shape is the one the combined function produced, including the per-provider wording the split had to preserve ('Could not reach Immich' stays in immich). The plan called for pulling addTripPhotos and getOrCreateTrekPhoto into one neutral module instead. Together they do not work as a pair: such a module still needs immich and synology to fetch, so the cycle only moves. There is a second cut that works - photoResolverService is really two files, and its trek_photos CRUD half touches no provider, so splitting that out also opens both loops with less churn. It is not the one taken here because it leaves immich -> unified standing, i.e. a provider depending on the trip aggregate. That direction is what tied the knot in the first place. The CRUD split is still worth doing, but as the photos/ vs memories/ module boundary, not as the cycle fix. server/src/services/memories/ is a DAG now: helpers <- immich/synology <- photoResolver <- unified. --- server/src/nest/memories/memories.service.ts | 8 ++-- server/src/services/memories/immichService.ts | 29 ++++++------ .../src/services/memories/synologyService.ts | 18 ++++---- .../src/services/memories/unifiedService.ts | 46 +++++++++++++++++++ server/tests/e2e/memories.e2e.test.ts | 13 ++++-- .../tests/unit/nest/memories.service.test.ts | 10 ++-- 6 files changed, 87 insertions(+), 37 deletions(-) diff --git a/server/src/nest/memories/memories.service.ts b/server/src/nest/memories/memories.service.ts index f02e0ffd0..414e8956b 100644 --- a/server/src/nest/memories/memories.service.ts +++ b/server/src/nest/memories/memories.service.ts @@ -6,6 +6,8 @@ import { createTripAlbumLink, removeAlbumLink, addTripPhotos, + syncImmichAlbum, + syncSynologyAlbum, removeTripPhoto, setTripPhotoSharing, } from '../../services/memories/unifiedService'; @@ -20,7 +22,6 @@ import { streamImmichAsset, listAlbums, getAlbumPhotos, - syncAlbumAssets, getAssetInfo, isValidAssetId, } from '../../services/memories/immichService'; @@ -31,7 +32,6 @@ import { testSynologyConnection, listSynologyAlbums, getSynologyAlbumPhotos, - syncSynologyAlbumLink, searchSynologyPhotos, getSynologyAssetInfo, streamSynologyAsset, @@ -140,7 +140,7 @@ export class MemoriesService { } immichSyncAlbumAssets(tripId: string, linkId: string, userId: number, sid: string) { - return syncAlbumAssets(tripId, linkId, userId, sid); + return syncImmichAlbum(tripId, linkId, userId, sid); } // ── Synology ──────────────────────────────────────────────────────────────── @@ -169,7 +169,7 @@ export class MemoriesService { } synologySyncAlbumLink(userId: number, tripId: string, linkId: string, sid: string) { - return syncSynologyAlbumLink(userId, tripId, linkId, sid); + return syncSynologyAlbum(userId, tripId, linkId, sid); } synologySearchPhotos(userId: number, from: string | undefined, to: string | undefined, offset: number, limit: number) { diff --git a/server/src/services/memories/immichService.ts b/server/src/services/memories/immichService.ts index 717d3c830..b3cdef105 100644 --- a/server/src/services/memories/immichService.ts +++ b/server/src/services/memories/immichService.ts @@ -3,7 +3,6 @@ import { db } from '../../db/database'; import { maybe_encrypt_api_key, decrypt_api_key } from '../../nest/common/crypto/apiKeyCrypto'; import { checkSsrf, safeFetch } from '../../utils/ssrfGuard'; import { writeAudit } from '../../nest/audit/audit.bridge'; -import { addTripPhotos} from './unifiedService'; import { getAlbumIdFromLink, updateSyncTimeForAlbumLink, Selection, pipeAsset } from './helpersService'; // ── Credentials ──────────────────────────────────────────────────────────── @@ -489,12 +488,21 @@ export async function getAlbumPhotos( } } -export async function syncAlbumAssets( +/** + * The Immich half of an album sync: resolve the link, fetch the album, and hand + * back the picked asset ids. + * + * Adding them to the trip stays with the unified service, which owns + * addTripPhotos. Splitting it there is what breaks the import cycle this module + * used to close (immich -> unified -> photoResolver -> immich): the provider now + * only ever points downhill. Every error string and status below is the one the + * combined function returned. + */ +export async function collectAlbumSelection( tripId: string, linkId: string, userId: number, - sid: string, -): Promise<{ success?: boolean; added?: number; total?: number; error?: string; status?: number }> { +): Promise<{ selection: Selection; total: number } | { error: string; status: number }> { const response = getAlbumIdFromLink(tripId, linkId, userId); if (!response.success) return { error: 'Album link not found', status: 404 }; @@ -508,17 +516,10 @@ export async function syncAlbumAssets( // visibility, so a stored hidden asset is a permanently broken tile (#1474). const assets = albumResult.assets.filter((a: any) => a.type === 'IMAGE' && isVisibleAsset(a)); - const selection: Selection = { - provider: 'immich', - asset_ids: assets.map((a: any) => a.id), + return { + selection: { provider: 'immich', asset_ids: assets.map((a: any) => a.id) }, + total: assets.length, }; - - const result = await addTripPhotos(tripId, userId, true, [selection], sid, linkId); - if ('error' in result) return { error: result.error.message, status: result.error.status }; - - updateSyncTimeForAlbumLink(linkId); - - return { success: true, added: result.data.added, total: assets.length }; } catch { return { error: 'Could not reach Immich', status: 502 }; } diff --git a/server/src/services/memories/synologyService.ts b/server/src/services/memories/synologyService.ts index 39dbf1636..b0228ea39 100644 --- a/server/src/services/memories/synologyService.ts +++ b/server/src/services/memories/synologyService.ts @@ -3,7 +3,6 @@ import { Response } from 'express'; import { db } from '../../db/database'; import { decrypt_api_key, encrypt_api_key, maybe_encrypt_api_key } from '../../nest/common/crypto/apiKeyCrypto'; import { safeFetch, SsrfBlockedError, checkSsrf } from '../../utils/ssrfGuard'; -import { addTripPhotos } from './unifiedService'; import { getAlbumLinkForSync, updateSyncTimeForAlbumLink, @@ -510,9 +509,13 @@ export async function getSynologyAlbumPhotos(userId: number, albumId: string, pa return success({ assets, total: assets.length, hasMore: false }); } -export async function syncSynologyAlbumLink(userId: number, tripId: string, linkId: string, sid: string): Promise> { +/** + * The Synology half of an album sync — see collectAlbumSelection in + * immichService for why the trip-side write lives in the unified service now. + */ +export async function collectSynologyAlbumSelection(userId: number, tripId: string, linkId: string): Promise> { const response = getAlbumLinkForSync(tripId, linkId, userId); - if (!response.success) return response as ServiceResult; + if (!response.success) return response as ServiceResult<{ selection: Selection; total: number }>; const { albumId, passphrase } = response.data; @@ -527,7 +530,7 @@ export async function syncSynologyAlbumLink(userId: number, tripId: string, link const result = await _requestSynologyApi<{ list: SynologyPhotoItem[] }>(userId, itemParams); - if (!result.success) return result as ServiceResult; + if (!result.success) return result as ServiceResult<{ selection: Selection; total: number }>; const items = result.data.list || []; allItems.push(...items); @@ -541,12 +544,7 @@ export async function syncSynologyAlbumLink(userId: number, tripId: string, link passphrase, }; - const result = await addTripPhotos(tripId, userId, true, [selection], sid, linkId); - if (!result.success) return result as ServiceResult; - - updateSyncTimeForAlbumLink(linkId); - - return success({ added: result.data.added, total: allItems.length }); + return success({ selection, total: allItems.length }); } export async function searchSynologyPhotos(userId: number, from?: string, to?: string, offset = 0, limit = 300): Promise> { diff --git a/server/src/services/memories/unifiedService.ts b/server/src/services/memories/unifiedService.ts index 668ed1a0e..46f4cf63c 100644 --- a/server/src/services/memories/unifiedService.ts +++ b/server/src/services/memories/unifiedService.ts @@ -3,12 +3,16 @@ import { send } from '../../nest/notifications/notifications.bridge'; import { broadcast } from '../../websocket'; import { ServiceResult, + SyncAlbumResult, fail, success, mapDbError, Selection, + updateSyncTimeForAlbumLink, } from './helpersService'; import { getOrCreateTrekPhoto, deleteTrekPhotoIfOrphan } from './photoResolverService'; +import { collectAlbumSelection } from './immichService'; +import { collectSynologyAlbumSelection } from './synologyService'; import { encrypt_api_key } from '../../nest/common/crypto/apiKeyCrypto'; @@ -313,3 +317,45 @@ async function _notifySharedTripPhotos( return fail('Failed to send notifications', 500); } } + +// ── Album sync (orchestration) ──────────────────────────────────────────── +// +// The provider services collect the asset ids; adding them to the trip and +// stamping the link is this module's job, because addTripPhotos lives here. +// Keeping the write on this side is what lets immich/synology stop importing +// this module — the cycle immich -> unified -> photoResolver -> immich is gone. +// Both functions return exactly what the combined provider-side ones returned. + +export async function syncImmichAlbum( + tripId: string, + linkId: string, + userId: number, + sid: string, +): Promise<{ success?: boolean; added?: number; total?: number; error?: string; status?: number }> { + const collected = await collectAlbumSelection(tripId, linkId, userId); + if ('error' in collected) return { error: collected.error, status: collected.status }; + + const result = await addTripPhotos(tripId, userId, true, [collected.selection], sid, linkId); + if ('error' in result) return { error: result.error.message, status: result.error.status }; + + updateSyncTimeForAlbumLink(linkId); + + return { success: true, added: result.data.added, total: collected.total }; +} + +export async function syncSynologyAlbum( + userId: number, + tripId: string, + linkId: string, + sid: string, +): Promise> { + const collected = await collectSynologyAlbumSelection(userId, tripId, linkId); + if (!collected.success) return collected as ServiceResult; + + const result = await addTripPhotos(tripId, userId, true, [collected.data.selection], sid, linkId); + if (!result.success) return result as ServiceResult; + + updateSyncTimeForAlbumLink(linkId); + + return success({ added: result.data.added, total: collected.data.total }); +} diff --git a/server/tests/e2e/memories.e2e.test.ts b/server/tests/e2e/memories.e2e.test.ts index 4e604e3db..de2697960 100644 --- a/server/tests/e2e/memories.e2e.test.ts +++ b/server/tests/e2e/memories.e2e.test.ts @@ -35,17 +35,20 @@ const { unified, immich, synology } = vi.hoisted(() => ({ unified: { listTripPhotos: vi.fn(), addTripPhotos: vi.fn(), setTripPhotoSharing: vi.fn(), removeTripPhoto: vi.fn(), listTripAlbumLinks: vi.fn(), createTripAlbumLink: vi.fn(), removeAlbumLink: vi.fn(), + // The album-sync orchestration moved here from the provider modules when the + // immich -> unified import cycle was broken. + syncImmichAlbum: vi.fn(), syncSynologyAlbum: vi.fn(), }, immich: { getConnectionSettings: vi.fn(), saveImmichSettings: vi.fn(), setImmichAutoUpload: vi.fn(), testConnection: vi.fn(), getConnectionStatus: vi.fn(), browseTimeline: vi.fn(), searchPhotos: vi.fn(), - streamImmichAsset: vi.fn(), listAlbums: vi.fn(), getAlbumPhotos: vi.fn(), syncAlbumAssets: vi.fn(), + streamImmichAsset: vi.fn(), listAlbums: vi.fn(), getAlbumPhotos: vi.fn(), collectAlbumSelection: vi.fn(), getAssetInfo: vi.fn(), isValidAssetId: vi.fn(), }, synology: { getSynologySettings: vi.fn(), updateSynologySettings: vi.fn(), getSynologyStatus: vi.fn(), testSynologyConnection: vi.fn(), listSynologyAlbums: vi.fn(), getSynologyAlbumPhotos: vi.fn(), - syncSynologyAlbumLink: vi.fn(), searchSynologyPhotos: vi.fn(), getSynologyAssetInfo: vi.fn(), + collectSynologyAlbumSelection: vi.fn(), searchSynologyPhotos: vi.fn(), getSynologyAssetInfo: vi.fn(), streamSynologyAsset: vi.fn(), }, })); @@ -290,12 +293,12 @@ describe('Memories e2e (real auth guard + temp SQLite)', () => { }); it('200 album sync (POST stays 200) / 404 envelope', async () => { - immich.syncAlbumAssets.mockResolvedValue({ success: true, added: 3, total: 10 }); + unified.syncImmichAlbum.mockResolvedValue({ success: true, added: 3, total: 10 }); const ok = await request(server).post(`${IMMICH}/trips/5/album-links/7/sync`).set('Cookie', sessionCookie(1)); expect(ok.status).toBe(200); expect(ok.body).toEqual({ success: true, added: 3, total: 10 }); - immich.syncAlbumAssets.mockResolvedValue({ error: 'Album link not found', status: 404 }); + unified.syncImmichAlbum.mockResolvedValue({ error: 'Album link not found', status: 404 }); const bad = await request(server).post(`${IMMICH}/trips/5/album-links/9/sync`).set('Cookie', sessionCookie(1)); expect(bad.status).toBe(404); expect(bad.body).toEqual({ error: 'Album link not found' }); @@ -369,7 +372,7 @@ describe('Memories e2e (real auth guard + temp SQLite)', () => { }); it('200 album sync (POST stays 200)', async () => { - synology.syncSynologyAlbumLink.mockResolvedValue({ success: true, data: { added: 2, total: 5 } }); + unified.syncSynologyAlbum.mockResolvedValue({ success: true, data: { added: 2, total: 5 } }); const res = await request(server).post(`${SYNO}/trips/5/album-links/7/sync`).set('Cookie', sessionCookie(1)); expect(res.status).toBe(200); expect(res.body).toEqual({ added: 2, total: 5 }); diff --git a/server/tests/unit/nest/memories.service.test.ts b/server/tests/unit/nest/memories.service.test.ts index 96b8da91a..ad8f3b1c2 100644 --- a/server/tests/unit/nest/memories.service.test.ts +++ b/server/tests/unit/nest/memories.service.test.ts @@ -10,6 +10,8 @@ const unified = vi.hoisted(() => ({ createTripAlbumLink: vi.fn(() => ({ data: {} })), removeAlbumLink: vi.fn(() => ({ data: {} })), addTripPhotos: vi.fn(async () => ({ data: { added: 0 } })), + syncImmichAlbum: vi.fn(async () => ({ success: true, added: 0, total: 0 })), + syncSynologyAlbum: vi.fn(async () => ({ success: true, data: { added: 0, total: 0 } })), removeTripPhoto: vi.fn(() => ({ data: {} })), setTripPhotoSharing: vi.fn(async () => ({ data: {} })), })); @@ -26,7 +28,7 @@ const immich = vi.hoisted(() => ({ streamImmichAsset: vi.fn(async () => undefined), listAlbums: vi.fn(async () => ({ albums: [] })), getAlbumPhotos: vi.fn(async () => ({ assets: [] })), - syncAlbumAssets: vi.fn(async () => ({ added: 0, total: 0 })), + collectAlbumSelection: vi.fn(async () => ({ selection: { provider: 'immich', asset_ids: [] }, total: 0 })), getAssetInfo: vi.fn(async () => ({ data: {} })), isValidAssetId: vi.fn(() => true), })); @@ -39,7 +41,7 @@ const synology = vi.hoisted(() => ({ testSynologyConnection: vi.fn(async () => ({ success: true, data: {} })), listSynologyAlbums: vi.fn(async () => ({ success: true, data: {} })), getSynologyAlbumPhotos: vi.fn(async () => ({ success: true, data: {} })), - syncSynologyAlbumLink: vi.fn(async () => ({ success: true, data: {} })), + collectSynologyAlbumSelection: vi.fn(async () => ({ success: true, data: { selection: { provider: 'synologyphotos', asset_ids: [] }, total: 0 } })), searchSynologyPhotos: vi.fn(async () => ({ success: true, data: {} })), getSynologyAssetInfo: vi.fn(async () => ({ success: true, data: {} })), streamSynologyAsset: vi.fn(async () => undefined), @@ -146,7 +148,7 @@ describe('MemoriesService (delegation wrapper over services/memories/*)', () => expect(immich.getAlbumPhotos).toHaveBeenCalledWith(7, 'al1'); await svc.immichSyncAlbumAssets('5', 'l1', 7, 'sock'); - expect(immich.syncAlbumAssets).toHaveBeenCalledWith('5', 'l1', 7, 'sock'); + expect(unified.syncImmichAlbum).toHaveBeenCalledWith('5', 'l1', 7, 'sock'); }); it('synology methods delegate', async () => { @@ -166,7 +168,7 @@ describe('MemoriesService (delegation wrapper over services/memories/*)', () => expect(synology.listSynologyAlbums).toHaveBeenCalledWith(7); await svc.synologySyncAlbumLink(7, '5', 'l1', 'sock'); - expect(synology.syncSynologyAlbumLink).toHaveBeenCalledWith(7, '5', 'l1', 'sock'); + expect(unified.syncSynologyAlbum).toHaveBeenCalledWith(7, '5', 'l1', 'sock'); await svc.synologySearchPhotos(7, 'f', 't', 0, 100); expect(synology.searchSynologyPhotos).toHaveBeenCalledWith(7, 'f', 't', 0, 100);