From 1bf6517b75096b4e82054102be691ea6ccf8d313 Mon Sep 17 00:00:00 2001 From: jubnl Date: Sun, 16 Aug 2026 11:07:04 +0200 Subject: [PATCH] refactor(share): stream shared place-photo bytes through storage --- server/src/nest/share/share.controller.ts | 26 +++++-- server/src/nest/share/share.module.ts | 3 +- server/src/nest/share/share.service.ts | 17 ++--- .../tests/unit/nest/share.controller.test.ts | 72 +++++++++++-------- server/tests/unit/nest/share.service.test.ts | 32 ++++----- 5 files changed, 91 insertions(+), 59 deletions(-) diff --git a/server/src/nest/share/share.controller.ts b/server/src/nest/share/share.controller.ts index ee0429877..a8efd9a61 100644 --- a/server/src/nest/share/share.controller.ts +++ b/server/src/nest/share/share.controller.ts @@ -1,8 +1,9 @@ import { Body, Controller, Delete, Get, HttpException, Param, Post, Res, UseGuards } from '@nestjs/common'; import type { Response } from 'express'; -import { createReadStream } from 'node:fs'; +import type { Readable } from 'node:stream'; import type { User } from '../../types'; import { ShareService } from './share.service'; +import { StorageService } from '../storage/storage.service'; import { ShareLinkDto } from './share.dto'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; @@ -78,7 +79,10 @@ export class TripShareController { @Public('share-token validated: a shared trip link has to work for somebody without an account') @Controller('api/shared') export class SharedController { - constructor(private readonly share: ShareService) {} + constructor( + private readonly share: ShareService, + private readonly storage: StorageService, + ) {} /** * Public, token-scoped place-photo proxy. The shared payload rewrites place @@ -89,15 +93,25 @@ export class SharedController { * MapsController.placePhotoBytes (cached photos are always JPEG). */ @Get(':token/place-photo/:placeId/bytes') - placePhotoBytes(@Param('token') token: string, @Param('placeId') placeId: string, @Res() res: Response): void { - const fp = this.share.getSharedPlacePhotoPath(token, placeId); - if (!fp) { + async placePhotoBytes(@Param('token') token: string, @Param('placeId') placeId: string, @Res() res: Response): Promise { + const key = this.share.getSharedPlacePhotoKey(token, placeId); + if (!key) { res.status(404).json({ error: 'Photo not cached' }); return; } + // Bytes are stream-piped like the maps handler; headers go on before the + // stream attempt so an early failure overrides them exactly as the old + // createReadStream error event did. res.set('Cache-Control', 'public, max-age=2592000, immutable'); res.type('image/jpeg'); - const stream = createReadStream(fp); + let stream: Readable; + try { + ({ stream } = await this.storage.getStream('photos-google', key)); + } catch { + // Cache-delete race — same terminal state as the old stream error path. + if (!res.headersSent) res.status(404).json({ error: 'Photo not cached' }); + return; + } stream.on('error', () => { if (!res.headersSent) res.status(404).json({ error: 'Photo not cached' }); }); diff --git a/server/src/nest/share/share.module.ts b/server/src/nest/share/share.module.ts index 12e5afc6c..dbd991d10 100644 --- a/server/src/nest/share/share.module.ts +++ b/server/src/nest/share/share.module.ts @@ -6,11 +6,12 @@ import { SettingsModule } from '../settings/settings.module'; import { PermissionsModule } from '../permissions/permissions.module'; import { QueryHelpersModule } from '../query-helpers/query-helpers.module'; import { PlacePhotosModule } from '../place-photos/place-photos.module'; +import { StorageModule } from '../storage/storage.module'; import { AuthModule } from '../auth/auth.module'; import { McpSharedModule } from '../mcp-shared/mcp-shared.module'; @Module({ - imports: [McpSharedModule, SettingsModule, PermissionsModule, QueryHelpersModule, AuthModule, PlacePhotosModule], + imports: [McpSharedModule, SettingsModule, PermissionsModule, QueryHelpersModule, AuthModule, PlacePhotosModule, StorageModule], controllers: [TripShareController, SharedController], providers: [ShareService, ShareMcp], }) diff --git a/server/src/nest/share/share.service.ts b/server/src/nest/share/share.service.ts index 11aec174b..c6cb641bd 100644 --- a/server/src/nest/share/share.service.ts +++ b/server/src/nest/share/share.service.ts @@ -297,14 +297,15 @@ export class ShareService { } /** - * Resolves the on-disk path for a cached place photo requested through a public - * share link. Validates that the token is valid + unexpired and that the place - * actually belongs to that token's trip (matched via the stored proxy URL, which - * covers both Google `placeId` and Wikimedia `coords:` pseudo-IDs without - * depending on google_place_id). Returns null — never throws — so the caller - * answers a plain 404, mirroring the authenticated bytes endpoint. + * Resolves the storage name (category 'photos-google') for a cached place + * photo requested through a public share link. Validates that the token is + * valid + unexpired and that the place actually belongs to that token's trip + * (matched via the stored proxy URL, which covers both Google `placeId` and + * Wikimedia `coords:` pseudo-IDs without depending on google_place_id). + * Returns null — never throws — so the caller answers a plain miss, + * mirroring the authenticated bytes endpoint. */ - getSharedPlacePhotoPath(token: string, placeId: string): string | null { + getSharedPlacePhotoKey(token: string, placeId: string): string | null { const shareRow = this.dbs.get<{ trip_id: string; share_map: number }>( "SELECT trip_id, share_map FROM share_tokens WHERE token = ? AND (expires_at IS NULL OR expires_at > datetime('now'))", token, @@ -319,6 +320,6 @@ export class ShareService { const place = this.dbs.get('SELECT 1 FROM places WHERE trip_id = ? AND image_url = ?', shareRow.trip_id, expectedUrl); if (!place) return null; - return this.photoCache.serveFilePath(placeId); + return this.photoCache.serveKey(placeId); } } diff --git a/server/tests/unit/nest/share.controller.test.ts b/server/tests/unit/nest/share.controller.test.ts index a794a54e4..c867b2621 100644 --- a/server/tests/unit/nest/share.controller.test.ts +++ b/server/tests/unit/nest/share.controller.test.ts @@ -2,13 +2,15 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { HttpException } from '@nestjs/common'; import type { Response } from 'express'; -const { createReadStream } = vi.hoisted(() => ({ createReadStream: vi.fn() })); -vi.mock('node:fs', () => ({ createReadStream })); - import { TripShareController, SharedController } from '../../../src/nest/share/share.controller'; import type { ShareService } from '../../../src/nest/share/share.service'; +import type { StorageService } from '../../../src/nest/storage/storage.service'; import type { User } from '../../../src/types'; +// Only the shared place-photo proxy consumes storage. +const getStream = vi.fn(); +const storageStub = { getStream } as unknown as StorageService; + const user = { id: 1, role: 'user', email: 'u@example.test' } as User; function svc(o: Partial = {}): ShareService { @@ -74,8 +76,8 @@ describe('TripShareController', () => { describe('SharedController', () => { it('404 for an invalid token, else returns the snapshot', () => { - expect(thrown(() => new SharedController(svc({ getSharedTripData: vi.fn().mockReturnValue(null) } as Partial)).read('bad'))).toEqual({ status: 404, body: { error: 'Invalid or expired link' } }); - expect(new SharedController(svc({ getSharedTripData: vi.fn().mockReturnValue({ trip: { id: 9 } }) } as Partial)).read('tok')).toEqual({ trip: { id: 9 } }); + expect(thrown(() => new SharedController(svc({ getSharedTripData: vi.fn().mockReturnValue(null) } as Partial), storageStub).read('bad'))).toEqual({ status: 404, body: { error: 'Invalid or expired link' } }); + expect(new SharedController(svc({ getSharedTripData: vi.fn().mockReturnValue({ trip: { id: 9 } }) } as Partial), storageStub).read('tok')).toEqual({ trip: { id: 9 } }); }); describe('place-photo proxy', () => { @@ -91,49 +93,63 @@ describe('SharedController', () => { return r as unknown as Response & { status: ReturnType; json: ReturnType; set: ReturnType; type: ReturnType }; } - beforeEach(() => createReadStream.mockReset()); - - function controller(path: string | null) { - return new SharedController(svc({ getSharedPlacePhotoPath: vi.fn().mockReturnValue(path) } as Partial)); - } - - it('404 without streaming when the photo is not cached for the token', () => { - const res = photoRes(); - controller(null).placePhotoBytes('tok', 'p1', res); - expect(res.status).toHaveBeenCalledWith(404); - expect(res.json).toHaveBeenCalledWith({ error: 'Photo not cached' }); - expect(createReadStream).not.toHaveBeenCalled(); + // Braced body on purpose: mockReset() returns the mock, and a function + // returned from beforeEach is invoked as a teardown callback — which would + // call getStream() bare and turn a rejecting impl into an unhandled + // rejection. + beforeEach(() => { + getStream.mockReset(); }); - it('streams the cached file with image/jpeg + an immutable cache header on a hit', () => { - const stream = { on: vi.fn().mockReturnThis(), pipe: vi.fn() }; - createReadStream.mockReturnValue(stream); + function controller(key: string | null) { + return new SharedController(svc({ getSharedPlacePhotoKey: vi.fn().mockReturnValue(key) } as Partial), storageStub); + } + + it('404 without streaming when the photo is not cached for the token', async () => { const res = photoRes(); - controller('/cache/p1.jpg').placePhotoBytes('tok', 'p1', res); + await controller(null).placePhotoBytes('tok', 'p1', res); + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith({ error: 'Photo not cached' }); + expect(getStream).not.toHaveBeenCalled(); + }); + + it('streams the cached bytes with image/jpeg + an immutable cache header on a hit', async () => { + const stream = { on: vi.fn().mockReturnThis(), pipe: vi.fn() }; + getStream.mockResolvedValue({ stream, stat: { key: 'photos/google/abc.jpg', size: 5, mtimeMs: 0 } }); + const res = photoRes(); + await controller('abc.jpg').placePhotoBytes('tok', 'p1', res); expect(res.set).toHaveBeenCalledWith('Cache-Control', 'public, max-age=2592000, immutable'); expect(res.type).toHaveBeenCalledWith('image/jpeg'); - expect(createReadStream).toHaveBeenCalledWith('/cache/p1.jpg'); + expect(getStream).toHaveBeenCalledWith('photos-google', 'abc.jpg'); expect(stream.pipe).toHaveBeenCalledWith(res); }); - it('falls back to 404 when the read stream errors before headers were sent', () => { + it('falls back to 404 when the stream cannot be opened (cache-delete race)', async () => { + getStream.mockRejectedValue(new Error('storage object not found')); + const res = photoRes(); + await controller('abc.jpg').placePhotoBytes('tok', 'p1', res); + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith({ error: 'Photo not cached' }); + }); + + it('falls back to 404 when the stream errors before headers were sent', async () => { let onError: () => void = () => {}; const stream = { on: vi.fn((ev: string, cb: () => void) => { if (ev === 'error') onError = cb; return stream; }), pipe: vi.fn() }; - createReadStream.mockReturnValue(stream); + getStream.mockResolvedValue({ stream, stat: { key: 'photos/google/abc.jpg', size: 5, mtimeMs: 0 } }); const res = photoRes(); - controller('/cache/p1.jpg').placePhotoBytes('tok', 'p1', res); + await controller('abc.jpg').placePhotoBytes('tok', 'p1', res); onError(); expect(res.status).toHaveBeenCalledWith(404); expect(res.json).toHaveBeenCalledWith({ error: 'Photo not cached' }); }); - it('does not re-send a 404 when the stream errors after headers were flushed', () => { + it('does not re-send a 404 when the stream errors after headers were flushed', async () => { let onError: () => void = () => {}; const stream = { on: vi.fn((ev: string, cb: () => void) => { if (ev === 'error') onError = cb; return stream; }), pipe: vi.fn() }; - createReadStream.mockReturnValue(stream); + getStream.mockResolvedValue({ stream, stat: { key: 'photos/google/abc.jpg', size: 5, mtimeMs: 0 } }); const res = photoRes(); (res as { headersSent: boolean }).headersSent = true; - controller('/cache/p1.jpg').placePhotoBytes('tok', 'p1', res); + await controller('abc.jpg').placePhotoBytes('tok', 'p1', res); onError(); expect(res.status).not.toHaveBeenCalled(); expect(res.json).not.toHaveBeenCalled(); diff --git a/server/tests/unit/nest/share.service.test.ts b/server/tests/unit/nest/share.service.test.ts index eca53a484..e1e5fee64 100644 --- a/server/tests/unit/nest/share.service.test.ts +++ b/server/tests/unit/nest/share.service.test.ts @@ -45,8 +45,8 @@ const checkPermission = vi.fn(); const permissionsStub = { checkPermission } as unknown as PermissionsService; // Injected stub since the photo-cache fold (was a path mock of the module). -const serveFilePath = vi.fn(); -const photoCacheStub = { serveFilePath } as unknown as PlacePhotoCacheService; +const serveKey = vi.fn(); +const photoCacheStub = { serveKey } as unknown as PlacePhotoCacheService; import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; @@ -71,7 +71,7 @@ beforeAll(() => { beforeEach(() => { resetTestDb(testDb); checkPermission.mockReset(); - serveFilePath.mockReset(); + serveKey.mockReset(); }); afterAll(() => { @@ -377,41 +377,41 @@ describe('getSharedTripData', () => { }); }); -// ── getSharedPlacePhotoPath ─────────────────────────────────────────────────── +// ── getSharedPlacePhotoKey ─────────────────────────────────────────────────── -describe('getSharedPlacePhotoPath', () => { +describe('getSharedPlacePhotoKey', () => { it('SHARE-SVC-022: returns null for an unknown or expired token', () => { - expect(svc.getSharedPlacePhotoPath('nope', 'ChIJabc')).toBeNull(); + expect(svc.getSharedPlacePhotoKey('nope', 'ChIJabc')).toBeNull(); const { trip, token } = seedSharedTrip(); testDb.prepare('UPDATE share_tokens SET expires_at = ? WHERE trip_id = ?').run('2020-01-01T00:00:00.000Z', trip.id); - expect(svc.getSharedPlacePhotoPath(token, 'ChIJabc')).toBeNull(); - expect(serveFilePath).not.toHaveBeenCalled(); + expect(svc.getSharedPlacePhotoKey(token, 'ChIJabc')).toBeNull(); + expect(serveKey).not.toHaveBeenCalled(); }); it('SHARE-SVC-023: returns null when the owner disabled the map section', () => { const { trip, token } = seedSharedTrip({ share_map: false }); const place = createPlace(testDb, trip.id); testDb.prepare('UPDATE places SET image_url = ? WHERE id = ?').run('/api/maps/place-photo/ChIJabc/bytes', place.id); - expect(svc.getSharedPlacePhotoPath(token, 'ChIJabc')).toBeNull(); - expect(serveFilePath).not.toHaveBeenCalled(); + expect(svc.getSharedPlacePhotoKey(token, 'ChIJabc')).toBeNull(); + expect(serveKey).not.toHaveBeenCalled(); }); it('SHARE-SVC-024: returns null when no place in the trip carries the proxy URL', () => { const { token } = seedSharedTrip(); - expect(svc.getSharedPlacePhotoPath(token, 'ChIJabc')).toBeNull(); - expect(serveFilePath).not.toHaveBeenCalled(); + expect(svc.getSharedPlacePhotoKey(token, 'ChIJabc')).toBeNull(); + expect(serveKey).not.toHaveBeenCalled(); }); - it('SHARE-SVC-025: resolves via serveFilePath when the encoded placeId matches the stored URL', () => { + it('SHARE-SVC-025: resolves via serveKey when the encoded placeId matches the stored URL', () => { const { trip, token } = seedSharedTrip(); const place = createPlace(testDb, trip.id); // Wikimedia pseudo-IDs contain characters that must round-trip encoded. const placeId = 'coords:48.8,2.3'; testDb.prepare('UPDATE places SET image_url = ? WHERE id = ?') .run(`/api/maps/place-photo/${encodeURIComponent(placeId)}/bytes`, place.id); - serveFilePath.mockReturnValue('/cache/abc.jpg'); - expect(svc.getSharedPlacePhotoPath(token, placeId)).toBe('/cache/abc.jpg'); - expect(serveFilePath).toHaveBeenCalledWith(placeId); + serveKey.mockReturnValue('abc.jpg'); + expect(svc.getSharedPlacePhotoKey(token, placeId)).toBe('abc.jpg'); + expect(serveKey).toHaveBeenCalledWith(placeId); }); });