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.

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

[WIP] [backend] Set up Swagger #25

Merged
merged 8 commits into from
Nov 4, 2023
Merged

[WIP] [backend] Set up Swagger #25

merged 8 commits into from
Nov 4, 2023

Conversation

usatie
Copy link
Owner

@usatie usatie commented Nov 4, 2023

とりあえず表示できるように。
http://localhost:4242/api

Screenshot 2023-11-04 at 16 35 35

Prisma公式ブログの中にあったチュートリアルPart1を参考にbackendを修正しました。
This article will teach you how to build a backend REST API with NestJS, Prisma, PostgreSQL and Swagger.とのことだったのですごくちょうどよかった。
https://www.prisma.io/blog/nestjs-prisma-rest-api-7D056s1BmOL0#define-get-articles-endpoint

Summary by CodeRabbit

  • New Features
    • Introduced a search functionality to the application.
  • Documentation
    • Added Swagger documentation for interactive API endpoint exploration.
  • Refactor
    • Reorganized Prisma service into its own module for better code structure.
  • Bug Fixes
    • Updated user service to use findUnique instead of findFirst for more accurate user retrieval.
  • Tests
    • Added new test suite for Prisma service.
  • Chores
    • Updated import paths due to file relocations.
  • Security
    • Passwords are now hashed before being stored and are excluded from API responses.

- `docker compose exec backend yarn add @nestjs/swagger swagger-ui-express`
Copy link
Contributor

coderabbitai bot commented Nov 4, 2023

Walkthrough

The changes primarily focus on enhancing the NestJS backend application by integrating Swagger for API documentation and restructuring the Prisma service. The User module has been updated with DTOs and Swagger decorators, and the Prisma service has been encapsulated in its own module. Password hashing has been introduced in the User service, and the password field is now excluded from the response.

Changes

File(s) Summary
backend/src/main.ts Swagger documentation added to the application.
backend/src/app.module.ts Imported PrismaModule.
backend/src/prisma/prisma.module.ts, backend/src/prisma/prisma.service.ts, backend/src/prisma/prisma.service.spec.ts Added PrismaModule and PrismaService, with corresponding test file.
backend/src/user/user.module.ts, backend/src/user/user.controller.spec.ts, backend/src/user/user.service.spec.ts, backend/src/user/user.service.ts Updated import paths for PrismaService and replaced it with PrismaModule in user.module.ts. Introduced password hashing and exclusion in user.service.ts.
backend/src/user/dto/create-user.dto.ts, backend/src/user/dto/update-user.dto.ts Added CreateUserDto and updated UpdateUserDto with new import path for PartialType.
backend/src/user/entities/user.entity.ts Introduced UserEntity class with Swagger decorators.
backend/src/user/user.controller.ts Updated methods with Swagger decorators and replaced parameters with DTOs.

Poem

🍂 As autumn leaves fall, so does old code retire, 🍁

In with the new, our spirits aspire. 🌟

Swagger docs now light our way, 📚

While Prisma shines in its own array. 💎

Passwords hidden, safe and sound, 🔒

In our NestJS playground. 🎠

So let's celebrate this coding spree, 🎉

With a warm cup of code-brewed tea! ☕️


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2daa22c and 34e176d.
Files ignored due to filter (2)
  • backend/package.json
  • backend/yarn.lock
Files selected for processing (1)
  • backend/src/main.ts (1 hunks)
Additional comments: 2
backend/src/main.ts (2)
  • 1-3: Imports look good and are correctly placed at the top of the file.

  • 19-19: Ensure that the PORT environment variable is correctly set in your environment, and that it doesn't conflict with any other services running on the same machine.

Comment on lines 5 to 21
async function bootstrap() {
const app = await NestFactory.create(AppModule);
app.enableCors(); // enable CORS
app.setGlobalPrefix('api');

const config = new DocumentBuilder()
.setTitle('Pong API')
.setDescription('The Pong API description')
.setVersion('0.1')
.build();

const document = SwaggerModule.createDocument(app, config);
SwaggerModule.setup('api', app, document);

await app.listen(process.env.PORT);
}
bootstrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

The bootstrap function is correctly set up and the Swagger documentation is correctly configured and set up. However, it's a good practice to add error handling for the app.listen() function. This will help catch and handle any errors that might occur when the application tries to listen on the specified port.

- await app.listen(process.env.PORT);
+ try {
+   await app.listen(process.env.PORT);
+ } catch (error) {
+   console.error(`Failed to bind port ${process.env.PORT}`, error);
+ }

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 function bootstrap() {
const app = await NestFactory.create(AppModule);
app.enableCors(); // enable CORS
app.setGlobalPrefix('api');
const config = new DocumentBuilder()
.setTitle('Pong API')
.setDescription('The Pong API description')
.setVersion('0.1')
.build();
const document = SwaggerModule.createDocument(app, config);
SwaggerModule.setup('api', app, document);
await app.listen(process.env.PORT);
}
bootstrap();
async function bootstrap() {
const app = await NestFactory.create(AppModule);
app.enableCors(); // enable CORS
app.setGlobalPrefix('api');
const config = new DocumentBuilder()
.setTitle('Pong API')
.setDescription('The Pong API description')
.setVersion('0.1')
.build();
const document = SwaggerModule.createDocument(app, config);
SwaggerModule.setup('api', app, document);
try {
await app.listen(process.env.PORT);
} catch (error) {
console.error(`Failed to bind port ${process.env.PORT}`, error);
}
}
bootstrap();

- `nest generate module prisma`
- `nest generate service prisma`
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 34e176d and 431807b.
Files selected for processing (8)
  • backend/src/app.module.ts (1 hunks)
  • backend/src/prisma/prisma.module.ts (1 hunks)
  • backend/src/prisma/prisma.service.spec.ts (1 hunks)
  • backend/src/prisma/prisma.service.ts (1 hunks)
  • backend/src/user/user.controller.spec.ts (1 hunks)
  • backend/src/user/user.module.ts (1 hunks)
  • backend/src/user/user.service.spec.ts (1 hunks)
  • backend/src/user/user.service.ts (1 hunks)
Files skipped from review due to trivial changes (2)
  • backend/src/app.module.ts
  • backend/src/prisma/prisma.module.ts
Additional comments: 7
backend/src/user/user.controller.spec.ts (1)
  • 1-4: The import path for PrismaService has been updated correctly. Ensure that the file prisma.service.ts is indeed located in the src/prisma/ directory.
backend/src/user/user.service.spec.ts (1)
  • 1-3: The import path for PrismaService has been updated. Ensure that the file prisma.service.ts has been moved to the prisma directory and that the path is correct.
- import { PrismaService } from 'src/prisma.service';
+ import { PrismaService } from 'src/prisma/prisma.service';
backend/src/prisma/prisma.service.ts (2)
  • 1-2: The import statements are correct and necessary for the PrismaService class.

  • 4-5: The PrismaService class is correctly decorated with @Injectable(), which allows it to be managed by NestJS's dependency injection system. It also correctly extends PrismaClient, which provides access to your database via Prisma.

backend/src/user/user.module.ts (1)
  • 1-11: The changes look good. The PrismaModule is now being imported instead of PrismaService, which is the correct way to use Prisma with NestJS. The PrismaModule is also added to the imports array in the @Module decorator, which is necessary for dependency injection. The PrismaService is no longer provided directly in this module, which is also correct because it will be provided by the PrismaModule.
backend/src/prisma/prisma.service.spec.ts (1)
  • 1-18: The test case is well written and follows the standard structure of a unit test in NestJS. It correctly imports the necessary modules and services, sets up the testing module, and checks if the service is defined. However, it would be beneficial to add more test cases to verify the functionality of the PrismaService methods.
+ it('should perform some operation', async () => {
+   const result = await service.someOperation();
+   expect(result).toBe(expectedResult);
+ });
backend/src/user/user.service.ts (1)
  • 1-6: The import statement for PrismaService has been updated. Ensure that the file path is correct and the PrismaService file is located at 'src/prisma/prisma.service'.

@usatie usatie changed the title [backend] Set up Swagger [WIP] [backend] Set up Swagger Nov 4, 2023
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 431807b and 435d6ef.
Files selected for processing (4)
  • backend/src/user/dto/create-user.dto.ts (1 hunks)
  • backend/src/user/entities/user.entity.ts (1 hunks)
  • backend/src/user/user.controller.ts (1 hunks)
  • backend/src/user/user.service.ts (1 hunks)
Additional comments: 4
backend/src/user/dto/create-user.dto.ts (1)
  • 1-12: The changes look good. The ApiProperty decorator is used correctly to provide metadata for Swagger to generate the API documentation. The required option is used correctly to indicate that the name property is optional. However, it's important to ensure that the password is stored securely and not returned in any API responses. Also, consider adding validation decorators from the class-validator package to enforce data integrity.
import { ApiProperty } from '@nestjs/swagger';
import { IsEmail, IsOptional, IsString, MinLength } from 'class-validator';

export class CreateUserDto {
  @ApiProperty()
  @IsEmail()
  email: string;

  @ApiProperty({ required: false })
  @IsOptional()
  @IsString()
  name?: string;

  @ApiProperty()
  @IsString()
  @MinLength(8)
  password: string;
}
backend/src/user/user.controller.ts (1)
  • 8-55: The Swagger decorators are correctly used to document the API endpoints. The @ApiTags, @ApiCreatedResponse, @ApiOkResponse, and @ApiNoContentResponse decorators are used appropriately to describe the endpoints, their responses, and to group them under the 'user' tag. The DTOs and the UserEntity are also correctly used in the decorators to describe the shape of the data. However, ensure that the UserEntity accurately represents the shape of the user data.
backend/src/user/user.service.ts (2)
  • 23-25: The findOne method has been updated to use findUnique instead of findFirst. This is a good change, as findUnique is more appropriate for finding a record by its unique identifier.

  • 27-31: The update method has been updated to use updateUserDto instead of data. This is a good change, as it makes the method signature more explicit and easier to understand.

Comment on lines 1 to 16
import { User } from '@prisma/client';
import { ApiProperty } from '@nestjs/swagger';

export class UserEntity implements User {
@ApiProperty()
id: number;

@ApiProperty()
email: string;

@ApiProperty({ required: false, nullable: true })
name: string | null;

@ApiProperty()
password: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The UserEntity class is correctly implementing the User interface and the ApiProperty decorators are correctly used to provide API documentation. However, it's not a good practice to expose sensitive data like passwords in the API documentation. Consider removing the ApiProperty decorator from the password field.

-  @ApiProperty()
  password: string;

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
import { User } from '@prisma/client';
import { ApiProperty } from '@nestjs/swagger';
export class UserEntity implements User {
@ApiProperty()
id: number;
@ApiProperty()
email: string;
@ApiProperty({ required: false, nullable: true })
name: string | null;
@ApiProperty()
password: string;
}
import { User } from '@prisma/client';
import { ApiProperty } from '@nestjs/swagger';
export class UserEntity implements User {
@ApiProperty()
id: number;
@ApiProperty()
email: string;
@ApiProperty({ required: false, nullable: true })
name: string | null;
- // @ApiProperty()
password: string;
}

}

@Delete(':id')
@ApiNoContentResponse()
remove(@Param('id') id: string) {
return this.userService.remove(+id);
Copy link
Contributor

Choose a reason for hiding this comment

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

The +id operation is used to convert the id from a string to a number. If the id is not a number, this operation will return NaN, which could lead to unexpected behavior. If the id is expected to be a number, it would be better to parse it with parseInt(id, 10) and handle the case where id is not a number. If the id is not expected to be a number, the + operation should be removed.

- return this.userService.findOne(+id);
+ return this.userService.findOne(parseInt(id, 10));

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
return this.userService.remove(+id);
return this.userService.findOne(parseInt(id, 10));

Comment on lines 12 to 17
async create(createUserDto: CreateUserDto): Promise<User> {
const saltRounds = 10;
const hashedPassword = await bcrypt.hash(data.password, saltRounds);
data.password = hashedPassword;
return this.prisma.user.create({ data });
const hashedPassword = await bcrypt.hash(createUserDto.password, saltRounds);
createUserDto.password = hashedPassword;
return this.prisma.user.create({ data: createUserDto });
}
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 is being hashed before being stored, which is a good security practice. However, directly modifying the createUserDto object to replace the password with the hashed password is not recommended. It's better to create a new object for the data to be stored, to avoid potential side effects from modifying the input object.

-    createUserDto.password = hashedPassword;
-    return this.prisma.user.create({ data: createUserDto });
+    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 bcrypt.hash(data.password, saltRounds);
data.password = hashedPassword;
return this.prisma.user.create({ data });
const hashedPassword = await bcrypt.hash(createUserDto.password, saltRounds);
createUserDto.password = hashedPassword;
return this.prisma.user.create({ data: createUserDto });
}
async create(createUserDto: CreateUserDto): Promise<User> {
const saltRounds = 10;
const hashedPassword = await bcrypt.hash(createUserDto.password, saltRounds);
const userData = { ...createUserDto, password: hashedPassword };
return this.prisma.user.create({ data: userData });
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 435d6ef and 11796eb.
Files selected for processing (1)
  • backend/src/user/user.service.ts (1 hunks)
Additional comments: 3
backend/src/user/user.service.ts (3)
  • 1-6: Imports look good and are correctly ordered. The hash function is imported directly from bcrypt which is a good practice as it only imports what is needed.

  • 23-25: The findOne function now uses findUnique instead of findFirst. This is a good change as findUnique is more appropriate when searching by ID.

  • 27-32: The update function now accepts an UpdateUserDto object. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

Comment on lines 12 to 17
async create(createUserDto: CreateUserDto): Promise<User> {
const saltRounds = 10;
const hashedPassword = await bcrypt.hash(data.password, saltRounds);
data.password = hashedPassword;
return this.prisma.user.create({ data });
const hashedPassword = await hash(createUserDto.password, saltRounds);
createUserDto.password = hashedPassword;
return this.prisma.user.create({ data: createUserDto });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The create function now accepts a CreateUserDto object and uses the hash function to hash the password. This is a good security practice. However, it's important to note that the createUserDto object is being mutated here. If this object is used elsewhere after this function call, it might lead to unexpected behavior. Consider creating a copy of the object before mutating it.

  async create(createUserDto: CreateUserDto): Promise<User> {
    const saltRounds = 10;
    const hashedPassword = await hash(createUserDto.password, saltRounds);
-   createUserDto.password = hashedPassword;
+   const userDtoWithHashedPassword = { ...createUserDto, password: hashedPassword };
-   return this.prisma.user.create({ data: createUserDto });
+   return this.prisma.user.create({ data: userDtoWithHashedPassword });
  }

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 bcrypt.hash(data.password, saltRounds);
data.password = hashedPassword;
return this.prisma.user.create({ data });
const hashedPassword = await hash(createUserDto.password, saltRounds);
createUserDto.password = hashedPassword;
return this.prisma.user.create({ data: createUserDto });
}
async create(createUserDto: CreateUserDto): Promise<User> {
const saltRounds = 10;
const hashedPassword = await hash(createUserDto.password, saltRounds);
const userDtoWithHashedPassword = { ...createUserDto, password: hashedPassword };
return this.prisma.user.create({ data: userDtoWithHashedPassword });
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 11796eb and c26f6a3.
Files selected for processing (2)
  • backend/src/prisma/prisma.module.ts (1 hunks)
  • backend/src/user/user.controller.ts (1 hunks)
Files skipped from review due to trivial changes (1)
  • backend/src/prisma/prisma.module.ts
Additional comments: 4
backend/src/user/user.controller.ts (4)
  • 8-19: The import statements are well organized and the necessary modules are imported for Swagger documentation.

  • 22-23: The @ApiTags('user') decorator is correctly used to categorize the API endpoints under the 'user' tag in the Swagger UI.

  • 27-31: The create method is correctly decorated with @ApiCreatedResponse({ type: UserEntity }) to indicate the response schema in the Swagger UI. The method signature is updated to accept createUserDto of type CreateUserDto.

  • 33-37: The findAll method is correctly decorated with @ApiOkResponse({ type: [UserEntity] }) to indicate the response schema in the Swagger UI.

Comment on lines 39 to 43
@Get(':id')
@ApiOkResponse({ type: UserEntity })
findOne(@Param('id') id: string) {
return this.userService.findOne(+id);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The findOne method is correctly decorated with @ApiOkResponse({ type: UserEntity }) to indicate the response schema in the Swagger UI. However, the +id operation is used to convert the id from a string to a number. If the id is not a number, this operation will return NaN, which could lead to unexpected behavior. If the id is expected to be a number, it would be better to parse it with parseInt(id, 10) and handle the case where id is not a number. If the id is not expected to be a number, the + operation should be removed.

- return this.userService.findOne(+id);
+ return this.userService.findOne(parseInt(id, 10));

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
@Get(':id')
@ApiOkResponse({ type: UserEntity })
findOne(@Param('id') id: string) {
return this.userService.findOne(+id);
}
@Get(':id')
@ApiOkResponse({ type: UserEntity })
findOne(@Param('id') id: string) {
return this.userService.findOne(parseInt(id, 10));
}

Comment on lines 45 to 49
@Patch(':id')
update(
@Param('id') id: string,
@Body() userData: { name?: string; email?: string; password?: string },
) {
return this.userService.update(+id, userData);
@ApiOkResponse({ type: UserEntity })
update(@Param('id') id: string, @Body() updateUserDto: UpdateUserDto) {
return this.userService.update(+id, updateUserDto);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The update method is correctly decorated with @ApiOkResponse({ type: UserEntity }) to indicate the response schema in the Swagger UI. The method signature is updated to accept updateUserDto of type UpdateUserDto. The same issue with the +id operation as in the findOne method applies here.

- return this.userService.update(+id, updateUserDto);
+ return this.userService.update(parseInt(id, 10), 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
@Patch(':id')
update(
@Param('id') id: string,
@Body() userData: { name?: string; email?: string; password?: string },
) {
return this.userService.update(+id, userData);
@ApiOkResponse({ type: UserEntity })
update(@Param('id') id: string, @Body() updateUserDto: UpdateUserDto) {
return this.userService.update(+id, updateUserDto);
}
@Patch(':id')
@ApiOkResponse({ type: UserEntity })
update(@Param('id') id: string, @Body() updateUserDto: UpdateUserDto) {
return this.userService.update(parseInt(id, 10), updateUserDto);
}

Comment on lines 51 to 55
@Delete(':id')
@ApiNoContentResponse()
remove(@Param('id') id: string) {
return this.userService.remove(+id);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The remove method is correctly decorated with @ApiNoContentResponse() to indicate that no content is returned in the response. The same issue with the +id operation as in the findOne method applies here.

- return this.userService.remove(+id);
+ return this.userService.remove(parseInt(id, 10));

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
@Delete(':id')
@ApiNoContentResponse()
remove(@Param('id') id: string) {
return this.userService.remove(+id);
}
@Delete(':id')
@ApiNoContentResponse()
remove(@Param('id') id: string) {
return this.userService.remove(parseInt(id, 10));
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between c26f6a3 and 03bfb19.
Files selected for processing (4)
  • backend/src/user/dto/update-user.dto.ts (1 hunks)
  • backend/src/user/entities/user.entity.ts (1 hunks)
  • backend/src/user/user.controller.ts (1 hunks)
  • backend/src/user/user.service.ts (1 hunks)
Additional comments: 8
backend/src/user/dto/update-user.dto.ts (1)
  • 1-4: The change from @nestjs/mapped-types to @nestjs/swagger for PartialType is correct. This is because PartialType has been moved to @nestjs/swagger in newer versions of NestJS. Ensure that the version of NestJS being used supports this change.
- import { PartialType } from '@nestjs/mapped-types';
+ import { PartialType } from '@nestjs/swagger';
backend/src/user/entities/user.entity.ts (1)
  • 1-15: The UserEntity class is correctly implementing the User interface from @prisma/client. The ApiProperty decorator is used appropriately to document the API. However, the password field is not decorated with ApiProperty. If this is intentional (for example, to avoid exposing sensitive data in the API documentation), it's fine. Otherwise, consider adding the decorator for consistency.
+ @ApiProperty({ required: false })
  password: string;

Also, ensure that the password field is not returned in API responses to avoid exposing sensitive data. This can be done in the service layer when mapping the data to DTOs.

backend/src/user/user.service.ts (2)
  • 1-57: The changes in this file are well-structured and follow best practices. The use of DTOs for data transfer, the exclusion of password fields from the returned user data, and the use of bcrypt for password hashing are all good security practices. The use of Prisma for database operations is also a good choice for type safety and developer productivity.

  • 19-24: Ensure that the saltRounds value is appropriate for your security requirements. A higher value will increase the time taken to hash the password, which can slow down the response time of your API, but it will also make the hashed password more secure.

backend/src/user/user.controller.ts (4)
  • 8-19: The import statements are well organized and the Swagger decorators are correctly imported.

  • 22-24: The UserController class is correctly decorated with @ApiTags('user') for Swagger documentation.

  • 27-31: The create method is correctly decorated with @ApiCreatedResponse({ type: UserEntity }) to indicate the response schema in the Swagger UI.

  • 33-37: The findAll method is correctly decorated with @ApiOkResponse({ type: [UserEntity] }) to indicate the response schema in the Swagger UI.

Comment on lines +38 to 48
update(
id: number,
updateUserDto: UpdateUserDto,
): Promise<UserWithoutPassword> {
return this.prisma.user
.update({
where: { id: id },
data: updateUserDto,
})
.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.

In the update function, if the updateUserDto contains a new password, it should be hashed before being stored in the database. This is not currently being done. Here's a proposed fix:

  update(
    id: number,
    updateUserDto: UpdateUserDto,
  ): Promise<UserWithoutPassword> {
+   if (updateUserDto.password) {
+     const saltRounds = 10;
+     updateUserDto.password = await hash(updateUserDto.password, saltRounds);
+   }
    return this.prisma.user
      .update({
        where: { id: id },
        data: updateUserDto,
      })
      .then(excludePassword);
  }

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> {
return this.prisma.user
.update({
where: { id: id },
data: updateUserDto,
})
.then(excludePassword);
}
update(
id: number,
updateUserDto: UpdateUserDto,
): Promise<UserWithoutPassword> {
if (updateUserDto.password) {
const saltRounds = 10;
updateUserDto.password = await hash(updateUserDto.password, saltRounds);
}
return this.prisma.user
.update({
where: { id: id },
data: updateUserDto,
})
.then(excludePassword);
}

Comment on lines 41 to 43
findOne(@Param('id') id: string) {
return this.userService.findOne(+id);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The +id operation is used to convert the id from a string to a number. If the id is not a number, this operation will return NaN, which could lead to unexpected behavior. If the id is expected to be a number, it would be better to parse it with parseInt(id, 10) and handle the case where id is not a number. If the id is not expected to be a number, the + operation should be removed.

- return this.userService.findOne(+id);
+ return this.userService.findOne(parseInt(id, 10));

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
findOne(@Param('id') id: string) {
return this.userService.findOne(+id);
}
findOne(@Param('id') id: string) {
return this.userService.findOne(parseInt(id, 10));
}

Comment on lines +47 to 49
update(@Param('id') id: string, @Body() updateUserDto: UpdateUserDto) {
return this.userService.update(+id, updateUserDto);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The same issue with the +id operation as in the findOne method applies here.

- return this.userService.update(+id, updateUserDto);
+ return this.userService.update(parseInt(id, 10), 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(@Param('id') id: string, @Body() updateUserDto: UpdateUserDto) {
return this.userService.update(+id, updateUserDto);
}
update(@Param('id') id: string, @Body() updateUserDto: UpdateUserDto) {
return this.userService.update(parseInt(id, 10), updateUserDto);
}

Comment on lines 53 to 55
remove(@Param('id') id: string) {
return this.userService.remove(+id);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The same issue with the +id operation as in the findOne method applies here.

- return this.userService.remove(+id);
+ return this.userService.remove(parseInt(id, 10));

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(@Param('id') id: string) {
return this.userService.remove(+id);
}
remove(@Param('id') id: string) {
return this.userService.remove(parseInt(id, 10));
}

@usatie usatie merged commit 4aad149 into main Nov 4, 2023
1 check passed
@usatie usatie deleted the setup/swagger branch November 4, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant