feat(server): better api error messages (for unhandled exceptions) (#4817)

* feat(server): better error messages

* chore: open api

* chore: remove debug log

* fix: syntax error

* fix: e2e test

server/src/domain/asset/asset.service.spec.ts

@@ -331,17 +331,17 @@ describe(AssetService.name, () => {
     });
   });
 
-  describe('getByTimeBucket', () => {
+  describe('getTimeBucket', () => {
     it('should return the assets for a album time bucket if user has album.read', async () => {
       accessMock.album.hasOwnerAccess.mockResolvedValue(true);
-      assetMock.getByTimeBucket.mockResolvedValue([assetStub.image]);
+      assetMock.getTimeBucket.mockResolvedValue([assetStub.image]);
 
       await expect(
-        sut.getByTimeBucket(authStub.admin, { size: TimeBucketSize.DAY, timeBucket: 'bucket', albumId: 'album-id' }),
+        sut.getTimeBucket(authStub.admin, { size: TimeBucketSize.DAY, timeBucket: 'bucket', albumId: 'album-id' }),
       ).resolves.toEqual(expect.arrayContaining([expect.objectContaining({ id: 'asset-id' })]));
 
       expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'album-id');
-      expect(assetMock.getByTimeBucket).toBeCalledWith('bucket', {
+      expect(assetMock.getTimeBucket).toBeCalledWith('bucket', {
         size: TimeBucketSize.DAY,
         timeBucket: 'bucket',
         albumId: 'album-id',
@@ -349,17 +349,17 @@ describe(AssetService.name, () => {
     });
 
     it('should return the assets for a archive time bucket if user has archive.read', async () => {
-      assetMock.getByTimeBucket.mockResolvedValue([assetStub.image]);
+      assetMock.getTimeBucket.mockResolvedValue([assetStub.image]);
 
       await expect(
-        sut.getByTimeBucket(authStub.admin, {
+        sut.getTimeBucket(authStub.admin, {
           size: TimeBucketSize.DAY,
           timeBucket: 'bucket',
           isArchived: true,
           userId: authStub.admin.id,
         }),
       ).resolves.toEqual(expect.arrayContaining([expect.objectContaining({ id: 'asset-id' })]));
-      expect(assetMock.getByTimeBucket).toBeCalledWith('bucket', {
+      expect(assetMock.getTimeBucket).toBeCalledWith('bucket', {
         size: TimeBucketSize.DAY,
         timeBucket: 'bucket',
         isArchived: true,
@@ -368,16 +368,16 @@ describe(AssetService.name, () => {
     });
 
     it('should return the assets for a library time bucket if user has library.read', async () => {
-      assetMock.getByTimeBucket.mockResolvedValue([assetStub.image]);
+      assetMock.getTimeBucket.mockResolvedValue([assetStub.image]);
 
       await expect(
-        sut.getByTimeBucket(authStub.admin, {
+        sut.getTimeBucket(authStub.admin, {
           size: TimeBucketSize.DAY,
           timeBucket: 'bucket',
           userId: authStub.admin.id,
         }),
       ).resolves.toEqual(expect.arrayContaining([expect.objectContaining({ id: 'asset-id' })]));
-      expect(assetMock.getByTimeBucket).toBeCalledWith('bucket', {
+      expect(assetMock.getTimeBucket).toBeCalledWith('bucket', {
         size: TimeBucketSize.DAY,
         timeBucket: 'bucket',
         userId: authStub.admin.id,

server/src/domain/asset/asset.service.ts

@@ -194,12 +194,12 @@ export class AssetService {
     return this.assetRepository.getTimeBuckets(dto);
   }
 
-  async getByTimeBucket(
+  async getTimeBucket(
     authUser: AuthUserDto,
     dto: TimeBucketAssetDto,
   ): Promise<AssetResponseDto[] | SanitizedAssetResponseDto[]> {
     await this.timeBucketChecks(authUser, dto);
-    const assets = await this.assetRepository.getByTimeBucket(dto.timeBucket, dto);
+    const assets = await this.assetRepository.getTimeBucket(dto.timeBucket, dto);
     if (authUser.isShowMetadata) {
       return assets.map((asset) => mapAsset(asset, { withStack: true }));
     } else {

server/src/domain/auth/auth.service.ts

@@ -315,13 +315,8 @@ export class AuthService {
     const redirectUri = this.normalize(config, url.split('?')[0]);
     const client = await this.getOAuthClient(config);
     const params = client.callbackParams(url);
-    try {
-      const tokens = await client.callback(redirectUri, params, { state: params.state });
-      return client.userinfo<OAuthProfile>(tokens.access_token || '');
-    } catch (error: Error | any) {
-      this.logger.error(`Unable to complete OAuth login: ${error}`, error?.stack);
-      throw new InternalServerErrorException(`Unable to complete OAuth login: ${error}`, { cause: error });
-    }
+    const tokens = await client.callback(redirectUri, params, { state: params.state });
+    return client.userinfo<OAuthProfile>(tokens.access_token || '');
   }
 
   private async getOAuthClient(config: SystemConfig) {

server/src/domain/repositories/asset.repository.ts

@@ -123,6 +123,6 @@ export interface IAssetRepository {
   getMapMarkers(ownerId: string, options?: MapMarkerSearchOptions): Promise<MapMarker[]>;
   getStatistics(ownerId: string, options: AssetStatsOptions): Promise<AssetStats>;
   getTimeBuckets(options: TimeBucketOptions): Promise<TimeBucketItem[]>;
-  getByTimeBucket(timeBucket: string, options: TimeBucketOptions): Promise<AssetEntity[]>;
+  getTimeBucket(timeBucket: string, options: TimeBucketOptions): Promise<AssetEntity[]>;
   upsertExif(exif: Partial<ExifEntity>): Promise<void>;
 }

server/src/domain/server-info/server-info.service.spec.ts

@@ -220,7 +220,7 @@ describe(ServerInfoService.name, () => {
         },
       ]);
 
-      await expect(sut.getStats()).resolves.toEqual({
+      await expect(sut.getStatistics()).resolves.toEqual({
         photos: 120,
         videos: 31,
         usage: 1123455,

server/src/domain/server-info/server-info.service.ts

@@ -92,7 +92,7 @@ export class ServerInfoService {
     };
   }
 
-  async getStats(): Promise<ServerStatsResponseDto> {
+  async getStatistics(): Promise<ServerStatsResponseDto> {
     const userStats: UserStatsQueryResponse[] = await this.userRepository.getUserStats();
     const serverStats = new ServerStatsResponseDto();
 

server/src/domain/user/user.core.ts

@@ -1,5 +1,5 @@
 import { LibraryType, UserEntity } from '@app/infra/entities';
-import { BadRequestException, ForbiddenException, InternalServerErrorException, Logger } from '@nestjs/common';
+import { BadRequestException, ForbiddenException } from '@nestjs/common';
 import path from 'path';
 import sanitize from 'sanitize-filename';
 import { AuthUserDto } from '../auth';
@@ -61,26 +61,21 @@ export class UserCore {
       }
     }
 
-    try {
-      if (dto.password) {
-        dto.password = await this.cryptoRepository.hashBcrypt(dto.password, SALT_ROUNDS);
-      }
-
-      if (dto.storageLabel === '') {
-        dto.storageLabel = null;
-      }
-
-      if (dto.externalPath === '') {
-        dto.externalPath = null;
-      } else if (dto.externalPath) {
-        dto.externalPath = path.normalize(dto.externalPath);
-      }
-
-      return this.userRepository.update(id, dto);
-    } catch (e) {
-      Logger.error(e, 'Failed to update user info');
-      throw new InternalServerErrorException('Failed to update user info');
+    if (dto.password) {
+      dto.password = await this.cryptoRepository.hashBcrypt(dto.password, SALT_ROUNDS);
     }
+
+    if (dto.storageLabel === '') {
+      dto.storageLabel = null;
+    }
+
+    if (dto.externalPath === '') {
+      dto.externalPath = null;
+    } else if (dto.externalPath) {
+      dto.externalPath = path.normalize(dto.externalPath);
+    }
+
+    return this.userRepository.update(id, dto);
   }
 
   async createUser(dto: Partial<UserEntity> & { email: string }): Promise<UserEntity> {
@@ -96,30 +91,25 @@ export class UserCore {
       }
     }
 
-    try {
-      const payload: Partial<UserEntity> = { ...dto };
-      if (payload.password) {
-        payload.password = await this.cryptoRepository.hashBcrypt(payload.password, SALT_ROUNDS);
-      }
-      if (payload.storageLabel) {
-        payload.storageLabel = sanitize(payload.storageLabel);
-      }
-
-      const userEntity = await this.userRepository.create(payload);
-      await this.libraryRepository.create({
-        owner: { id: userEntity.id } as UserEntity,
-        name: 'Default Library',
-        assets: [],
-        type: LibraryType.UPLOAD,
-        importPaths: [],
-        exclusionPatterns: [],
-        isVisible: true,
-      });
-
-      return userEntity;
-    } catch (e) {
-      Logger.error(e, 'Create new user');
-      throw new InternalServerErrorException('Failed to register new user');
+    const payload: Partial<UserEntity> = { ...dto };
+    if (payload.password) {
+      payload.password = await this.cryptoRepository.hashBcrypt(payload.password, SALT_ROUNDS);
     }
+    if (payload.storageLabel) {
+      payload.storageLabel = sanitize(payload.storageLabel);
+    }
+
+    const userEntity = await this.userRepository.create(payload);
+    await this.libraryRepository.create({
+      owner: { id: userEntity.id } as UserEntity,
+      name: 'Default Library',
+      assets: [],
+      type: LibraryType.UPLOAD,
+      importPaths: [],
+      exclusionPatterns: [],
+      isVisible: true,
+    });
+
+    return userEntity;
   }
 }

server/src/immich/api-v1/asset/asset.controller.ts

@@ -17,8 +17,8 @@ import {
 import { ApiBody, ApiConsumes, ApiHeader, ApiOkResponse, ApiTags } from '@nestjs/swagger';
 import { Response as Res } from 'express';
 import { AuthUser, Authenticated, SharedLinkRoute } from '../../app.guard';
-import { FileUploadInterceptor, ImmichFile, Route, mapToUploadFile } from '../../app.interceptor';
 import { UUIDParamDto } from '../../controllers/dto/uuid-param.dto';
+import { FileUploadInterceptor, ImmichFile, Route, mapToUploadFile } from '../../interceptors';
 import FileNotEmptyValidator from '../validation/file-not-empty-validator';
 import { AssetService } from './asset.service';
 import { AssetBulkUploadCheckDto } from './dto/asset-check.dto';
@@ -204,7 +204,7 @@ export class AssetController {
    */
   @Post('/bulk-upload-check')
   @HttpCode(HttpStatus.OK)
-  bulkUploadCheck(
+  checkBulkUpload(
     @AuthUser() authUser: AuthUserDto,
     @Body(ValidationPipe) dto: AssetBulkUploadCheckDto,
   ): Promise<AssetBulkUploadCheckResponseDto> {

server/src/immich/app.module.ts

@@ -2,14 +2,13 @@ import { DomainModule } from '@app/domain';
 import { InfraModule } from '@app/infra';
 import { AssetEntity } from '@app/infra/entities';
 import { Module, OnModuleDestroy, OnModuleInit } from '@nestjs/common';
-import { APP_GUARD } from '@nestjs/core';
+import { APP_GUARD, APP_INTERCEPTOR } from '@nestjs/core';
 import { ScheduleModule } from '@nestjs/schedule';
 import { TypeOrmModule } from '@nestjs/typeorm';
 import { AssetRepository, IAssetRepository } from './api-v1/asset/asset-repository';
 import { AssetController as AssetControllerV1 } from './api-v1/asset/asset.controller';
 import { AssetService } from './api-v1/asset/asset.service';
 import { AppGuard } from './app.guard';
-import { FileUploadInterceptor } from './app.interceptor';
 import { AppService } from './app.service';
 import {
   APIKeyController,
@@ -31,6 +30,7 @@ import {
   TagController,
   UserController,
 } from './controllers';
+import { ErrorInterceptor, FileUploadInterceptor } from './interceptors';
 
 @Module({
   imports: [
@@ -61,10 +61,9 @@ import {
     PersonController,
   ],
   providers: [
-    //
-    { provide: APP_GUARD, useExisting: AppGuard },
+    { provide: APP_INTERCEPTOR, useClass: ErrorInterceptor },
+    { provide: APP_GUARD, useClass: AppGuard },
     { provide: IAssetRepository, useClass: AssetRepository },
-    AppGuard,
     AppService,
     AssetService,
     FileUploadInterceptor,

server/src/immich/app.utils.ts

@@ -47,6 +47,9 @@ function sortKeys<T>(obj: T): T {
   return result as T;
 }
 
+export const routeToErrorMessage = (methodName: string) =>
+  'Failed to ' + methodName.replace(/[A-Z]+/g, (letter) => ` ${letter.toLowerCase()}`);
+
 const patchOpenAPI = (document: OpenAPIObject) => {
   document.paths = sortKeys(document.paths);
   if (document.components?.schemas) {
@@ -78,6 +81,10 @@ const patchOpenAPI = (document: OpenAPIObject) => {
         delete operation.summary;
       }
 
+      if (operation.operationId) {
+        // console.log(`${routeToErrorMessage(operation.operationId).padEnd(40)} (${operation.operationId})`);
+      }
+
       if (operation.description === '') {
         delete operation.description;
       }

server/src/immich/controllers/api-key.controller.ts

@@ -20,22 +20,22 @@ export class APIKeyController {
   constructor(private service: APIKeyService) {}
 
   @Post()
-  createKey(@AuthUser() authUser: AuthUserDto, @Body() dto: APIKeyCreateDto): Promise<APIKeyCreateResponseDto> {
+  createApiKey(@AuthUser() authUser: AuthUserDto, @Body() dto: APIKeyCreateDto): Promise<APIKeyCreateResponseDto> {
     return this.service.create(authUser, dto);
   }
 
   @Get()
-  getKeys(@AuthUser() authUser: AuthUserDto): Promise<APIKeyResponseDto[]> {
+  getApiKeys(@AuthUser() authUser: AuthUserDto): Promise<APIKeyResponseDto[]> {
     return this.service.getAll(authUser);
   }
 
   @Get(':id')
-  getKey(@AuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto): Promise<APIKeyResponseDto> {
+  getApiKey(@AuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto): Promise<APIKeyResponseDto> {
     return this.service.getById(authUser, id);
   }
 
   @Put(':id')
-  updateKey(
+  updateApiKey(
     @AuthUser() authUser: AuthUserDto,
     @Param() { id }: UUIDParamDto,
     @Body() dto: APIKeyUpdateDto,
@@ -44,7 +44,7 @@ export class APIKeyController {
   }
 
   @Delete(':id')
-  deleteKey(@AuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto): Promise<void> {
+  deleteApiKey(@AuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto): Promise<void> {
     return this.service.delete(authUser, id);
   }
 }

server/src/immich/controllers/asset.controller.ts

@@ -39,10 +39,11 @@ import {
 import { ApiOkResponse, ApiTags } from '@nestjs/swagger';
 import { AuthUser, Authenticated, SharedLinkRoute } from '../app.guard';
 import { UseValidation, asStreamableFile } from '../app.utils';
+import { Route } from '../interceptors';
 import { UUIDParamDto } from './dto/uuid-param.dto';
 
 @ApiTags('Asset')
-@Controller('asset')
+@Controller(Route.ASSET)
 @Authenticated()
 @UseValidation()
 export class AssetController {
@@ -86,7 +87,7 @@ export class AssetController {
   }
 
   @Get('statistics')
-  getAssetStats(@AuthUser() authUser: AuthUserDto, @Query() dto: AssetStatsDto): Promise<AssetStatsResponseDto> {
+  getAssetStatistics(@AuthUser() authUser: AuthUserDto, @Query() dto: AssetStatsDto): Promise<AssetStatsResponseDto> {
     return this.service.getStatistics(authUser, dto);
   }
 
@@ -98,8 +99,8 @@ export class AssetController {
 
   @Authenticated({ isShared: true })
   @Get('time-bucket')
-  getByTimeBucket(@AuthUser() authUser: AuthUserDto, @Query() dto: TimeBucketAssetDto): Promise<AssetResponseDto[]> {
-    return this.service.getByTimeBucket(authUser, dto) as Promise<AssetResponseDto[]>;
+  getTimeBucket(@AuthUser() authUser: AuthUserDto, @Query() dto: TimeBucketAssetDto): Promise<AssetResponseDto[]> {
+    return this.service.getTimeBucket(authUser, dto) as Promise<AssetResponseDto[]>;
   }
 
   @Post('jobs')

server/src/immich/controllers/auth.controller.ts

@@ -43,7 +43,7 @@ export class AuthController {
   @PublicRoute()
   @Post('admin-sign-up')
   @ApiBadRequestResponse({ description: 'The server already has an admin' })
-  adminSignUp(@Body() signUpCredential: SignUpDto): Promise<AdminSignupResponseDto> {
+  signUpAdmin(@Body() signUpCredential: SignUpDto): Promise<AdminSignupResponseDto> {
     return this.service.adminSignUp(signUpCredential);
   }
 

server/src/immich/controllers/library.controller.ts

@@ -21,7 +21,7 @@ export class LibraryController {
   constructor(private service: LibraryService) {}
 
   @Get()
-  getAllForUser(@AuthUser() authUser: AuthUserDto): Promise<ResponseDto[]> {
+  getLibraries(@AuthUser() authUser: AuthUserDto): Promise<ResponseDto[]> {
     return this.service.getAllForUser(authUser);
   }
 

server/src/immich/controllers/oauth.controller.ts

@@ -25,7 +25,7 @@ export class OAuthController {
   @PublicRoute()
   @Get('mobile-redirect')
   @Redirect()
-  mobileRedirect(@Req() req: Request) {
+  redirectOAuthToMobile(@Req() req: Request) {
     return {
       url: this.service.getMobileRedirect(req.url),
       statusCode: HttpStatus.TEMPORARY_REDIRECT,
@@ -35,19 +35,19 @@ export class OAuthController {
   /** @deprecated use feature flags and /oauth/authorize */
   @PublicRoute()
   @Post('config')
-  generateConfig(@Body() dto: OAuthConfigDto): Promise<OAuthConfigResponseDto> {
+  generateOAuthConfig(@Body() dto: OAuthConfigDto): Promise<OAuthConfigResponseDto> {
     return this.service.generateConfig(dto);
   }
 
   @PublicRoute()
   @Post('authorize')
-  authorizeOAuth(@Body() dto: OAuthConfigDto): Promise<OAuthAuthorizeResponseDto> {
+  startOAuth(@Body() dto: OAuthConfigDto): Promise<OAuthAuthorizeResponseDto> {
     return this.service.authorize(dto);
   }
 
   @PublicRoute()
   @Post('callback')
-  async callback(
+  async finishOAuth(
     @Res({ passthrough: true }) res: Response,
     @Body() dto: OAuthCallbackDto,
     @GetLoginDetails() loginDetails: LoginDetails,
@@ -58,12 +58,12 @@ export class OAuthController {
   }
 
   @Post('link')
-  link(@AuthUser() authUser: AuthUserDto, @Body() dto: OAuthCallbackDto): Promise<UserResponseDto> {
+  linkOAuthAccount(@AuthUser() authUser: AuthUserDto, @Body() dto: OAuthCallbackDto): Promise<UserResponseDto> {
     return this.service.link(authUser, dto);
   }
 
   @Post('unlink')
-  unlink(@AuthUser() authUser: AuthUserDto): Promise<UserResponseDto> {
+  unlinkOAuthAccount(@AuthUser() authUser: AuthUserDto): Promise<UserResponseDto> {
     return this.service.unlink(authUser);
   }
 }

server/src/immich/controllers/server-info.controller.ts

@@ -57,9 +57,9 @@ export class ServerInfoController {
   }
 
   @AdminRoute()
-  @Get('stats')
-  getStats(): Promise<ServerStatsResponseDto> {
-    return this.service.getStats();
+  @Get('statistics')
+  getServerStatistics(): Promise<ServerStatsResponseDto> {
+    return this.service.getStatistics();
   }
 
   @PublicRoute()

server/src/immich/controllers/system-config.controller.ts

@@ -17,7 +17,7 @@ export class SystemConfigController {
   }
 
   @Get('defaults')
-  getDefaults(): SystemConfigDto {
+  getConfigDefaults(): SystemConfigDto {
     return this.service.getDefaults();
   }
 

server/src/immich/controllers/user.controller.ts

@@ -22,8 +22,8 @@ import {
 } from '@nestjs/common';
 import { ApiBody, ApiConsumes, ApiTags } from '@nestjs/swagger';
 import { AdminRoute, AuthUser, Authenticated } from '../app.guard';
-import { FileUploadInterceptor, Route } from '../app.interceptor';
 import { UseValidation, asStreamableFile } from '../app.utils';
+import { FileUploadInterceptor, Route } from '../interceptors';
 import { UUIDParamDto } from './dto/uuid-param.dto';
 
 @ApiTags('User')

server/src/immich/interceptors/error.interceptor.ts

@@ -0,0 +1,32 @@
+import {
+  CallHandler,
+  ExecutionContext,
+  HttpException,
+  Injectable,
+  InternalServerErrorException,
+  Logger,
+  NestInterceptor,
+} from '@nestjs/common';
+import { Observable, catchError, throwError } from 'rxjs';
+import { routeToErrorMessage } from '../app.utils';
+
+@Injectable()
+export class ErrorInterceptor implements NestInterceptor {
+  private logger = new Logger(ErrorInterceptor.name);
+
+  async intercept(context: ExecutionContext, next: CallHandler<any>): Promise<Observable<any>> {
+    return next.handle().pipe(
+      catchError((error) =>
+        throwError(() => {
+          if (error instanceof HttpException === false) {
+            const errorMessage = routeToErrorMessage(context.getHandler().name);
+            this.logger.error(errorMessage, error, error?.errors);
+            return new InternalServerErrorException(errorMessage);
+          } else {
+            return error;
+          }
+        }),
+      ),
+    );
+  }
+}

server/src/immich/interceptors/file.interceptor.ts

@@ -7,7 +7,7 @@ import { createHash } from 'crypto';
 import { NextFunction, RequestHandler } from 'express';
 import multer, { StorageEngine, diskStorage } from 'multer';
 import { Observable } from 'rxjs';
-import { AuthRequest } from './app.guard';
+import { AuthRequest } from '../app.guard';
 
 export enum Route {
   ASSET = 'asset',

server/src/immich/interceptors/index.ts

@@ -0,0 +1,2 @@
+export * from './error.interceptor';
+export * from './file.interceptor';

server/src/infra/repositories/asset.repository.ts

@@ -493,7 +493,7 @@ export class AssetRepository implements IAssetRepository {
       .getRawMany();
   }
 
-  getByTimeBucket(timeBucket: string, options: TimeBucketOptions): Promise<AssetEntity[]> {
+  getTimeBucket(timeBucket: string, options: TimeBucketOptions): Promise<AssetEntity[]> {
     const truncated = dateTrunc(options);
     return (
       this.getBuilder(options)