From ecec3d56fefab4f9fefc942b06dd9ed13dc7a7c3 Mon Sep 17 00:00:00 2001 From: Till JS Date: Fri, 20 Mar 2026 20:01:43 +0100 Subject: [PATCH] fix(contacts): optimize storage usage and fix photo orphan bug Bug fix: - ContactService.delete() now cleans up S3 photo before DB deletion (previously left orphaned files in storage) - ContactModule imports PhotoModule for dependency injection PhotoService improvements: - Use maxSizeBytes in upload() instead of manual validateFileSize() - Use getPublicUrl()/result.url instead of manual URL construction via getStorageConfig() + BUCKETS concatenation - Add cacheControl header for immutable photo assets (1 year) - Add upload hooks for structured logging via Logger - Add deletePhotoByUrl() for contact deletion cleanup - Add deleteAllUserPhotos() for account deletion via deleteByPrefix() - Store photos in 'photos' subfolder for cleaner key structure - Remove unused getStorageConfig/BUCKETS imports Test fix: - Add PhotoService mock to ContactService spec Co-Authored-By: Claude Opus 4.6 (1M context) --- .../contact/__tests__/contact.service.spec.ts | 5 + .../backend/src/contact/contact.module.ts | 2 + .../backend/src/contact/contact.service.ts | 9 +- .../apps/backend/src/photo/photo.service.ts | 97 ++++++++++--------- 4 files changed, 68 insertions(+), 45 deletions(-) diff --git a/apps/contacts/apps/backend/src/contact/__tests__/contact.service.spec.ts b/apps/contacts/apps/backend/src/contact/__tests__/contact.service.spec.ts index 6130c177d..0d0b0fc32 100644 --- a/apps/contacts/apps/backend/src/contact/__tests__/contact.service.spec.ts +++ b/apps/contacts/apps/backend/src/contact/__tests__/contact.service.spec.ts @@ -1,6 +1,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { NotFoundException } from '@nestjs/common'; import { ContactService } from '../contact.service'; +import { PhotoService } from '../../photo/photo.service'; import { DATABASE_CONNECTION } from '../../db/database.module'; describe('ContactService', () => { @@ -61,6 +62,10 @@ describe('ContactService', () => { provide: DATABASE_CONNECTION, useValue: mockDb, }, + { + provide: PhotoService, + useValue: { deletePhotoByUrl: jest.fn() }, + }, ], }).compile(); diff --git a/apps/contacts/apps/backend/src/contact/contact.module.ts b/apps/contacts/apps/backend/src/contact/contact.module.ts index 739d9bb8b..46b47e39c 100644 --- a/apps/contacts/apps/backend/src/contact/contact.module.ts +++ b/apps/contacts/apps/backend/src/contact/contact.module.ts @@ -1,8 +1,10 @@ import { Module } from '@nestjs/common'; import { ContactController } from './contact.controller'; import { ContactService } from './contact.service'; +import { PhotoModule } from '../photo/photo.module'; @Module({ + imports: [PhotoModule], controllers: [ContactController], providers: [ContactService], exports: [ContactService], diff --git a/apps/contacts/apps/backend/src/contact/contact.service.ts b/apps/contacts/apps/backend/src/contact/contact.service.ts index e57c9c6c4..d38abc921 100644 --- a/apps/contacts/apps/backend/src/contact/contact.service.ts +++ b/apps/contacts/apps/backend/src/contact/contact.service.ts @@ -4,6 +4,7 @@ import { DATABASE_CONNECTION } from '../db/database.module'; import { Database } from '../db/connection'; import { contacts, contactToTags } from '../db/schema'; import type { Contact, NewContact } from '../db/schema'; +import { PhotoService } from '../photo/photo.service'; export interface ContactBirthdaySummary { id: string; @@ -25,7 +26,10 @@ export interface ContactFilters { @Injectable() export class ContactService { - constructor(@Inject(DATABASE_CONNECTION) private db: Database) {} + constructor( + @Inject(DATABASE_CONNECTION) private db: Database, + private readonly photoService: PhotoService + ) {} async findByUserId(userId: string, filters: ContactFilters = {}): Promise { const { search, isFavorite, isArchived = false, tagId, limit = 50, offset = 0 } = filters; @@ -140,6 +144,9 @@ export class ContactService { throw new NotFoundException('Contact not found'); } + // Clean up photo from S3 before deleting the DB record + await this.photoService.deletePhotoByUrl(existing.photoUrl); + await this.db.delete(contacts).where(and(eq(contacts.id, id), eq(contacts.userId, userId))); } diff --git a/apps/contacts/apps/backend/src/photo/photo.service.ts b/apps/contacts/apps/backend/src/photo/photo.service.ts index f6c0ac11a..6a0338005 100644 --- a/apps/contacts/apps/backend/src/photo/photo.service.ts +++ b/apps/contacts/apps/backend/src/photo/photo.service.ts @@ -1,4 +1,4 @@ -import { Injectable, Inject, BadRequestException } from '@nestjs/common'; +import { Injectable, Inject, BadRequestException, Logger } from '@nestjs/common'; import { eq, and } from 'drizzle-orm'; import { DATABASE_CONNECTION } from '../db/database.module'; import { Database } from '../db/connection'; @@ -7,20 +7,28 @@ import { createContactsStorage, generateUserFileKey, getContentType, - validateFileSize, validateFileExtension, IMAGE_EXTENSIONS, - getStorageConfig, - BUCKETS, + type StorageClient, } from '@manacore/shared-storage'; const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB @Injectable() export class PhotoService { - private storage = createContactsStorage(); + private readonly logger = new Logger(PhotoService.name); + private storage: StorageClient; - constructor(@Inject(DATABASE_CONNECTION) private db: Database) {} + constructor(@Inject(DATABASE_CONNECTION) private db: Database) { + this.storage = createContactsStorage(); + + this.storage.hooks.on('upload', ({ key, sizeBytes, contentType }) => { + this.logger.debug(`Uploaded photo ${key} (${sizeBytes} bytes, ${contentType})`); + }); + this.storage.hooks.on('upload:error', ({ key, error }) => { + this.logger.error(`Failed to upload photo ${key}: ${error.message}`); + }); + } /** * Upload a photo for a contact @@ -30,22 +38,14 @@ export class PhotoService { userId: string, file: Express.Multer.File ): Promise<{ photoUrl: string }> { - // Validate file if (!file) { throw new BadRequestException('No file provided'); } - if (!validateFileSize(file.size, MAX_FILE_SIZE)) { - throw new BadRequestException(`File size exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit`); - } - - // validateFileExtension expects a filename, not just the extension if (!validateFileExtension(file.originalname, IMAGE_EXTENSIONS)) { throw new BadRequestException(`Invalid file type. Allowed: ${IMAGE_EXTENSIONS.join(', ')}`); } - const extension = file.originalname.split('.').pop()?.toLowerCase() || ''; - // Verify contact belongs to user const [contact] = await this.db .select() @@ -57,19 +57,17 @@ export class PhotoService { } // Generate unique key for the new photo - const filename = `${contactId}.${extension}`; - const key = generateUserFileKey(userId, filename); + const key = generateUserFileKey(userId, file.originalname, 'photos'); - // Upload new photo to S3 first (safe: if this fails, nothing is lost) - const contentType = getContentType(filename); - await this.storage.upload(key, file.buffer, { - contentType, + // Upload with size enforcement and cache headers + const result = await this.storage.upload(key, file.buffer, { + contentType: getContentType(file.originalname), public: true, + maxSizeBytes: MAX_FILE_SIZE, + cacheControl: 'public, max-age=31536000, immutable', }); - // Generate the URL from S3 endpoint configuration - const { endpoint } = getStorageConfig(); - const photoUrl = `${endpoint}/${BUCKETS.CONTACTS}/${key}`; + const photoUrl = result.url ?? this.storage.getPublicUrl(key) ?? ''; // Update contact with photo URL await this.db @@ -79,14 +77,7 @@ export class PhotoService { // Delete old photo if exists (after successful upload + DB update) if (contact.photoUrl) { - try { - const oldKey = this.extractKeyFromUrl(contact.photoUrl); - if (oldKey) { - await this.storage.delete(oldKey); - } - } catch { - // Ignore deletion errors - orphaned old file is harmless - } + await this.deleteStorageFile(contact.photoUrl); } return { photoUrl }; @@ -96,7 +87,6 @@ export class PhotoService { * Delete photo for a contact */ async deletePhoto(contactId: string, userId: string): Promise { - // Get contact const [contact] = await this.db .select() .from(contacts) @@ -107,28 +97,47 @@ export class PhotoService { } if (!contact.photoUrl) { - return; // No photo to delete + return; } - // Delete from S3 - try { - const key = this.extractKeyFromUrl(contact.photoUrl); - if (key) { - await this.storage.delete(key); - } - } catch { - // Ignore deletion errors - } + await this.deleteStorageFile(contact.photoUrl); - // Update contact to remove photo URL await this.db .update(contacts) .set({ photoUrl: null, updatedAt: new Date() }) .where(eq(contacts.id, contactId)); } + /** + * Delete photo from S3 when a contact is deleted. + * Called by ContactService.delete() to avoid orphaned files. + */ + async deletePhotoByUrl(photoUrl: string | null): Promise { + if (!photoUrl) return; + await this.deleteStorageFile(photoUrl); + } + + /** + * Delete all photos for a user (account deletion). + */ + async deleteAllUserPhotos(userId: string): Promise { + return this.storage.deleteByPrefix(`users/${userId}/`); + } + + private async deleteStorageFile(photoUrl: string): Promise { + try { + const key = this.extractKeyFromUrl(photoUrl); + if (key) { + await this.storage.delete(key); + } + } catch (err) { + this.logger.warn( + `Failed to delete photo from storage: ${photoUrl} — ${err instanceof Error ? err.message : err}` + ); + } + } + private extractKeyFromUrl(url: string): string | null { - // Extract key from URLs like {endpoint}/contacts-storage/users/xxx/file.jpg const match = url.match(/contacts-storage\/(.+)$/); return match ? match[1] : null; }