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);