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) <noreply@anthropic.com>
This commit is contained in:
Till JS 2026-03-20 20:01:43 +01:00
parent d9ccb5e31b
commit ecec3d56fe
4 changed files with 68 additions and 45 deletions

View file

@ -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();

View file

@ -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],

View file

@ -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<Contact[]> {
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)));
}

View file

@ -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<void> {
// 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<void> {
if (!photoUrl) return;
await this.deleteStorageFile(photoUrl);
}
/**
* Delete all photos for a user (account deletion).
*/
async deleteAllUserPhotos(userId: string): Promise<number> {
return this.storage.deleteByPrefix(`users/${userId}/`);
}
private async deleteStorageFile(photoUrl: string): Promise<void> {
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;
}