From fc9742357b3111f032e34e37ae14a7ac703891d6 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 10 Aug 2026 16:08:36 +0200 Subject: [PATCH] One photo provider interface instead of three switches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dispatch to a photo backend was `switch (photo.provider)` in photo-resolver.service.ts twice and, in journey-public.controller.ts, an if/else that handed anything not named immich to Synology — where it 404'd from inside that provider's id parser. Adding a third backend meant finding all three. PhotoProviderRegistry.get(id) replaces them, over adapters registered under the PHOTO_PROVIDERS token in memories.module.ts. Adapters rather than `implements` on the services themselves: ImmichService and SynologyService are also the album and browse surfaces, and folding the interface into them would drag their argument orders and return shapes along. PhotoAssetRef is the part worth having. The two services took the same three values in different orders — Immich as (userId, assetId, ownerId), Synology as (userId, targetUserId, photoId) — so every dispatch site had to remember which was which, and a transposition would have compiled and served another user's photo. One place builds the ref, one place decrypts the Synology share passphrase. `local` stays an explicit branch at both dispatch sites: the bytes are on this disk, there is no row for it in photo_providers, and its asset info comes from the stored row rather than from a backend. The rename memories/ -> media/ is deliberately not here. It is import churn across 275 files, and coverage-thresholds.mjs buckets on one path segment, so merging photos/ in would collapse two ratchet entries into one and keep the looser of the two. --- server/src/nest/README.md | 21 +++ .../nest/journey/journey-public.controller.ts | 24 ++- server/src/nest/journey/journey.service.ts | 27 +++- server/src/nest/memories/memories.module.ts | 19 ++- .../nest/memories/photo-provider.registry.ts | 30 ++++ server/src/nest/memories/photo-provider.ts | 71 +++++++++ .../nest/memories/photo-resolver.service.ts | 120 +++++++------- .../memories/providers/immich.provider.ts | 42 +++++ .../memories/providers/synology.provider.ts | 33 ++++ .../unit/nest/journey.controller.test.ts | 40 +++-- server/tests/unit/nest/photo-provider.test.ts | 148 ++++++++++++++++++ .../unit/nest/photo-resolver.service.test.ts | 15 +- server/vitest.config.ts | 8 +- 13 files changed, 505 insertions(+), 93 deletions(-) create mode 100644 server/src/nest/memories/photo-provider.registry.ts create mode 100644 server/src/nest/memories/photo-provider.ts create mode 100644 server/src/nest/memories/providers/immich.provider.ts create mode 100644 server/src/nest/memories/providers/synology.provider.ts create mode 100644 server/tests/unit/nest/photo-provider.test.ts diff --git a/server/src/nest/README.md b/server/src/nest/README.md index b3025df75..939b544c1 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -124,6 +124,27 @@ tools, which never pass through an HTTP guard. In the five domains piloted for answer 404 to anonymous callers, so the addon check must beat the 401. - `common/demo-write.ts` — `isDemoWriteBlocked`, the demo-mode upload block. A function and not a guard on purpose; see the gotcha below. +- `memories/photo-provider.ts` + `providers/` — the PhotoProvider seam (#584). + Dispatch to a photo backend was a `switch (photo.provider)` in three places + (`photo-resolver.service.ts` twice, `journey-public.controller.ts` once, where + anything that was not Immich got handed to Synology). It is + `PhotoProviderRegistry.get(id)` now, over adapters registered under the + `PHOTO_PROVIDERS` multi-provider token in `memories.module.ts` — adding a + backend is one registration. + - Adapters, not `implements` on the services: ImmichService and + SynologyService are also the album/browse surfaces, with their own argument + orders and return shapes. The mapping lives in the adapter. + - `PhotoAssetRef` exists because the two services took the same three values + in different orders — `(userId, assetId, ownerId)` against + `(userId, targetUserId, photoId)`. A transposition compiled cleanly and + served another user's photo. + - `local` is deliberately NOT a provider. It is storage on this disk, it has + no row in `photo_providers`, and its "asset info" comes from the DB row + rather than from a backend. Both dispatch sites branch on it explicitly. + - The rename `memories/` → `media/` was considered and dropped: it is 275 + files of import churn, and `scripts/coverage-thresholds.mjs` buckets on one + path segment, so merging `photos/` into it would collapse two ratchet + entries into one and drop the stricter of the two. - `geo/` — the ONE Nominatim client (#576). There used to be two: atlas throttled to the published 1.1s and cached its answers, maps did neither, so five interactive routes fired straight at the service while the atlas loops diff --git a/server/src/nest/journey/journey-public.controller.ts b/server/src/nest/journey/journey-public.controller.ts index 4fb6c4a99..f31de1406 100644 --- a/server/src/nest/journey/journey-public.controller.ts +++ b/server/src/nest/journey/journey-public.controller.ts @@ -67,14 +67,22 @@ export class JourneyPublicController { } const effectiveOwnerId = valid.ownerId || Number(ownerId); - if (provider === 'immich') { - await this.journey.streamImmichAsset(res, effectiveOwnerId, assetId, wantThumb, effectiveOwnerId); - } else { - try { - await this.journey.streamSynologyAsset(res, effectiveOwnerId, effectiveOwnerId, assetId, wantThumb); - } catch { - res.status(404).json({ error: 'Provider not supported' }); - } + const streaming = this.journey.streamProviderAsset( + res, + provider, + { userId: effectiveOwnerId, ownerId: effectiveOwnerId, assetId }, + wantThumb, + ); + if (!streaming) { + res.status(404).json({ error: 'Provider not supported' }); + return; + } + try { + await streaming; + } catch { + // Kept: a provider that throws mid-parse answered 404 here before, and a + // share link that half-writes a response is worse than one that 404s. + res.status(404).json({ error: 'Provider not supported' }); } } } diff --git a/server/src/nest/journey/journey.service.ts b/server/src/nest/journey/journey.service.ts index 8ad32c6e7..e9921b673 100644 --- a/server/src/nest/journey/journey.service.ts +++ b/server/src/nest/journey/journey.service.ts @@ -3,7 +3,7 @@ import { DatabaseService } from '../database/database.service'; import { JourneyDomainService } from './journey-domain.service'; import { JourneyShareService } from './journey-share.service'; import { ImmichService } from '../memories/immich.service'; -import { SynologyService } from '../memories/synology.service'; +import { PhotoProviderRegistry } from '../memories/photo-provider.registry'; import { PhotoResolverService } from '../memories/photo-resolver.service'; import { AddonsService } from '../addons/addons.service'; import { ADDON_IDS } from '../../addons'; @@ -21,7 +21,7 @@ export class JourneyService { private readonly dbs: DatabaseService, private readonly addons: AddonsService, private readonly immich: ImmichService, - private readonly synology: SynologyService, + private readonly providers: PhotoProviderRegistry, private readonly photoResolver: PhotoResolverService, private readonly journey: JourneyDomainService, private readonly share: JourneyShareService, @@ -98,10 +98,23 @@ export class JourneyService { validateShareTokenForPhoto(token: string, photoId: number) { return this.share.validateShareTokenForPhoto(token, photoId); } validateShareTokenForAsset(token: string, assetId: string) { return this.share.validateShareTokenForAsset(token, assetId); } streamPhoto(res: Response, ownerId: number, photoId: number, kind: 'thumbnail' | 'original') { return this.photoResolver.streamPhoto(res, ownerId, photoId, kind); } - streamImmichAsset(res: Response, userId: number, assetId: string, kind: 'thumbnail' | 'original', ownerId: number) { return this.immich.streamImmichAsset(res, userId, assetId, kind, ownerId); } - // Was a dynamic import(): the memories cluster had a real import cycle and - // this was the workaround. The cycle is gone, so it is a plain injection now. - streamSynologyAsset(res: Response, userId: number, ownerId: number, assetId: string, kind: 'thumbnail' | 'original') { - return this.synology.streamSynologyAsset(res, userId, ownerId, assetId, kind); + /** + * Stream a shared journey asset from whichever backend holds it. + * + * Replaced a pair of per-provider passthroughs (and, before those, a dynamic + * import() around a since-removed cycle). The public controller used to pick + * between them with `provider === 'immich' ? … : trySynology()`, so a third + * provider would have been handed to Synology and 404'd from inside its + * parser. An unregistered provider is now told apart from a failing one. + */ + streamProviderAsset( + res: Response, + provider: string, + ref: { userId: number; ownerId: number; assetId: string }, + kind: 'thumbnail' | 'original', + ): Promise | null { + const backend = this.providers.get(provider); + if (!backend) return null; + return backend.streamAsset(res, ref, kind); } } diff --git a/server/src/nest/memories/memories.module.ts b/server/src/nest/memories/memories.module.ts index c32ffc509..9d4daac92 100644 --- a/server/src/nest/memories/memories.module.ts +++ b/server/src/nest/memories/memories.module.ts @@ -14,6 +14,10 @@ import { AddonsModule } from '../addons/addons.module'; import { AuditModule } from '../audit/audit.module'; import { TrekPhotosModule } from '../photos/trek-photos.module'; import { RealtimeModule } from '../realtime/realtime.module'; +import { PHOTO_PROVIDERS } from './photo-provider'; +import { PhotoProviderRegistry } from './photo-provider.registry'; +import { ImmichPhotoProvider } from './providers/immich.provider'; +import { SynologyPhotoProvider } from './providers/synology.provider'; /** * Memories (photo-providers) domain — mounted at /api/integrations/memories. @@ -29,6 +33,11 @@ import { RealtimeModule } from '../realtime/realtime.module'; * PhotoResolverService and MemoriesAccessService are exported because the * /api/photos surface and the journey domain resolve provider assets through * them. + * + * PHOTO_PROVIDERS is the multi-provider array behind PhotoProviderRegistry + * (#584): adding a photo backend means adding an adapter to this one list, not + * finding every `switch (photo.provider)`. Registered here rather than in the + * adapters themselves so the set is readable in one place. */ @Module({ imports: [AddonsModule, AuditModule, TrekPhotosModule, RealtimeModule], @@ -42,7 +51,15 @@ import { RealtimeModule } from '../realtime/realtime.module'; PhotoResolverService, ThumbnailService, TrekPhotoCacheService, + ImmichPhotoProvider, + SynologyPhotoProvider, + PhotoProviderRegistry, + { + provide: PHOTO_PROVIDERS, + useFactory: (immich: ImmichPhotoProvider, synology: SynologyPhotoProvider) => [immich, synology], + inject: [ImmichPhotoProvider, SynologyPhotoProvider], + }, ], - exports: [MemoriesAccessService, PhotoResolverService, ImmichService, SynologyService], + exports: [MemoriesAccessService, PhotoResolverService, ImmichService, SynologyService, PhotoProviderRegistry], }) export class MemoriesModule {} diff --git a/server/src/nest/memories/photo-provider.registry.ts b/server/src/nest/memories/photo-provider.registry.ts new file mode 100644 index 000000000..21b8e8beb --- /dev/null +++ b/server/src/nest/memories/photo-provider.registry.ts @@ -0,0 +1,30 @@ +import { Inject, Injectable } from '@nestjs/common'; +import { PHOTO_PROVIDERS, type PhotoProvider } from './photo-provider'; + +/** + * Where the `switch (photo.provider)` went (#584). + * + * Adding a backend is registering an adapter under PHOTO_PROVIDERS in + * memories.module.ts — no dispatch site changes, because there is one. + * Whether a registered provider is ENABLED is a separate question, answered by + * the photo_providers table in UnifiedMemoriesService; this only knows what the + * server can talk to at all. + */ +@Injectable() +export class PhotoProviderRegistry { + private readonly byId: Map; + + constructor(@Inject(PHOTO_PROVIDERS) providers: readonly PhotoProvider[]) { + this.byId = new Map(providers.map((p) => [p.id, p])); + } + + /** The provider for a stored photo's `provider` column, or undefined. */ + get(id: string | null | undefined): PhotoProvider | undefined { + return id ? this.byId.get(id) : undefined; + } + + /** Registered ids, for error messages and diagnostics. */ + ids(): string[] { + return [...this.byId.keys()]; + } +} diff --git a/server/src/nest/memories/photo-provider.ts b/server/src/nest/memories/photo-provider.ts new file mode 100644 index 000000000..a6fc80fd8 --- /dev/null +++ b/server/src/nest/memories/photo-provider.ts @@ -0,0 +1,71 @@ +import type { Response } from 'express'; +import type { AssetInfo, ServiceResult } from './memories.helpers'; + +/** What a caller can ask a provider for. */ +export type PhotoAssetKind = 'thumbnail' | 'original'; + +/** + * One asset, and everything a provider needs to reach it. + * + * The two shipped providers took the same three numbers in different orders — + * Immich as `(userId, assetId, ownerId)`, Synology as `(userId, targetUserId, + * photoId)` — so every dispatch site had to remember which was which, and a + * transposition would have compiled cleanly and served the wrong user's photo. + */ +export interface PhotoAssetRef { + /** Who is asking. */ + userId: number; + /** Whose credentials the provider is reached with — not always the caller. */ + ownerId: number; + assetId: string; + /** Synology share links carry their own passphrase; other providers ignore it. */ + passphrase?: string; + /** Lets a provider tell a video apart from an image without a second lookup. */ + mediaType?: string | null; + /** Forwarded Range header, for seeking in a video. */ + range?: string; +} + +export interface PhotoBytes { + bytes: Buffer; + contentType: string; +} + +export interface PhotoFetchError { + error: string; + status: number; +} + +/** + * A photo backend TREK can serve assets from (#584). + * + * The dispatch used to be a `switch (photo.provider)` in three places, which is + * what made "add a provider" a search-and-edit task rather than a registration. + * `local` is deliberately NOT one of these: it is storage on this disk, not an + * integration, and the `photo_providers` table it would have to appear in does + * not list it either. + */ +export interface PhotoProvider { + /** Matches `trek_photos.provider` and the `photo_providers` row id. */ + readonly id: string; + + /** + * Write the asset to the response, headers and status included. + * + * The error return is Immich's: it answers `{ error, status }` without + * touching the response when the owner has no credentials stored. Every + * caller has always ignored that — which leaves the request open until the + * client times out — so the shape is carried here rather than quietly + * dropped, and the pre-existing handling is left alone. + */ + streamAsset(res: Response, ref: PhotoAssetRef, kind: PhotoAssetKind): Promise; + + /** Thumbnail bytes for the cache, rather than straight to a response. */ + fetchThumbnailBytes(ref: PhotoAssetRef): Promise; + + /** Capture date, dimensions, place — whatever the backend knows. */ + getAssetInfo(ref: PhotoAssetRef): Promise>; +} + +/** Multi-provider injection token. Register an adapter here and it is reachable. */ +export const PHOTO_PROVIDERS = Symbol('PHOTO_PROVIDERS'); diff --git a/server/src/nest/memories/photo-resolver.service.ts b/server/src/nest/memories/photo-resolver.service.ts index 751cf9a27..bd3828c90 100644 --- a/server/src/nest/memories/photo-resolver.service.ts +++ b/server/src/nest/memories/photo-resolver.service.ts @@ -6,25 +6,29 @@ import type { TrekPhoto } from '../../types'; import { decrypt_api_key } from '../common/crypto/apiKeyCrypto'; import { TrekPhotosRepository } from '../photos/trek-photos.repository'; import { UPLOADS_ROOT } from './uploads-root'; -import { ImmichService } from './immich.service'; -import { SynologyService } from './synology.service'; import { ThumbnailService } from './thumbnail.service'; import { TrekPhotoCacheService } from './trek-photo-cache.service'; import { fail, success, type AssetInfo, type ServiceResult } from './memories.helpers'; +import { PhotoProviderRegistry } from './photo-provider.registry'; +import type { PhotoAssetRef } from './photo-provider'; /** * Resolves a stored trek_photo to bytes or metadata by asking whichever provider * owns it. The storage half of the old photoResolverService lives in * nest/photos/trek-photos.repository.ts; this is the dispatch half. + * + * It no longer knows WHICH providers exist (#584). It held ImmichService and + * SynologyService and a `switch` over their ids; it holds the registry now, so + * a third backend is a registration in memories.module.ts rather than a case + * added in two places here and a third in journey-public.controller.ts. */ @Injectable() export class PhotoResolverService { constructor( private readonly photos: TrekPhotosRepository, - private readonly immich: ImmichService, - private readonly synology: SynologyService, private readonly thumbnails: ThumbnailService, private readonly cache: TrekPhotoCacheService, + private readonly providers: PhotoProviderRegistry, ) {} // ── Streaming ──────────────────────────────────────────────────────────── @@ -114,39 +118,45 @@ export class PhotoResolverService { } } - switch (photo.provider) { - case 'local': { - res.status(404).json({ error: 'File not found' }); - return; - } - case 'immich': { - if (kind === 'thumbnail') { - await this.streamCachedThumbnail( - res, photo, - () => this.immich.fetchImmichThumbnailBytes(userId, photo.asset_id!, photo.owner_id!), - () => this.immich.streamImmichAsset(res, userId, photo.asset_id!, kind, photo.owner_id!, { mediaType: photo.media_type }), - ); - return; - } - await this.immich.streamImmichAsset(res, userId, photo.asset_id!, kind, photo.owner_id!, { mediaType: photo.media_type, range }); - return; - } - case 'synologyphotos': { - const passphrase = photo.passphrase ? (decrypt_api_key(photo.passphrase) || undefined) : undefined; - if (kind === 'thumbnail') { - await this.streamCachedThumbnail( - res, photo, - () => this.synology.fetchSynologyThumbnailBytes(userId, photo.owner_id!, photo.asset_id!, passphrase), - () => this.synology.streamSynologyAsset(res, userId, photo.owner_id!, photo.asset_id!, kind, undefined, passphrase), - ); - return; - } - await this.synology.streamSynologyAsset(res, userId, photo.owner_id!, photo.asset_id!, kind, undefined, passphrase); - return; - } - default: - res.status(400).json({ error: `Unknown provider: ${photo.provider}` }); + // 'local' is not a provider: the bytes are on this disk, the block above + // already tried to send them, and there is no backend left to ask. + if (photo.provider === 'local') { + res.status(404).json({ error: 'File not found' }); + return; } + + const provider = this.providers.get(photo.provider); + if (!provider) { + res.status(400).json({ error: `Unknown provider: ${photo.provider}` }); + return; + } + + const ref = this.refFor(photo, userId); + if (kind === 'thumbnail') { + await this.streamCachedThumbnail( + res, photo, + () => provider.fetchThumbnailBytes(ref), + // The fallback streams the thumbnail itself, so it carries no Range. + () => provider.streamAsset(res, ref, kind), + ); + return; + } + await provider.streamAsset(res, { ...ref, range }, kind); + } + + /** + * The stored row as a provider-shaped request. One place decrypts the + * Synology share passphrase, and one place decides which of the two user ids + * means "whose credentials". + */ + private refFor(photo: TrekPhoto, userId: number): PhotoAssetRef { + return { + userId, + ownerId: photo.owner_id!, + assetId: photo.asset_id!, + passphrase: photo.passphrase ? (decrypt_api_key(photo.passphrase) || undefined) : undefined, + mediaType: photo.media_type, + }; } // ── Asset Info ──────────────────────────────────────────────────────────── @@ -158,29 +168,21 @@ export class PhotoResolverService { const photo = this.photos.resolve(photoId); if (!photo) return fail('Photo not found', 404); - switch (photo.provider) { - case 'local': { - return success({ - id: String(photo.id), - takenAt: photo.created_at, - city: null, - country: null, - width: photo.width, - height: photo.height, - fileName: photo.file_path?.split('/').pop() || null, - } as AssetInfo); - } - case 'immich': { - const result = await this.immich.getAssetInfo(userId, photo.asset_id!, photo.owner_id!); - if (result.error) return fail(result.error, result.status || 500); - return success(result.data as AssetInfo); - } - case 'synologyphotos': { - const passphrase = photo.passphrase ? (decrypt_api_key(photo.passphrase) || undefined) : undefined; - return this.synology.getSynologyAssetInfo(userId, photo.asset_id!, photo.owner_id!, passphrase); - } - default: - return fail(`Unknown provider: ${photo.provider}`, 400); + // Local rows answer from the row itself — nothing to ask. + if (photo.provider === 'local') { + return success({ + id: String(photo.id), + takenAt: photo.created_at, + city: null, + country: null, + width: photo.width, + height: photo.height, + fileName: photo.file_path?.split('/').pop() || null, + } as AssetInfo); } + + const provider = this.providers.get(photo.provider); + if (!provider) return fail(`Unknown provider: ${photo.provider}`, 400); + return provider.getAssetInfo(this.refFor(photo, userId)); } } diff --git a/server/src/nest/memories/providers/immich.provider.ts b/server/src/nest/memories/providers/immich.provider.ts new file mode 100644 index 000000000..b8a4572da --- /dev/null +++ b/server/src/nest/memories/providers/immich.provider.ts @@ -0,0 +1,42 @@ +import { Injectable } from '@nestjs/common'; +import type { Response } from 'express'; +import { ImmichService } from '../immich.service'; +import { fail, success, type AssetInfo, type ServiceResult } from '../memories.helpers'; +import type { + PhotoAssetKind, PhotoAssetRef, PhotoBytes, PhotoFetchError, PhotoProvider, +} from '../photo-provider'; + +/** + * Immich behind the PhotoProvider interface. + * + * An adapter rather than `implements` on ImmichService itself: that service is + * also the album/browse surface the controllers use, with its own argument + * order and its own `{ data?, error?, status? }` return shape. Mapping here + * keeps the interface narrow and leaves the service's callers untouched. + */ +@Injectable() +export class ImmichPhotoProvider implements PhotoProvider { + readonly id = 'immich'; + + constructor(private readonly immich: ImmichService) {} + + async streamAsset(res: Response, ref: PhotoAssetRef, kind: PhotoAssetKind): Promise { + const result = await this.immich.streamImmichAsset(res, ref.userId, ref.assetId, kind, ref.ownerId, { + mediaType: ref.mediaType, + range: ref.range, + }); + // The service types both fields optional; the one path that returns at all + // always fills them. Normalised here so the interface can require them. + if (result && result.error) return { error: result.error, status: result.status ?? 500 }; + } + + fetchThumbnailBytes(ref: PhotoAssetRef): Promise { + return this.immich.fetchImmichThumbnailBytes(ref.userId, ref.assetId, ref.ownerId); + } + + async getAssetInfo(ref: PhotoAssetRef): Promise> { + const result = await this.immich.getAssetInfo(ref.userId, ref.assetId, ref.ownerId); + if (result.error) return fail(result.error, result.status || 500); + return success(result.data as AssetInfo); + } +} diff --git a/server/src/nest/memories/providers/synology.provider.ts b/server/src/nest/memories/providers/synology.provider.ts new file mode 100644 index 000000000..4717755ff --- /dev/null +++ b/server/src/nest/memories/providers/synology.provider.ts @@ -0,0 +1,33 @@ +import { Injectable } from '@nestjs/common'; +import type { Response } from 'express'; +import { SynologyService } from '../synology.service'; +import type { AssetInfo, ServiceResult } from '../memories.helpers'; +import type { + PhotoAssetKind, PhotoAssetRef, PhotoBytes, PhotoFetchError, PhotoProvider, +} from '../photo-provider'; + +/** + * Synology Photos behind the PhotoProvider interface. + * + * The `size` parameter stays unset, as every caller left it. The passphrase is + * the one thing this provider needs that Immich does not: a shared album link + * carries its own, and the caller decrypts it before handing it over. + */ +@Injectable() +export class SynologyPhotoProvider implements PhotoProvider { + readonly id = 'synologyphotos'; + + constructor(private readonly synology: SynologyService) {} + + streamAsset(res: Response, ref: PhotoAssetRef, kind: PhotoAssetKind): Promise { + return this.synology.streamSynologyAsset(res, ref.userId, ref.ownerId, ref.assetId, kind, undefined, ref.passphrase); + } + + fetchThumbnailBytes(ref: PhotoAssetRef): Promise { + return this.synology.fetchSynologyThumbnailBytes(ref.userId, ref.ownerId, ref.assetId, ref.passphrase); + } + + getAssetInfo(ref: PhotoAssetRef): Promise> { + return this.synology.getSynologyAssetInfo(ref.userId, ref.assetId, ref.ownerId, ref.passphrase); + } +} diff --git a/server/tests/unit/nest/journey.controller.test.ts b/server/tests/unit/nest/journey.controller.test.ts index e892e6619..194179dc4 100644 --- a/server/tests/unit/nest/journey.controller.test.ts +++ b/server/tests/unit/nest/journey.controller.test.ts @@ -297,10 +297,12 @@ describe('JourneyPublicController', () => { it('legacy photo proxy: 404 invalid token, immich path streams', async () => { expect(await thrownAsync(() => new JourneyPublicController(svc({ validateShareTokenForAsset: vi.fn().mockReturnValue(null) } as Partial)).legacyPhoto('tok', 'immich', 'a1', '2', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); - const streamImmichAsset = vi.fn().mockResolvedValue(undefined); - const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamImmichAsset } as Partial); + // One call for every provider now, with the ids in a ref instead of in a + // per-provider argument order. + const streamProviderAsset = vi.fn().mockResolvedValue(undefined); + const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamProviderAsset } as Partial); await new JourneyPublicController(s).legacyPhoto('tok', 'immich', 'a1', '2', 'original', {} as Response); - expect(streamImmichAsset).toHaveBeenCalledWith({}, 5, 'a1', 'original', 5); + expect(streamProviderAsset).toHaveBeenCalledWith({}, 'immich', { userId: 5, ownerId: 5, assetId: 'a1' }, 'original'); }); it('photo proxy streams thumbnails too', async () => { @@ -311,25 +313,39 @@ describe('JourneyPublicController', () => { }); it('legacy photo proxy: synology streams, and a failure becomes a 404 json', async () => { - const streamSynologyAsset = vi.fn().mockResolvedValue(undefined); - const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamSynologyAsset } as Partial); - await new JourneyPublicController(s).legacyPhoto('tok', 'synology', 'a1', '2', 'thumbnail', {} as Response); - expect(streamSynologyAsset).toHaveBeenCalledWith({}, 5, 5, 'a1', 'thumbnail'); + const streamProviderAsset = vi.fn().mockResolvedValue(undefined); + const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamProviderAsset } as Partial); + await new JourneyPublicController(s).legacyPhoto('tok', 'synologyphotos', 'a1', '2', 'thumbnail', {} as Response); + expect(streamProviderAsset).toHaveBeenCalledWith({}, 'synologyphotos', { userId: 5, ownerId: 5, assetId: 'a1' }, 'thumbnail'); const status = vi.fn().mockReturnThis(); const json = vi.fn(); const res = { status, json } as unknown as Response; - const failing = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 0 }), streamSynologyAsset: vi.fn().mockRejectedValue(new Error('no synology')) } as Partial); - await new JourneyPublicController(failing).legacyPhoto('tok', 'synology', 'a1', '6', 'original', res); + const failing = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 0 }), streamProviderAsset: vi.fn().mockRejectedValue(new Error('no synology')) } as Partial); + await new JourneyPublicController(failing).legacyPhoto('tok', 'synologyphotos', 'a1', '6', 'original', res); + expect(status).toHaveBeenCalledWith(404); + expect(json).toHaveBeenCalledWith({ error: 'Provider not supported' }); + }); + + it('legacy photo proxy: an unregistered provider 404s instead of being handed to synology', async () => { + // The old if/else sent everything that was not immich to synology, which + // 404'd from inside its own id parser. The registry can tell "no such + // backend" apart from "that backend failed". + const streamProviderAsset = vi.fn().mockReturnValue(null); + const status = vi.fn().mockReturnThis(); + const json = vi.fn(); + const res = { status, json } as unknown as Response; + const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamProviderAsset } as Partial); + await new JourneyPublicController(s).legacyPhoto('tok', 'photoprism', 'a1', '2', 'original', res); expect(status).toHaveBeenCalledWith(404); expect(json).toHaveBeenCalledWith({ error: 'Provider not supported' }); }); it('legacy photo proxy: falls back to the path ownerId when the token has none', async () => { - const streamImmichAsset = vi.fn().mockResolvedValue(undefined); - const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 0 }), streamImmichAsset } as Partial); + const streamProviderAsset = vi.fn().mockResolvedValue(undefined); + const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 0 }), streamProviderAsset } as Partial); await new JourneyPublicController(s).legacyPhoto('tok', 'immich', 'a1', '8', 'original', {} as Response); - expect(streamImmichAsset).toHaveBeenCalledWith({}, 8, 'a1', 'original', 8); + expect(streamProviderAsset).toHaveBeenCalledWith({}, 'immich', { userId: 8, ownerId: 8, assetId: 'a1' }, 'original'); }); it('legacy photo proxy: local provider 404s when the resolved file does not exist', async () => { diff --git a/server/tests/unit/nest/photo-provider.test.ts b/server/tests/unit/nest/photo-provider.test.ts new file mode 100644 index 000000000..280c5c42d --- /dev/null +++ b/server/tests/unit/nest/photo-provider.test.ts @@ -0,0 +1,148 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +/** + * The PhotoProvider seam (#584). + * + * What is worth pinning here is not that a lookup works — it is the two things + * the old `switch (photo.provider)` could not express. First, that a backend + * arrives by registration rather than by editing three dispatch sites. Second, + * that the adapters put the caller and the credential owner in the right slots: + * Immich took `(userId, assetId, ownerId)` and Synology `(userId, targetUserId, + * photoId)`, so a transposition would have compiled and served another user's + * photo. + */ +import { PhotoProviderRegistry } from '../../../src/nest/memories/photo-provider.registry'; +import { ImmichPhotoProvider } from '../../../src/nest/memories/providers/immich.provider'; +import { SynologyPhotoProvider } from '../../../src/nest/memories/providers/synology.provider'; +import type { ImmichService } from '../../../src/nest/memories/immich.service'; +import type { SynologyService } from '../../../src/nest/memories/synology.service'; +import type { PhotoAssetRef, PhotoProvider } from '../../../src/nest/memories/photo-provider'; +import type { Response } from 'express'; + +const immich = { + streamImmichAsset: vi.fn(), + fetchImmichThumbnailBytes: vi.fn(), + getAssetInfo: vi.fn(), +}; +const synology = { + streamSynologyAsset: vi.fn(), + fetchSynologyThumbnailBytes: vi.fn(), + getSynologyAssetInfo: vi.fn(), +}; + +const immichProvider = new ImmichPhotoProvider(immich as unknown as ImmichService); +const synologyProvider = new SynologyPhotoProvider(synology as unknown as SynologyService); + +// Caller and credential owner deliberately differ: a shared album is reached +// with the OWNER's credentials on behalf of the viewer. +const ref: PhotoAssetRef = { + userId: 7, + ownerId: 42, + assetId: 'asset-1', + passphrase: 'share-pw', + mediaType: 'image', + range: 'bytes=0-99', +}; + +const res = {} as Response; + +beforeEach(() => { + vi.clearAllMocks(); + immich.streamImmichAsset.mockResolvedValue(undefined); + synology.streamSynologyAsset.mockResolvedValue(undefined); +}); + +describe('PhotoProviderRegistry', () => { + it('PROV-001: resolves a stored photo\'s provider column', () => { + const registry = new PhotoProviderRegistry([immichProvider, synologyProvider]); + expect(registry.get('immich')).toBe(immichProvider); + expect(registry.get('synologyphotos')).toBe(synologyProvider); + expect(registry.ids().sort()).toEqual(['immich', 'synologyphotos']); + }); + + it('PROV-002: answers undefined for local, null and an unknown id', () => { + const registry = new PhotoProviderRegistry([immichProvider, synologyProvider]); + // 'local' is storage, not a backend, and never gets a row in photo_providers. + expect(registry.get('local')).toBeUndefined(); + expect(registry.get(null)).toBeUndefined(); + expect(registry.get(undefined)).toBeUndefined(); + expect(registry.get('photoprism')).toBeUndefined(); + }); + + it('PROV-003: a new backend arrives by registration, with no dispatch site touched', () => { + const photoprism: PhotoProvider = { + id: 'photoprism', + streamAsset: vi.fn(), + fetchThumbnailBytes: vi.fn(), + getAssetInfo: vi.fn(), + }; + const registry = new PhotoProviderRegistry([immichProvider, synologyProvider, photoprism]); + expect(registry.get('photoprism')).toBe(photoprism); + }); +}); + +describe('ImmichPhotoProvider', () => { + it('PROV-004: maps the ref onto the service\'s (userId, assetId, ownerId) order', async () => { + await immichProvider.streamAsset(res, ref, 'original'); + expect(immich.streamImmichAsset).toHaveBeenCalledWith(res, 7, 'asset-1', 'original', 42, { + mediaType: 'image', + range: 'bytes=0-99', + }); + + await immichProvider.fetchThumbnailBytes(ref); + expect(immich.fetchImmichThumbnailBytes).toHaveBeenCalledWith(7, 'asset-1', 42); + }); + + it('PROV-005: normalises the credential-less answer, which the service types as optional', async () => { + immich.streamImmichAsset.mockResolvedValue({ error: 'Not found', status: 404 }); + expect(await immichProvider.streamAsset(res, ref, 'original')).toEqual({ error: 'Not found', status: 404 }); + + immich.streamImmichAsset.mockResolvedValue({ error: 'Broken' }); + expect(await immichProvider.streamAsset(res, ref, 'original')).toEqual({ error: 'Broken', status: 500 }); + }); + + it('PROV-006: a written response reports nothing', async () => { + immich.streamImmichAsset.mockResolvedValue(undefined); + expect(await immichProvider.streamAsset(res, ref, 'thumbnail')).toBeUndefined(); + }); + + it('PROV-007: turns the service\'s {data,error} into a ServiceResult', async () => { + immich.getAssetInfo.mockResolvedValue({ data: { id: 'a' } }); + expect(await immichProvider.getAssetInfo(ref)).toEqual({ success: true, data: { id: 'a' } }); + + immich.getAssetInfo.mockResolvedValue({ error: 'gone', status: 404 }); + expect(await immichProvider.getAssetInfo(ref)).toEqual({ + success: false, + error: { message: 'gone', status: 404 }, + }); + + // No status from the service is a server error, not a silent success. + immich.getAssetInfo.mockResolvedValue({ error: 'gone' }); + expect(await immichProvider.getAssetInfo(ref)).toEqual({ + success: false, + error: { message: 'gone', status: 500 }, + }); + }); +}); + +describe('SynologyPhotoProvider', () => { + it('PROV-008: maps the ref onto the service\'s (userId, ownerId, assetId) order', async () => { + await synologyProvider.streamAsset(res, ref, 'thumbnail'); + // size stays undefined, as every caller left it. + expect(synology.streamSynologyAsset).toHaveBeenCalledWith(res, 7, 42, 'asset-1', 'thumbnail', undefined, 'share-pw'); + + await synologyProvider.fetchThumbnailBytes(ref); + expect(synology.fetchSynologyThumbnailBytes).toHaveBeenCalledWith(7, 42, 'asset-1', 'share-pw'); + }); + + it('PROV-009: passes the share passphrase to getAssetInfo, in its own argument order', async () => { + synology.getSynologyAssetInfo.mockResolvedValue({ data: { id: 'a' } }); + await synologyProvider.getAssetInfo(ref); + expect(synology.getSynologyAssetInfo).toHaveBeenCalledWith(7, 'asset-1', 42, 'share-pw'); + }); + + it('PROV-010: forwards an absent passphrase as undefined rather than an empty string', async () => { + await synologyProvider.streamAsset(res, { userId: 1, ownerId: 1, assetId: 'x' }, 'original'); + expect(synology.streamSynologyAsset).toHaveBeenCalledWith(res, 1, 1, 'x', 'original', undefined, undefined); + }); +}); diff --git a/server/tests/unit/nest/photo-resolver.service.test.ts b/server/tests/unit/nest/photo-resolver.service.test.ts index 6f4b15c78..9b443c413 100644 --- a/server/tests/unit/nest/photo-resolver.service.test.ts +++ b/server/tests/unit/nest/photo-resolver.service.test.ts @@ -40,6 +40,9 @@ import type { ImmichService } from '../../../src/nest/memories/immich.service'; import type { SynologyService } from '../../../src/nest/memories/synology.service'; import type { ThumbnailService } from '../../../src/nest/memories/thumbnail.service'; import type { TrekPhotoCacheService } from '../../../src/nest/memories/trek-photo-cache.service'; +import { PhotoProviderRegistry } from '../../../src/nest/memories/photo-provider.registry'; +import { ImmichPhotoProvider } from '../../../src/nest/memories/providers/immich.provider'; +import { SynologyPhotoProvider } from '../../../src/nest/memories/providers/synology.provider'; const immich = { streamImmichAsset: vi.fn(), @@ -63,12 +66,20 @@ const cache = { const dbs = new DatabaseService(testDb); const repo = new TrekPhotosRepository(dbs); +// Real adapters over the stubbed services, and a real registry: the cases below +// keep asserting on immich.streamImmichAsset/synology.streamSynologyAsset, so +// they now also pin the adapters' argument mapping — which is where the two +// providers' differing parameter orders used to be a transposition waiting to +// happen. +const providers = new PhotoProviderRegistry([ + new ImmichPhotoProvider(immich as unknown as ImmichService), + new SynologyPhotoProvider(synology as unknown as SynologyService), +]); const svc = new PhotoResolverService( repo, - immich as unknown as ImmichService, - synology as unknown as SynologyService, thumbnails as unknown as ThumbnailService, cache as unknown as TrekPhotoCacheService, + providers, ); function makeRes() { diff --git a/server/vitest.config.ts b/server/vitest.config.ts index 624d3e7bd..7e7924a63 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -62,7 +62,7 @@ export default defineConfig({ // only covers what sits directly under src/nest (app.module.ts and the like). thresholds: { 'src/nest/accommodations/**/*.ts': { statements: 92, branches: 87, functions: 99, lines: 97 }, - 'src/nest/addons/**/*.ts': { statements: 99, branches: 87, functions: 99, lines: 99 }, + 'src/nest/addons/**/*.ts': { statements: 99, branches: 89, functions: 99, lines: 99 }, 'src/nest/admin/**/*.ts': { statements: 86, branches: 70, functions: 89, lines: 88 }, 'src/nest/airports/**/*.ts': { statements: 68, branches: 60, functions: 93, lines: 73 }, 'src/nest/app-config/**/*.ts': { statements: 79, branches: 100, functions: 65, lines: 71 }, @@ -88,10 +88,10 @@ export default defineConfig({ 'src/nest/health/**/*.ts': { statements: 99, branches: 65, functions: 99, lines: 99 }, 'src/nest/help/**/*.ts': { statements: 81, branches: 70, functions: 99, lines: 86 }, 'src/nest/integrations/**/*.ts': { statements: 72, branches: 56, functions: 76, lines: 74 }, - 'src/nest/journey/**/*.ts': { statements: 90, branches: 84, functions: 83, lines: 91 }, + 'src/nest/journey/**/*.ts': { statements: 90, branches: 84, functions: 84, lines: 91 }, 'src/nest/llm-parse/**/*.ts': { statements: 91, branches: 85, functions: 85, lines: 94 }, 'src/nest/maps/**/*.ts': { statements: 93, branches: 86, functions: 97, lines: 96 }, - 'src/nest/memories/**/*.ts': { statements: 88, branches: 80, functions: 95, lines: 90 }, + 'src/nest/memories/**/*.ts': { statements: 88, branches: 81, functions: 96, lines: 90 }, 'src/nest/notifications/**/*.ts': { statements: 83, branches: 72, functions: 84, lines: 86 }, 'src/nest/oauth/**/*.ts': { statements: 96, branches: 95, functions: 97, lines: 97 }, 'src/nest/oidc/**/*.ts': { statements: 88, branches: 83, functions: 91, lines: 92 }, @@ -109,7 +109,7 @@ export default defineConfig({ 'src/nest/query-helpers/**/*.ts': { statements: 90, branches: 75, functions: 99, lines: 92 }, 'src/nest/realtime/**/*.ts': { statements: 99, branches: 100, functions: 99, lines: 99 }, 'src/nest/reservation-import/**/*.ts': { statements: 61, branches: 55, functions: 41, lines: 61 }, - 'src/nest/reservations/**/*.ts': { statements: 92, branches: 83, functions: 93, lines: 95 }, + 'src/nest/reservations/**/*.ts': { statements: 92, branches: 83, functions: 95, lines: 96 }, 'src/nest/settings/**/*.ts': { statements: 85, branches: 68, functions: 99, lines: 87 }, 'src/nest/share/**/*.ts': { statements: 97, branches: 87, functions: 99, lines: 99 }, 'src/nest/system-notices/**/*.ts': { statements: 99, branches: 99, functions: 99, lines: 99 },