[backend] Use class serializer interceptor #29
Conversation
cf. Building a REST API with NestJS and Prisma: Handling Relational Data https://www.prisma.io/blog/nestjs-prisma-relational-data-7D056s1kOabc#add-a-user-model-to-the-database
|
Warning Rate Limit Exceeded@usatie has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 0 minutes and 56 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. WalkthroughThe changes in this update focus on improving the handling of user data in the backend. The password field is now excluded from serialization using the Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- backend/src/main.ts (2 hunks)
- backend/src/user/entities/user.entity.ts (2 hunks)
- backend/src/user/user.controller.ts (1 hunks)
- backend/src/user/user.service.ts (1 hunks)
Additional comments: 10
backend/src/user/entities/user.entity.ts (2)
1-8: The constructor is a good addition to the
UserEntityclass. It allows for easy instantiation of the class with a partial object. This is a common pattern in TypeScript and is useful for creating instances from database results or request bodies.19-20: The
@Exclude()decorator is a good way to prevent the password from being serialized when instances ofUserEntityare transformed to JSON. This is a good security practice as it prevents the password from being accidentally exposed in API responses or logs.backend/src/main.ts (2)
1-7: The
Reflectorclass is imported from@nestjs/coreand theClassSerializerInterceptoris imported from@nestjs/common. These are necessary for the global application of theClassSerializerInterceptor.10-13: The
ClassSerializerInterceptoris globally applied to the application using theapp.useGlobalInterceptors()method. This interceptor will automatically serialize the response objects according to the decorators used in the entity classes.+ app.useGlobalInterceptors(new ClassSerializerInterceptor(app.get(Reflector))); // enable serializationbackend/src/user/user.controller.ts (5)
27-34: The
createmethod now returns a newUserEntityinstance created from the result ofuserService.create. This is a good practice as it ensures that the returned user object is an instance ofUserEntityand not just a plain JavaScript object. This allows for better type checking and can prevent potential runtime errors.36-41: The
findAllmethod now returns an array ofUserEntityinstances created from the result ofuserService.findAll. This is a good practice as it ensures that the returned user objects are instances ofUserEntityand not just plain JavaScript objects. This allows for better type checking and can prevent potential runtime errors.43-49: The
findOnemethod now returns aUserEntityinstance created from the result ofuserService.findOne. This is a good practice as it ensures that the returned user object is an instance ofUserEntityand not just a plain JavaScript object. This allows for better type checking and can prevent potential runtime errors.51-60: The
updatemethod now returns aUserEntityinstance created from the result ofuserService.update. This is a good practice as it ensures that the returned user object is an instance ofUserEntityand not just a plain JavaScript object. This allows for better type checking and can prevent potential runtime errors.62-68: The
removemethod now returns aUserEntityinstance created from the result ofuserService.remove. This is a good practice as it ensures that the returned user object is an instance ofUserEntityand not just a plain JavaScript object. This allows for better type checking and can prevent potential runtime errors.backend/src/user/user.service.ts (1)
- 24-27: The
findUniqueOrThrowmethod will throw an error if the user is not found. Ensure that this error is being properly handled in the calling code.
backend/src/user/user.service.ts
Outdated
| update( | ||
| id: number, | ||
| updateUserDto: UpdateUserDto, | ||
| ): Promise<UserWithoutPassword> { | ||
| ): Promise<User> { | ||
| return this.prisma.user | ||
| .update({ | ||
| where: { id: id }, | ||
| data: updateUserDto, | ||
| }) | ||
| .then(excludePassword); | ||
| }); | ||
| } |
There was a problem hiding this comment.
When updating a user, consider checking if the user exists before attempting the update. This can prevent unnecessary database operations and provide more informative error messages.
+ async userExists(id: number): Promise<boolean> {
+ const user = await this.prisma.user.findUnique({ where: { id: id } });
+ return user !== null;
+ }
- update(
- id: number,
- updateUserDto: UpdateUserDto,
- ): Promise<User> {
- return this.prisma.user
- .update({
- where: { id: id },
- data: updateUserDto,
- });
- }
+ async update(
+ id: number,
+ updateUserDto: UpdateUserDto,
+ ): Promise<User> {
+ if (!await this.userExists(id)) {
+ throw new Error(`User with id ${id} does not exist`);
+ }
+ return this.prisma.user
+ .update({
+ where: { id: id },
+ data: updateUserDto,
+ });
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| update( | |
| id: number, | |
| updateUserDto: UpdateUserDto, | |
| ): Promise<UserWithoutPassword> { | |
| ): Promise<User> { | |
| return this.prisma.user | |
| .update({ | |
| where: { id: id }, | |
| data: updateUserDto, | |
| }) | |
| .then(excludePassword); | |
| }); | |
| } | |
| async userExists(id: number): Promise<boolean> { | |
| const user = await this.prisma.user.findUnique({ where: { id: id } }); | |
| return user !== null; | |
| } | |
| async update( | |
| id: number, | |
| updateUserDto: UpdateUserDto, | |
| ): Promise<User> { | |
| if (!await this.userExists(id)) { | |
| throw new Error(`User with id ${id} does not exist`); | |
| } | |
| return this.prisma.user | |
| .update({ | |
| where: { id: id }, | |
| data: updateUserDto, | |
| }); | |
| } |
There was a problem hiding this comment.
Checking if the user exists before attempting the update is done by ValidationPipe.
There was a problem hiding this comment.
@usatie, thank you for your comment. You're correct that the ValidationPipe can be used to validate incoming data in the request. However, it doesn't inherently check if a resource (like a user in this case) exists in the database before performing operations like update or delete.
The suggestion to add a userExists method is to ensure that the user exists in the database before attempting to update. This can provide more informative error messages and prevent unnecessary database operations. If the ValidationPipe in your application is already configured to perform this check, then the userExists method may not be necessary.
There was a problem hiding this comment.
It is already configured to perform that check, so it's unnecessary.
There was a problem hiding this comment.
@usatie, thank you for the clarification. If the ValidationPipe is already configured to check if the user exists before attempting an update, then the userExists method is indeed unnecessary. I appreciate your time and input on this matter.
backend/src/user/user.service.ts
Outdated
| remove(id: number): Promise<User> { | ||
| return this.prisma.user | ||
| .delete({ | ||
| where: { id: id }, | ||
| }) | ||
| .then(excludePassword); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Similar to the update method, consider checking if the user exists before attempting to delete. This can prevent unnecessary database operations and provide more informative error messages.
- remove(id: number): Promise<User> {
- return this.prisma.user
- .delete({
- where: { id: id },
- });
- }
+ async remove(id: number): Promise<User> {
+ if (!await this.userExists(id)) {
+ throw new Error(`User with id ${id} does not exist`);
+ }
+ return this.prisma.user
+ .delete({
- where: { id: id },
+ });
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| remove(id: number): Promise<User> { | |
| return this.prisma.user | |
| .delete({ | |
| where: { id: id }, | |
| }) | |
| .then(excludePassword); | |
| }); | |
| } | |
| async remove(id: number): Promise<User> { | |
| if (!await this.userExists(id)) { | |
| throw new Error(`User with id ${id} does not exist`); | |
| } | |
| return this.prisma.user | |
| .delete({ | |
| where: { id: id }, | |
| }); | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- backend/src/user/entities/user.entity.ts (2 hunks)
- backend/src/user/user.controller.ts (1 hunks)
- backend/src/user/user.service.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- backend/src/user/entities/user.entity.ts
Additional comments: 6
backend/src/user/user.controller.ts (5)
27-32: The
createmethod is now asynchronous and returns a newUserEntityobject created from the result ofuserService.create. Ensure that thecreatemethod inuserServicereturns a promise that resolves to a user object that can be used to create aUserEntity.34-39: The
findAllmethod is now asynchronous and returns an array ofUserEntityobjects created from the result ofuserService.findAll. Ensure that thefindAllmethod inuserServicereturns a promise that resolves to an array of user objects that can be used to createUserEntityobjects.41-45: The
findOnemethod is now asynchronous and returns aUserEntityobject created from the result ofuserService.findOne. Ensure that thefindOnemethod inuserServicereturns a promise that resolves to a user object that can be used to create aUserEntity.47-54: The
updatemethod is now asynchronous and returns aUserEntityobject created from the result ofuserService.update. Ensure that theupdatemethod inuserServicereturns a promise that resolves to a user object that can be used to create aUserEntity.56-60: The
removemethod is now asynchronous and returns aUserEntityobject created from the result ofuserService.remove. Ensure that theremovemethod inuserServicereturns a promise that resolves to a user object that can be used to create aUserEntity.backend/src/user/user.service.ts (1)
- 34-38: The return types of these methods have been changed to
UserfromUserWithoutPassword. This means that the password field is now included in the returned user objects. This could potentially expose sensitive user information. Ensure that the password field is properly excluded from the response in the controller or elsewhere in the application.
backend/src/user/user.service.ts
Outdated
| async create(createUserDto: CreateUserDto): Promise<User> { | ||
| const saltRounds = 10; | ||
| const hashedPassword = await hash(createUserDto.password, saltRounds); | ||
| const userData = { ...createUserDto, password: hashedPassword }; | ||
| return this.prisma.user.create({ data: userData }).then(excludePassword); | ||
| return this.prisma.user.create({ data: userData }); | ||
| } |
There was a problem hiding this comment.
The password hashing logic is still present in the create method. Consider moving it to a separate function for better modularity and reusability. This will also make the code easier to test and maintain.
+ async hashPassword(password: string): Promise<string> {
+ const saltRounds = 10;
+ return await hash(password, saltRounds);
+ }
- async create(createUserDto: CreateUserDto): Promise<User> {
- const saltRounds = 10;
- const hashedPassword = await hash(createUserDto.password, saltRounds);
- const userData = { ...createUserDto, password: hashedPassword };
- return this.prisma.user.create({ data: userData });
- }
+ async create(createUserDto: CreateUserDto): Promise<User> {
+ const hashedPassword = await this.hashPassword(createUserDto.password);
+ const userData = { ...createUserDto, password: hashedPassword };
+ return this.prisma.user.create({ data: userData });
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| async create(createUserDto: CreateUserDto): Promise<User> { | |
| const saltRounds = 10; | |
| const hashedPassword = await hash(createUserDto.password, saltRounds); | |
| const userData = { ...createUserDto, password: hashedPassword }; | |
| return this.prisma.user.create({ data: userData }).then(excludePassword); | |
| return this.prisma.user.create({ data: userData }); | |
| } | |
| async hashPassword(password: string): Promise<string> { | |
| const saltRounds = 10; | |
| return await hash(password, saltRounds); | |
| } | |
| async create(createUserDto: CreateUserDto): Promise<User> { | |
| const hashedPassword = await this.hashPassword(createUserDto.password); | |
| const userData = { ...createUserDto, password: hashedPassword }; | |
| return this.prisma.user.create({ data: userData }); | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- backend/src/user/user.controller.ts (2 hunks)
Additional comments: 6
backend/src/user/user.controller.ts (6)
6-11: The import statements are well-organized and only necessary modules are imported. Good job on maintaining clean and efficient code.
28-34: The
createmethod now returns aUserEntityobject. Ensure that the frontend or any other consumer of this API is expecting this new return type.36-41: The
findAllmethod now returns an array ofUserEntityobjects. As with thecreatemethod, ensure that the frontend or any other consumer of this API is expecting this new return type.43-47: The
findOnemethod now returns aUserEntityobject. As with thecreatemethod, ensure that the frontend or any other consumer of this API is expecting this new return type.49-56: The
updatemethod now returns aUserEntityobject. As with thecreatemethod, ensure that the frontend or any other consumer of this API is expecting this new return type.58-63: The
removemethod now returns a promise ofvoid. This is a good practice as it doesn't reveal any unnecessary information to the client. However, ensure that the frontend or any other consumer of this API is expecting this new return type.
usatie
changed the title
UserEntityをResponseタイプに持つhandlerから自動的に
@Excludeで指定したpasswordカラムが削除されるようにしました。とはいえ、Prismaで指定されたUserを返り値に指定してしまうことがあったらダメなので、もっとまともなチェックはないのかなーという感じがしました。
cf. Building a REST API with NestJS and Prisma: Handling Relational Data https://www.prisma.io/blog/nestjs-prisma-relational-data-7D056s1kOabc#add-a-user-model-to-the-database
Summary by CodeRabbit
UserEntityclass to include a constructor and exclude password from serialization.UserControllerandUserServiceclasses for improved data return types.excludePasswordfunction to prevent password exposure in user objects.