refactor(server): standardize user controller (#3501)

* chore: remove redundant sortint

* chore: standardize user controller

* chore: open api

* fix: web dtos

server/immich-openapi-specs.json

@@ -4206,12 +4206,12 @@
         ]
       }
     },
-    "/user/info/{userId}": {
+    "/user/info/{id}": {
       "get": {
         "operationId": "getUserById",
         "parameters": [
           {
-            "name": "userId",
+            "name": "id",
             "required": true,
             "in": "path",
             "schema": {
@@ -4323,12 +4323,12 @@
         ]
       }
     },
-    "/user/profile-image/{userId}": {
+    "/user/profile-image/{id}": {
       "get": {
         "operationId": "getProfileImage",
         "parameters": [
           {
-            "name": "userId",
+            "name": "id",
             "required": true,
             "in": "path",
             "schema": {
@@ -4365,12 +4365,12 @@
         ]
       }
     },
-    "/user/{userId}": {
+    "/user/{id}": {
       "delete": {
         "operationId": "deleteUser",
         "parameters": [
           {
-            "name": "userId",
+            "name": "id",
             "required": true,
             "in": "path",
             "schema": {
@@ -4407,12 +4407,12 @@
         ]
       }
     },
-    "/user/{userId}/restore": {
+    "/user/{id}/restore": {
       "post": {
         "operationId": "restoreUser",
         "parameters": [
           {
-            "name": "userId",
+            "name": "id",
             "required": true,
             "in": "path",
             "schema": {

server/src/admin-cli/commands/list-users.command.ts

@@ -13,7 +13,7 @@ export class ListUsersCommand extends CommandRunner {
 
   async run(): Promise<void> {
     try {
-      const users = await this.userService.getAllUsers(CLI_USER, true);
+      const users = await this.userService.getAll(CLI_USER, true);
       console.dir(users);
     } catch (error) {
       console.error(error);

server/src/domain/user/dto/user-id.dto.ts

@@ -1,9 +0,0 @@
-import { ApiProperty } from '@nestjs/swagger';
-import { IsNotEmpty, IsUUID } from 'class-validator';
-
-export class UserIdDto {
-  @IsNotEmpty()
-  @IsUUID('4')
-  @ApiProperty({ format: 'uuid' })
-  userId!: string;
-}

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

@@ -136,11 +136,11 @@ describe(UserService.name, () => {
     when(userMock.get).calledWith(immichUser.id, undefined).mockResolvedValue(immichUser);
   });
 
-  describe('getAllUsers', () => {
+  describe('getAll', () => {
     it('should get all users', async () => {
       userMock.getList.mockResolvedValue([adminUser]);
 
-      const response = await sut.getAllUsers(adminUserAuth, false);
+      const response = await sut.getAll(adminUserAuth, false);
 
       expect(userMock.getList).toHaveBeenCalledWith({ withDeleted: true });
       expect(response).toEqual([
@@ -163,11 +163,11 @@ describe(UserService.name, () => {
     });
   });
 
-  describe('getUserById', () => {
+  describe('get', () => {
     it('should get a user by id', async () => {
       userMock.get.mockResolvedValue(adminUser);
 
-      const response = await sut.getUserById(adminUser.id);
+      const response = await sut.get(adminUser.id);
 
       expect(userMock.get).toHaveBeenCalledWith(adminUser.id, false);
       expect(response).toEqual(adminUserResponse);
@@ -176,17 +176,17 @@ describe(UserService.name, () => {
     it('should throw an error if a user is not found', async () => {
       userMock.get.mockResolvedValue(null);
 
-      await expect(sut.getUserById(adminUser.id)).rejects.toBeInstanceOf(NotFoundException);
+      await expect(sut.get(adminUser.id)).rejects.toBeInstanceOf(NotFoundException);
 
       expect(userMock.get).toHaveBeenCalledWith(adminUser.id, false);
     });
   });
 
-  describe('getUserInfo', () => {
+  describe('getMe', () => {
     it("should get the auth user's info", async () => {
       userMock.get.mockResolvedValue(adminUser);
 
-      const response = await sut.getUserInfo(adminUser);
+      const response = await sut.getMe(adminUser);
 
       expect(userMock.get).toHaveBeenCalledWith(adminUser.id, undefined);
       expect(response).toEqual(adminUserResponse);
@@ -195,17 +195,17 @@ describe(UserService.name, () => {
     it('should throw an error if a user is not found', async () => {
       userMock.get.mockResolvedValue(null);
 
-      await expect(sut.getUserInfo(adminUser)).rejects.toBeInstanceOf(BadRequestException);
+      await expect(sut.getMe(adminUser)).rejects.toBeInstanceOf(BadRequestException);
 
       expect(userMock.get).toHaveBeenCalledWith(adminUser.id, undefined);
     });
   });
 
-  describe('getUserCount', () => {
+  describe('getCount', () => {
     it('should get the user count', async () => {
       userMock.getList.mockResolvedValue([adminUser]);
 
-      const response = await sut.getUserCount({});
+      const response = await sut.getCount({});
 
       expect(userMock.getList).toHaveBeenCalled();
       expect(response).toEqual({ userCount: 1 });
@@ -221,14 +221,14 @@ describe(UserService.name, () => {
 
       when(userMock.update).calledWith(update.id, update).mockResolvedValueOnce(updatedImmichUser);
 
-      const updatedUser = await sut.updateUser(immichUserAuth, update);
+      const updatedUser = await sut.update(immichUserAuth, update);
       expect(updatedUser.shouldChangePassword).toEqual(true);
     });
 
     it('should not set an empty string for storage label', async () => {
       userMock.update.mockResolvedValue(updatedImmichUser);
 
-      await sut.updateUser(adminUserAuth, { id: immichUser.id, storageLabel: '' });
+      await sut.update(adminUserAuth, { id: immichUser.id, storageLabel: '' });
 
       expect(userMock.update).toHaveBeenCalledWith(immichUser.id, { id: immichUser.id, storageLabel: null });
     });
@@ -236,7 +236,7 @@ describe(UserService.name, () => {
     it('should omit a storage label set by non-admin users', async () => {
       userMock.update.mockResolvedValue(updatedImmichUser);
 
-      await sut.updateUser(immichUserAuth, { id: immichUser.id, storageLabel: 'admin' });
+      await sut.update(immichUserAuth, { id: immichUser.id, storageLabel: 'admin' });
 
       expect(userMock.update).toHaveBeenCalledWith(immichUser.id, { id: immichUser.id });
     });
@@ -249,7 +249,7 @@ describe(UserService.name, () => {
           id: 'not_immich_auth_user_id',
         });
 
-      const result = sut.updateUser(immichUserAuth, {
+      const result = sut.update(immichUserAuth, {
         id: 'not_immich_auth_user_id',
         password: 'I take over your account now',
       });
@@ -262,7 +262,7 @@ describe(UserService.name, () => {
       userMock.get.mockResolvedValue(immichUser);
       userMock.update.mockResolvedValue(immichUser);
 
-      await sut.updateUser(immichUser, dto);
+      await sut.update(immichUser, dto);
 
       expect(userMock.update).toHaveBeenCalledWith(immichUser.id, {
         id: 'user-id',
@@ -276,7 +276,7 @@ describe(UserService.name, () => {
       userMock.get.mockResolvedValue(immichUser);
       userMock.getByEmail.mockResolvedValue(adminUser);
 
-      await expect(sut.updateUser(immichUser, dto)).rejects.toBeInstanceOf(BadRequestException);
+      await expect(sut.update(immichUser, dto)).rejects.toBeInstanceOf(BadRequestException);
 
       expect(userMock.update).not.toHaveBeenCalled();
     });
@@ -287,7 +287,7 @@ describe(UserService.name, () => {
       userMock.get.mockResolvedValue(immichUser);
       userMock.getByStorageLabel.mockResolvedValue(adminUser);
 
-      await expect(sut.updateUser(adminUser, dto)).rejects.toBeInstanceOf(BadRequestException);
+      await expect(sut.update(adminUser, dto)).rejects.toBeInstanceOf(BadRequestException);
 
       expect(userMock.update).not.toHaveBeenCalled();
     });
@@ -300,7 +300,7 @@ describe(UserService.name, () => {
 
       when(userMock.update).calledWith(immichUser.id, update).mockResolvedValueOnce(updatedImmichUser);
 
-      const result = await sut.updateUser(adminUserAuth, update);
+      const result = await sut.update(adminUserAuth, update);
 
       expect(result).toBeDefined();
       expect(result.id).toEqual(updatedImmichUser.id);
@@ -310,7 +310,7 @@ describe(UserService.name, () => {
     it('update user information should throw error if user not found', async () => {
       when(userMock.get).calledWith(immichUser.id, undefined).mockResolvedValueOnce(null);
 
-      const result = sut.updateUser(adminUser, {
+      const result = sut.update(adminUser, {
         id: immichUser.id,
         shouldChangePassword: true,
       });
@@ -324,7 +324,7 @@ describe(UserService.name, () => {
       when(userMock.get).calledWith(adminUser.id).mockResolvedValueOnce(null);
       when(userMock.update).calledWith(adminUser.id, dto).mockResolvedValueOnce(adminUser);
 
-      await sut.updateUser(adminUser, dto);
+      await sut.update(adminUser, dto);
 
       expect(userMock.update).toHaveBeenCalledWith(adminUser.id, dto);
     });
@@ -334,31 +334,31 @@ describe(UserService.name, () => {
 
       when(userMock.get).calledWith(immichUser.id).mockResolvedValueOnce(immichUser);
 
-      await expect(sut.updateUser(adminUser, dto)).rejects.toBeInstanceOf(BadRequestException);
+      await expect(sut.update(adminUser, dto)).rejects.toBeInstanceOf(BadRequestException);
     });
   });
 
-  describe('restoreUser', () => {
+  describe('restore', () => {
     it('should require an admin', async () => {
       when(userMock.get).calledWith(adminUser.id, true).mockResolvedValue(adminUser);
-      await expect(sut.restoreUser(immichUserAuth, adminUser.id)).rejects.toBeInstanceOf(ForbiddenException);
+      await expect(sut.restore(immichUserAuth, adminUser.id)).rejects.toBeInstanceOf(ForbiddenException);
       expect(userMock.get).toHaveBeenCalledWith(adminUser.id, true);
     });
 
     it('should require the auth user be an admin', async () => {
-      await expect(sut.deleteUser(immichUserAuth, adminUserAuth.id)).rejects.toBeInstanceOf(ForbiddenException);
+      await expect(sut.delete(immichUserAuth, adminUserAuth.id)).rejects.toBeInstanceOf(ForbiddenException);
 
       expect(userMock.delete).not.toHaveBeenCalled();
     });
   });
 
-  describe('deleteUser', () => {
+  describe('delete', () => {
     it('cannot delete admin user', async () => {
-      await expect(sut.deleteUser(adminUserAuth, adminUserAuth.id)).rejects.toBeInstanceOf(ForbiddenException);
+      await expect(sut.delete(adminUserAuth, adminUserAuth.id)).rejects.toBeInstanceOf(ForbiddenException);
     });
 
     it('should require the auth user be an admin', async () => {
-      await expect(sut.deleteUser(immichUserAuth, adminUserAuth.id)).rejects.toBeInstanceOf(ForbiddenException);
+      await expect(sut.delete(immichUserAuth, adminUserAuth.id)).rejects.toBeInstanceOf(ForbiddenException);
 
       expect(userMock.delete).not.toHaveBeenCalled();
     });
@@ -369,7 +369,7 @@ describe(UserService.name, () => {
       when(userMock.getAdmin).calledWith().mockResolvedValueOnce(null);
 
       await expect(
-        sut.createUser({
+        sut.create({
           email: 'john_smith@email.com',
           firstName: 'John',
           lastName: 'Smith',
@@ -394,7 +394,7 @@ describe(UserService.name, () => {
     it('should throw an error if the user does not exist', async () => {
       userMock.get.mockResolvedValue(null);
 
-      await expect(sut.getUserProfileImage(adminUserAuth.id)).rejects.toBeInstanceOf(NotFoundException);
+      await expect(sut.getProfileImage(adminUserAuth.id)).rejects.toBeInstanceOf(NotFoundException);
 
       expect(userMock.get).toHaveBeenCalledWith(adminUserAuth.id, undefined);
     });
@@ -402,7 +402,7 @@ describe(UserService.name, () => {
     it('should throw an error if the user does not have a picture', async () => {
       userMock.get.mockResolvedValue(adminUser);
 
-      await expect(sut.getUserProfileImage(adminUserAuth.id)).rejects.toBeInstanceOf(NotFoundException);
+      await expect(sut.getProfileImage(adminUserAuth.id)).rejects.toBeInstanceOf(NotFoundException);
 
       expect(userMock.get).toHaveBeenCalledWith(adminUserAuth.id, undefined);
     });

server/src/domain/user/user.service.ts

@@ -39,12 +39,12 @@ export class UserService {
     this.userCore = new UserCore(userRepository, cryptoRepository);
   }
 
-  async getAllUsers(authUser: AuthUserDto, isAll: boolean): Promise<UserResponseDto[]> {
+  async getAll(authUser: AuthUserDto, isAll: boolean): Promise<UserResponseDto[]> {
     const users = await this.userCore.getList({ withDeleted: !isAll });
     return users.map(mapUser);
   }
 
-  async getUserById(userId: string, withDeleted = false): Promise<UserResponseDto> {
+  async get(userId: string, withDeleted = false): Promise<UserResponseDto> {
     const user = await this.userCore.get(userId, withDeleted);
     if (!user) {
       throw new NotFoundException('User not found');
@@ -53,7 +53,7 @@ export class UserService {
     return mapUser(user);
   }
 
-  async getUserInfo(authUser: AuthUserDto): Promise<UserResponseDto> {
+  async getMe(authUser: AuthUserDto): Promise<UserResponseDto> {
     const user = await this.userCore.get(authUser.id);
     if (!user) {
       throw new BadRequestException('User not found');
@@ -61,7 +61,7 @@ export class UserService {
     return mapUser(user);
   }
 
-  async getUserCount(dto: UserCountDto): Promise<UserCountResponseDto> {
+  async getCount(dto: UserCountDto): Promise<UserCountResponseDto> {
     let users = await this.userCore.getList();
 
     if (dto.admin) {
@@ -71,12 +71,12 @@ export class UserService {
     return mapUserCountResponse(users.length);
   }
 
-  async createUser(createUserDto: CreateUserDto): Promise<UserResponseDto> {
+  async create(createUserDto: CreateUserDto): Promise<UserResponseDto> {
     const createdUser = await this.userCore.createUser(createUserDto);
     return mapUser(createdUser);
   }
 
-  async updateUser(authUser: AuthUserDto, dto: UpdateUserDto): Promise<UserResponseDto> {
+  async update(authUser: AuthUserDto, dto: UpdateUserDto): Promise<UserResponseDto> {
     const user = await this.userCore.get(dto.id);
     if (!user) {
       throw new NotFoundException('User not found');
@@ -86,7 +86,7 @@ export class UserService {
     return mapUser(updatedUser);
   }
 
-  async deleteUser(authUser: AuthUserDto, userId: string): Promise<UserResponseDto> {
+  async delete(authUser: AuthUserDto, userId: string): Promise<UserResponseDto> {
     const user = await this.userCore.get(userId);
     if (!user) {
       throw new BadRequestException('User not found');
@@ -95,7 +95,7 @@ export class UserService {
     return mapUser(deletedUser);
   }
 
-  async restoreUser(authUser: AuthUserDto, userId: string): Promise<UserResponseDto> {
+  async restore(authUser: AuthUserDto, userId: string): Promise<UserResponseDto> {
     const user = await this.userCore.get(userId, true);
     if (!user) {
       throw new BadRequestException('User not found');
@@ -112,7 +112,7 @@ export class UserService {
     return mapCreateProfileImageResponse(updatedUser.id, updatedUser.profileImagePath);
   }
 
-  async getUserProfileImage(userId: string): Promise<ReadStream> {
+  async getProfileImage(userId: string): Promise<ReadStream> {
     const user = await this.userCore.get(userId);
     if (!user) {
       throw new NotFoundException('User not found');

server/src/immich/app.utils.ts

@@ -48,11 +48,7 @@ function sortKeys<T>(obj: T): T {
 }
 
 const patchOpenAPI = (document: OpenAPIObject) => {
-  for (const [key, value] of Object.entries(document.paths)) {
-    document.paths[key] = sortKeys(value);
-  }
   document.paths = sortKeys(document.paths);
-
   if (document.components?.schemas) {
     document.components.schemas = sortKeys(document.components.schemas);
   }

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

@@ -2,14 +2,13 @@ import {
   AuthUserDto,
   CreateProfileImageDto,
   CreateProfileImageResponseDto,
-  CreateUserDto,
-  UpdateUserDto,
-  UserCountDto,
+  CreateUserDto as CreateDto,
+  UpdateUserDto as UpdateDto,
+  UserCountDto as CountDto,
   UserCountResponseDto,
   UserResponseDto,
   UserService,
 } from '@app/domain';
-import { UserIdDto } from '@app/domain/user/dto/user-id.dto';
 import {
   Body,
   Controller,
@@ -30,6 +29,7 @@ import { Response as Res } from 'express';
 import { AdminRoute, Authenticated, AuthUser, PublicRoute } from '../app.guard';
 import { FileUploadInterceptor, Route } from '../app.interceptor';
 import { UseValidation } from '../app.utils';
+import { UUIDParamDto } from './dto/uuid-param.dto';
 
 @ApiTags('User')
 @Controller(Route.USER)
@@ -40,53 +40,53 @@ export class UserController {
 
   @Get()
   getAllUsers(@AuthUser() authUser: AuthUserDto, @Query('isAll') isAll: boolean): Promise<UserResponseDto[]> {
-    return this.service.getAllUsers(authUser, isAll);
+    return this.service.getAll(authUser, isAll);
   }
 
-  @Get('/info/:userId')
-  getUserById(@Param() { userId }: UserIdDto): Promise<UserResponseDto> {
-    return this.service.getUserById(userId);
+  @Get('info/:id')
+  getUserById(@Param() { id }: UUIDParamDto): Promise<UserResponseDto> {
+    return this.service.get(id);
   }
 
   @Get('me')
   getMyUserInfo(@AuthUser() authUser: AuthUserDto): Promise<UserResponseDto> {
-    return this.service.getUserInfo(authUser);
+    return this.service.getMe(authUser);
   }
 
   @AdminRoute()
   @Post()
-  createUser(@Body() createUserDto: CreateUserDto): Promise<UserResponseDto> {
-    return this.service.createUser(createUserDto);
+  createUser(@Body() createUserDto: CreateDto): Promise<UserResponseDto> {
+    return this.service.create(createUserDto);
   }
 
   @PublicRoute()
-  @Get('/count')
-  getUserCount(@Query() dto: UserCountDto): Promise<UserCountResponseDto> {
-    return this.service.getUserCount(dto);
+  @Get('count')
+  getUserCount(@Query() dto: CountDto): Promise<UserCountResponseDto> {
+    return this.service.getCount(dto);
   }
 
   @AdminRoute()
-  @Delete('/:userId')
-  deleteUser(@AuthUser() authUser: AuthUserDto, @Param() { userId }: UserIdDto): Promise<UserResponseDto> {
-    return this.service.deleteUser(authUser, userId);
+  @Delete(':id')
+  deleteUser(@AuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto): Promise<UserResponseDto> {
+    return this.service.delete(authUser, id);
   }
 
   @AdminRoute()
-  @Post('/:userId/restore')
-  restoreUser(@AuthUser() authUser: AuthUserDto, @Param() { userId }: UserIdDto): Promise<UserResponseDto> {
-    return this.service.restoreUser(authUser, userId);
+  @Post(':id/restore')
+  restoreUser(@AuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto): Promise<UserResponseDto> {
+    return this.service.restore(authUser, id);
   }
 
   // TODO: replace with @Put(':id')
   @Put()
-  updateUser(@AuthUser() authUser: AuthUserDto, @Body() updateUserDto: UpdateUserDto): Promise<UserResponseDto> {
-    return this.service.updateUser(authUser, updateUserDto);
+  updateUser(@AuthUser() authUser: AuthUserDto, @Body() updateUserDto: UpdateDto): Promise<UserResponseDto> {
+    return this.service.update(authUser, updateUserDto);
   }
 
   @UseInterceptors(FileUploadInterceptor)
   @ApiConsumes('multipart/form-data')
   @ApiBody({ description: 'A new avatar for the user', type: CreateProfileImageDto })
-  @Post('/profile-image')
+  @Post('profile-image')
   createProfileImage(
     @AuthUser() authUser: AuthUserDto,
     @UploadedFile() fileInfo: Express.Multer.File,
@@ -94,10 +94,10 @@ export class UserController {
     return this.service.createProfileImage(authUser, fileInfo);
   }
 
-  @Get('/profile-image/:userId')
+  @Get('profile-image/:id')
   @Header('Cache-Control', 'private, no-cache, no-transform')
-  async getProfileImage(@Param() { userId }: UserIdDto, @Response({ passthrough: true }) res: Res): Promise<any> {
-    const readableStream = await this.service.getUserProfileImage(userId);
+  async getProfileImage(@Param() { id }: UUIDParamDto, @Response({ passthrough: true }) res: Res): Promise<any> {
+    const readableStream = await this.service.getProfileImage(id);
     res.header('Content-Type', 'image/jpeg');
     return new StreamableFile(readableStream);
   }