From 364f376b074cf0f663854c9c8e5cccee6421f101 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 17 Aug 2026 06:08:04 +0200 Subject: [PATCH] refactor(memories): thumbnail service category-addressed --- .../nest/memories/photo-resolver.service.ts | 5 +- server/src/nest/memories/thumbnail.service.ts | 54 ++++++++++++------- .../tests/unit/nest/thumbnail.service.test.ts | 38 +++++++------ 3 files changed, 60 insertions(+), 37 deletions(-) diff --git a/server/src/nest/memories/photo-resolver.service.ts b/server/src/nest/memories/photo-resolver.service.ts index c1d039f39..142435754 100644 --- a/server/src/nest/memories/photo-resolver.service.ts +++ b/server/src/nest/memories/photo-resolver.service.ts @@ -3,7 +3,6 @@ import { Response } from 'express'; 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 { ThumbnailService } from './thumbnail.service'; import { TrekPhotoCacheService } from './trek-photo-cache.service'; import { fail, success, type AssetInfo, type ServiceResult } from './memories.helpers'; @@ -90,9 +89,7 @@ export class PhotoResolverService { // Only raster images get a lazily-generated Jimp thumbnail; Jimp can't decode // video, so a video relies on the poster captured at upload (#823). if (!thumbRel && !isVideo) { - // Generation stays root-addressed until ThumbnailService becomes - // category-addressed in slice 4; serving below is already storage-backed. - const result = await this.thumbnails.ensureLocalThumbnail(UPLOADS_ROOT, photo.file_path); + const result = await this.thumbnails.ensureLocalThumbnail(photo.file_path); if (result) { thumbRel = result.thumbnailRelPath; this.photos.recordLocalThumbnail(photo.id, thumbRel, result.width, result.height); diff --git a/server/src/nest/memories/thumbnail.service.ts b/server/src/nest/memories/thumbnail.service.ts index d37197cee..f32abc30d 100644 --- a/server/src/nest/memories/thumbnail.service.ts +++ b/server/src/nest/memories/thumbnail.service.ts @@ -1,55 +1,73 @@ import { Jimp } from 'jimp' -import path from 'path' -import fs from 'fs/promises' -import crypto from 'crypto' +import path from 'node:path' +import crypto from 'node:crypto' import { Injectable } from '@nestjs/common' import { ADDON_IDS } from '../../addons' import { AddonsService } from '../addons/addons.service' +import { StorageService } from '../storage/storage.service' const THUMB_MAX = 800 const THUMB_QUALITY = 80 +/** + * Storage name (category 'journey') of the derived thumbnail for an + * uploads-relative journey photo path ('journey/'). The hash input is + * deliberately the uploads-relative path — the scheme predates the storage + * layer, and changing it would orphan every existing thumbnail. + */ +export function journeyThumbName(originalRelPath: string): string { + const hash = crypto.createHash('sha1').update(originalRelPath).digest('hex').slice(0, 16) + return `thumbs/${hash}.jpg` +} + /** * Downscaled JPEGs for locally uploaded journey photos. Gated on the journey - * addon, because that is the only surface that uploads them. + * addon, because that is the only surface that uploads them. Category-addressed + * since storage slice 4: originals and thumbs are ('journey', ) objects. */ @Injectable() export class ThumbnailService { - constructor(private readonly addons: AddonsService) {} + constructor( + private readonly addons: AddonsService, + private readonly storage: StorageService, + ) {} async ensureLocalThumbnail( - uploadsRoot: string, originalRelPath: string, ): Promise<{ thumbnailRelPath: string; width: number; height: number } | null> { if (!this.addons.isAddonEnabled(ADDON_IDS.JOURNEY)) return null - const originalAbs = path.join(uploadsRoot, originalRelPath) - try { await fs.access(originalAbs) } catch { return null } + // The DB stores uploads-relative paths ('journey/'); anything else + // is not a local journey photo and has no thumbnail to derive. + const origName = originalRelPath.startsWith('journey/') ? originalRelPath.slice('journey/'.length) : null + if (!origName) return null // Deterministic name so concurrent requests don't race on the same photo. - const hash = crypto.createHash('sha1').update(originalRelPath).digest('hex').slice(0, 16) - const thumbRel = `journey/thumbs/${hash}.jpg` - const thumbAbs = path.join(uploadsRoot, thumbRel) + const thumbName = journeyThumbName(originalRelPath) + const thumbRel = `journey/${thumbName}` try { const [srcStat, dstStat] = await Promise.all([ - fs.stat(originalAbs), - fs.stat(thumbAbs).catch(() => null), + this.storage.stat('journey', origName), + this.storage.stat('journey', thumbName), ]) + if (!srcStat) return null if (dstStat && dstStat.mtimeMs >= srcStat.mtimeMs) { - const img = await Jimp.read(thumbAbs) + const img = await this.storage.withLocalFile('journey', thumbName, (p) => Jimp.read(p)) return { thumbnailRelPath: thumbRel, width: img.bitmap.width, height: img.bitmap.height } } - await fs.mkdir(path.dirname(thumbAbs), { recursive: true }) - // Jimp auto-applies EXIF orientation on read, matching sharp's .rotate() behavior. - const img = await Jimp.read(originalAbs) + const img = await this.storage.withLocalFile('journey', origName, (p) => Jimp.read(p)) const { width: w, height: h } = img.bitmap if (w > THUMB_MAX || h > THUMB_MAX) { img.scaleToFit({ w: THUMB_MAX, h: THUMB_MAX }) } - await img.write(thumbAbs as `${string}.jpg`, { quality: THUMB_QUALITY }) + // Jimp writes straight to a path, so spool on the category's volume and + // commit through storage for the atomic rename. + const spool = path.join(this.storage.spoolDirFor('journey'), `thumb-${crypto.randomUUID()}.jpg`) + await img.write(spool as `${string}.jpg`, { quality: THUMB_QUALITY }) + await this.storage.put('journey', thumbName, { tmpPath: spool }) return { thumbnailRelPath: thumbRel, width: img.bitmap.width, height: img.bitmap.height } } catch { diff --git a/server/tests/unit/nest/thumbnail.service.test.ts b/server/tests/unit/nest/thumbnail.service.test.ts index b5006e204..303ac7e82 100644 --- a/server/tests/unit/nest/thumbnail.service.test.ts +++ b/server/tests/unit/nest/thumbnail.service.test.ts @@ -11,14 +11,18 @@ import { describe, it, expect, vi, beforeEach, afterAll } from 'vitest'; const { isAddonEnabled } = vi.hoisted(() => ({ isAddonEnabled: vi.fn(() => true) })); import fs from 'node:fs'; -import os from 'node:os'; import path from 'node:path'; import { Jimp } from 'jimp'; import { ThumbnailService } from '../../../src/nest/memories/thumbnail.service'; import type { AddonsService } from '../../../src/nest/addons/addons.service'; +import { makeStorageFixture } from '../../helpers/storage-fixture'; -const svc = new ThumbnailService({ isAddonEnabled } as unknown as AddonsService); -const root = fs.mkdtempSync(path.join(os.tmpdir(), 'trek-thumbs-')); +// Category-addressed since slice 4: originals + thumbs are ('journey', ) +// objects; the fixture's 'journey/' prefix reproduces the real layout, so the +// on-disk paths below look exactly like the old uploads-root ones. +const fx = makeStorageFixture('journey/'); +const svc = new ThumbnailService({ isAddonEnabled } as unknown as AddonsService, fx.storage); +const root = fx.root; /** A real 1200x900 JPEG — Jimp has to be able to decode it for the happy path. */ async function writeSourceImage(rel: string): Promise { @@ -34,23 +38,27 @@ beforeEach(() => { }); afterAll(() => { - fs.rmSync(root, { recursive: true, force: true }); + fx.cleanup(); }); describe('ensureLocalThumbnail', () => { it('THUMB-001: returns null when the journey addon is off, without touching the disk', async () => { isAddonEnabled.mockReturnValue(false); - expect(await svc.ensureLocalThumbnail(root, 'anything.jpg')).toBeNull(); + expect(await svc.ensureLocalThumbnail('anything.jpg')).toBeNull(); }); - it('THUMB-002: returns null when the source file does not exist', async () => { - expect(await svc.ensureLocalThumbnail(root, 'missing/nope.jpg')).toBeNull(); + it('THUMB-002: returns null when the source object does not exist', async () => { + expect(await svc.ensureLocalThumbnail('journey/missing.jpg')).toBeNull(); + }); + + it('THUMB-002b: returns null for a path outside the journey category', async () => { + expect(await svc.ensureLocalThumbnail('missing/nope.jpg')).toBeNull(); }); it('THUMB-003: downscales an oversized image and reports the resulting size', async () => { await writeSourceImage('journey/big.jpg'); - const result = await svc.ensureLocalThumbnail(root, 'journey/big.jpg'); + const result = await svc.ensureLocalThumbnail('journey/big.jpg'); expect(result).not.toBeNull(); expect(result!.thumbnailRelPath).toMatch(/^journey\/thumbs\/[0-9a-f]{16}\.jpg$/); @@ -62,19 +70,19 @@ describe('ensureLocalThumbnail', () => { it('THUMB-004: the path is deterministic, so concurrent requests cannot race on two names', async () => { await writeSourceImage('journey/stable.jpg'); - const first = await svc.ensureLocalThumbnail(root, 'journey/stable.jpg'); - const second = await svc.ensureLocalThumbnail(root, 'journey/stable.jpg'); + const first = await svc.ensureLocalThumbnail('journey/stable.jpg'); + const second = await svc.ensureLocalThumbnail('journey/stable.jpg'); expect(second!.thumbnailRelPath).toBe(first!.thumbnailRelPath); }); it('THUMB-005: reuses an existing thumbnail that is newer than the source', async () => { await writeSourceImage('journey/cached.jpg'); - const first = await svc.ensureLocalThumbnail(root, 'journey/cached.jpg'); + const first = await svc.ensureLocalThumbnail('journey/cached.jpg'); const thumbAbs = path.join(root, first!.thumbnailRelPath); const mtimeBefore = fs.statSync(thumbAbs).mtimeMs; - const second = await svc.ensureLocalThumbnail(root, 'journey/cached.jpg'); + const second = await svc.ensureLocalThumbnail('journey/cached.jpg'); expect(second).toEqual(first); expect(fs.statSync(thumbAbs).mtimeMs).toBe(mtimeBefore); @@ -82,13 +90,13 @@ describe('ensureLocalThumbnail', () => { it('THUMB-006: regenerates when the source is newer than the thumbnail', async () => { await writeSourceImage('journey/changed.jpg'); - const first = await svc.ensureLocalThumbnail(root, 'journey/changed.jpg'); + const first = await svc.ensureLocalThumbnail('journey/changed.jpg'); const thumbAbs = path.join(root, first!.thumbnailRelPath); // Age the thumbnail past the source. const old = new Date(Date.now() - 60_000); fs.utimesSync(thumbAbs, old, old); - const second = await svc.ensureLocalThumbnail(root, 'journey/changed.jpg'); + const second = await svc.ensureLocalThumbnail('journey/changed.jpg'); expect(second!.thumbnailRelPath).toBe(first!.thumbnailRelPath); expect(fs.statSync(thumbAbs).mtimeMs).toBeGreaterThan(old.getTime()); @@ -99,6 +107,6 @@ describe('ensureLocalThumbnail', () => { fs.mkdirSync(path.dirname(abs), { recursive: true }); fs.writeFileSync(abs, 'not an image at all'); - expect(await svc.ensureLocalThumbnail(root, 'journey/corrupt.jpg')).toBeNull(); + expect(await svc.ensureLocalThumbnail('journey/corrupt.jpg')).toBeNull(); }); });