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 },