Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[backend] Use class serializer interceptor #29

Merged
merged 4 commits into from
Nov 6, 2023
Merged

[backend] Use class serializer interceptor #29

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b4ce51b
[backend] Use class serializer interceptor
usatie Nov 6, 2023
215de65
[format] Format code with prettier
usatie Nov 6, 2023
885d819
[backend] Return 204 on DELETE /users/:id
usatie Nov 6, 2023
f0f5856
[backend] Add hashPassword method to UserService
usatie Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions backend/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { HttpAdapterHost, NestFactory } from '@nestjs/core';
import { HttpAdapterHost, NestFactory, Reflector } from '@nestjs/core';
import { AppModule } from './app.module';
import { SwaggerModule, DocumentBuilder } from '@nestjs/swagger';
import { ValidationPipe } from '@nestjs/common';
import { ClassSerializerInterceptor, ValidationPipe } from '@nestjs/common';
import { PrismaClientExceptionFilter } from 'nestjs-prisma';

async function bootstrap() {
Expand All @@ -10,6 +10,7 @@ async function bootstrap() {
app.setGlobalPrefix('api');

app.useGlobalPipes(new ValidationPipe({ whitelist: true })); // enable validation
app.useGlobalInterceptors(new ClassSerializerInterceptor(app.get(Reflector))); // enable serialization

const config = new DocumentBuilder()
.setTitle('Pong API')
Expand Down
6 changes: 6 additions & 0 deletions backend/src/user/entities/user.entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { User } from '@prisma/client';
import { ApiProperty } from '@nestjs/swagger';
import { Exclude } from 'class-transformer';

export class UserEntity implements User {
constructor(partial: Partial<UserEntity>) {
Object.assign(this, partial);
}

@ApiProperty()
id: number;

Expand All @@ -11,5 +16,6 @@ export class UserEntity implements User {
@ApiProperty({ required: false, nullable: true })
name: string | null;

@Exclude()
password: string;
}
29 changes: 19 additions & 10 deletions backend/src/user/user.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,34 +27,43 @@ export class UserController {

@Post()
@ApiCreatedResponse({ type: UserEntity })
create(@Body() createUserDto: CreateUserDto) {
return this.userService.create(createUserDto);
async create(@Body() createUserDto: CreateUserDto) {
return new UserEntity(
await this.userService.create(createUserDto)
);
}

@Get()
@ApiOkResponse({ type: [UserEntity] })
findAll() {
return this.userService.findAll();
async findAll() {
const users = await this.userService.findAll();
return users.map(user => new UserEntity(user));
}

@Get(':id')
@ApiOkResponse({ type: UserEntity })
findOne(@Param('id', ParseIntPipe) id: number) {
return this.userService.findOne(+id);
async findOne(@Param('id', ParseIntPipe) id: number) {
return new UserEntity(
await this.userService.findOne(id)
);
}

@Patch(':id')
@ApiOkResponse({ type: UserEntity })
update(
async update(
@Param('id', ParseIntPipe) id: number,
@Body() updateUserDto: UpdateUserDto,
) {
return this.userService.update(+id, updateUserDto);
return new UserEntity(
await this.userService.update(id, updateUserDto)
);
}

@Delete(':id')
@ApiNoContentResponse()
remove(@Param('id', ParseIntPipe) id: number) {
return this.userService.remove(+id);
async remove(@Param('id', ParseIntPipe) id: number) {
return new UserEntity(
await this.userService.remove(id)
);
}
}
31 changes: 10 additions & 21 deletions backend/src/user/user.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,53 +5,42 @@ import { PrismaService } from 'src/prisma/prisma.service';
import { User, Prisma } from '@prisma/client';
import { hash } from 'bcrypt';

type UserWithoutPassword = Omit<User, 'password'>;

function excludePassword(user: User): Omit<User, 'password'> {
const { password, ...userWithoutPassword } = user;
return userWithoutPassword;
}

@Injectable()
export class UserService {
constructor(private prisma: PrismaService) {}

async create(createUserDto: CreateUserDto): Promise<UserWithoutPassword> {
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 });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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 });
}


findAll(): Promise<UserWithoutPassword[]> {
findAll(): Promise<User[]> {
return this.prisma.user
.findMany()
.then((users) => users.map(excludePassword));
.findMany();
}

findOne(id: number): Promise<UserWithoutPassword> {
findOne(id: number): Promise<User> {
return this.prisma.user
.findUniqueOrThrow({ where: { id: id } })
.then(excludePassword);
.findUniqueOrThrow({ where: { id: id } });
}

update(
id: number,
updateUserDto: UpdateUserDto,
): Promise<UserWithoutPassword> {
): Promise<User> {
return this.prisma.user
.update({
where: { id: id },
data: updateUserDto,
})
.then(excludePassword);
});
}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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,
});
}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking if the user exists before attempting the update is done by ValidationPipe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is already configured to perform that check, so it's unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


remove(id: number): Promise<UserWithoutPassword> {
remove(id: number): Promise<User> {
return this.prisma.user
.delete({
where: { id: id },
})
.then(excludePassword);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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 },
});
}

}