From 939c2ec3e853188ecd66d644d81ae3931672caad Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 17 Aug 2026 06:15:01 +0200 Subject: [PATCH] refactor(memories): immich upload reads through storage; drop uploads-root --- server/src/nest/memories/immich.service.ts | 18 ++++--- server/src/nest/memories/uploads-root.ts | 14 ----- server/tests/unit/nest/immich.service.test.ts | 52 ++++++++++++++++++- .../unit/nest/storage/storage-keys.test.ts | 7 +-- server/tests/unit/nest/uploads-root.test.ts | 17 ------ 5 files changed, 66 insertions(+), 42 deletions(-) delete mode 100644 server/src/nest/memories/uploads-root.ts delete mode 100644 server/tests/unit/nest/uploads-root.test.ts diff --git a/server/src/nest/memories/immich.service.ts b/server/src/nest/memories/immich.service.ts index c52f71449..b78a3d14b 100644 --- a/server/src/nest/memories/immich.service.ts +++ b/server/src/nest/memories/immich.service.ts @@ -3,10 +3,12 @@ import type { Response } from 'express'; import { maybe_encrypt_api_key, decrypt_api_key } from '../common/crypto/apiKeyCrypto'; import { checkSsrf, safeFetch } from '../../utils/ssrfGuard'; import { AuditService } from '../audit/audit.service'; +import { StorageService } from '../storage/storage.service'; +import fsPromises from 'node:fs/promises'; +import path from 'node:path'; import { DatabaseService } from '../database/database.service'; import { MemoriesAccessService } from './memories-access.service'; import { fail, handleServiceResult, pipeAsset, type Selection } from './memories.helpers'; -import { UPLOADS_ROOT } from './uploads-root'; const ALBUM_PAGE_SIZE = 1000; const ALBUM_MAX_PAGES = 20; @@ -24,6 +26,7 @@ export class ImmichService { private readonly db: DatabaseService, private readonly audit: AuditService, private readonly access: MemoriesAccessService, + private readonly storage: StorageService, ) {} getImmichCredentials(userId: number) { @@ -570,14 +573,15 @@ export class ImmichService { const creds = this.getImmichCredentials(userId); if (!creds) return null; - const fs = await import('node:fs'); - const path = await import('node:path'); - - const fullPath = path.join(UPLOADS_ROOT, filePath); - if (!fs.existsSync(fullPath)) return null; + // Journey uploads store the uploads-relative 'journey/' path; only + // those are local storage objects this can push. + const name = filePath.startsWith('journey/') ? filePath.slice('journey/'.length) : null; + if (!name) return null; try { - const fileBuffer = fs.readFileSync(fullPath); + // A missing object throws StorageNotFoundError into the catch below — + // the same null the old existsSync guard answered. + const fileBuffer = await this.storage.withLocalFile('journey', name, (p) => fsPromises.readFile(p)); const boundary = '----ImmichUpload' + Date.now(); const ext = path.extname(fileName).toLowerCase(); const mimeTypes: Record = { diff --git a/server/src/nest/memories/uploads-root.ts b/server/src/nest/memories/uploads-root.ts deleted file mode 100644 index b0b5d0492..000000000 --- a/server/src/nest/memories/uploads-root.ts +++ /dev/null @@ -1,14 +0,0 @@ -import path from 'node:path'; - -/** - * `/uploads`, the one anchor for everything the app stores on disk. - * - * `__dirname` moves with the file, and three separate copies of this join used - * to live in services/memories/ — every one of them a silent breakage waiting - * for the next move (multer would have created the wrong directory without a - * word). One constant, one test (UPLOADS-ROOT-001), and the depth is stated - * rather than counted by hand: `server/{src,dist}/nest/memories` sits three - * levels under `server/`, the same depth `services/memories/` sat at — so the - * resolved path is unchanged by the move. - */ -export const UPLOADS_ROOT = path.resolve(__dirname, '..', '..', '..', 'uploads'); diff --git a/server/tests/unit/nest/immich.service.test.ts b/server/tests/unit/nest/immich.service.test.ts index 4493c5f39..91d01f04d 100644 --- a/server/tests/unit/nest/immich.service.test.ts +++ b/server/tests/unit/nest/immich.service.test.ts @@ -46,11 +46,15 @@ import { DatabaseService } from '../../../src/nest/database/database.service'; import { ImmichService } from '../../../src/nest/memories/immich.service'; import type { AuditService } from '../../../src/nest/audit/audit.service'; import type { MemoriesAccessService } from '../../../src/nest/memories/memories-access.service'; +import fs from 'node:fs'; +import path from 'node:path'; +import { makeStorageFixture } from '../../helpers/storage-fixture'; const audit = { writeAudit: vi.fn() }; const access = { getAlbumIdFromLink: vi.fn() }; const dbs = new DatabaseService(testDb); -const svc = new ImmichService(dbs, audit as unknown as AuditService, access as unknown as MemoriesAccessService); +const journeyFx = makeStorageFixture('journey/'); +const svc = new ImmichService(dbs, audit as unknown as AuditService, access as unknown as MemoriesAccessService, journeyFx.storage); const USER = 1; @@ -478,3 +482,49 @@ describe('collectAlbumSelection', () => { expect(await svc.collectAlbumSelection('1', 'l1', USER)).toEqual({ error: 'Could not reach Immich', status: 502 }); }); }); + +describe('uploadToImmich', () => { + function writeJourneyObject(name: string, bytes: string): void { + const dir = path.join(journeyFx.root, 'journey'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, name), bytes); + } + + it('IMMICH-UP-001: returns null without credentials and never fetches', async () => { + expect(await svc.uploadToImmich(999, 'journey/pic.jpg', 'pic.jpg')).toBeNull(); + expect(safeFetch).not.toHaveBeenCalled(); + }); + + it('IMMICH-UP-002: returns null for a path outside the journey category', async () => { + seedUser(USER, 'https://immich.test', 'key-1'); + expect(await svc.uploadToImmich(USER, 'files/doc.pdf', 'doc.pdf')).toBeNull(); + expect(safeFetch).not.toHaveBeenCalled(); + }); + + it('IMMICH-UP-003: returns null when the object is missing (old existsSync guard)', async () => { + seedUser(USER, 'https://immich.test', 'key-1'); + expect(await svc.uploadToImmich(USER, 'journey/gone.jpg', 'gone.jpg')).toBeNull(); + expect(safeFetch).not.toHaveBeenCalled(); + }); + + it('IMMICH-UP-004: posts the bytes as multipart and answers the created asset id', async () => { + seedUser(USER, 'https://immich.test', 'key-1'); + writeJourneyObject('up.jpg', 'jpeg-bytes-here'); + safeFetch.mockResolvedValueOnce({ ok: true, json: async () => ({ id: 'immich-42' }) }); + + expect(await svc.uploadToImmich(USER, 'journey/up.jpg', 'up.jpg')).toBe('immich-42'); + + const [url, init] = safeFetch.mock.calls[0] as [string, { method: string; body: Buffer; headers: Record }]; + expect(url).toBe('https://immich.test/api/assets'); + expect(init.method).toBe('POST'); + expect(init.headers['x-api-key']).toBe('key-1'); + expect(init.body.includes(Buffer.from('jpeg-bytes-here'))).toBe(true); + }); + + it('IMMICH-UP-005: a rejected upstream answers null, not a throw', async () => { + seedUser(USER, 'https://immich.test', 'key-1'); + writeJourneyObject('rej.jpg', 'bytes'); + safeFetch.mockResolvedValueOnce({ ok: false, status: 500 }); + expect(await svc.uploadToImmich(USER, 'journey/rej.jpg', 'rej.jpg')).toBeNull(); + }); +}); diff --git a/server/tests/unit/nest/storage/storage-keys.test.ts b/server/tests/unit/nest/storage/storage-keys.test.ts index 910f44950..c6d2d6e90 100644 --- a/server/tests/unit/nest/storage/storage-keys.test.ts +++ b/server/tests/unit/nest/storage/storage-keys.test.ts @@ -72,9 +72,10 @@ describe('storage keys', () => { }); }); -// Depth contract for the storage domain's one __dirname anchor, same guard as -// tests/unit/nest/uploads-root.test.ts: the resolved defaults must equal the -// paths the server has always used, under both src (vitest) and dist layouts. +// Depth contract for the storage domain's one __dirname anchor (the guard the +// deleted memories/uploads-root.ts pin used to carry): the resolved defaults +// must equal the paths the server has always used, under both src (vitest) and +// dist layouts. describe('storage default paths', () => { it('anchors the default uploads root at /uploads', () => { expect(DEFAULT_UPLOADS_ROOT).toBe(path.resolve(process.cwd(), 'uploads')); diff --git a/server/tests/unit/nest/uploads-root.test.ts b/server/tests/unit/nest/uploads-root.test.ts deleted file mode 100644 index 769135be1..000000000 --- a/server/tests/unit/nest/uploads-root.test.ts +++ /dev/null @@ -1,17 +0,0 @@ -import { describe, it, expect } from 'vitest'; -import path from 'node:path'; -import { UPLOADS_ROOT } from '../../../src/nest/memories/uploads-root'; - -/** - * The anchor three separate `__dirname` joins used to duplicate. A wrong depth - * does not throw — multer and mkdirSync would happily create the wrong tree and - * every stored photo would land somewhere nobody serves from. - */ -describe('UPLOADS_ROOT', () => { - it('UPLOADS-ROOT-001: resolves to /uploads, whatever the process cwd is', () => { - // tests run from server/, so the repo-relative answer is unambiguous - expect(UPLOADS_ROOT).toBe(path.resolve(process.cwd(), 'uploads')); - expect(path.basename(UPLOADS_ROOT)).toBe('uploads'); - expect(path.basename(path.dirname(UPLOADS_ROOT))).toBe('server'); - }); -});