Redesign/review scotch #305
Conversation
add review liked patch api / not tested
add review put api
clean code / change review like response type / update swagger
update swagger
update resp type / update swagger
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| return await this.reviewsV2Service.updateReviewLiked(body, user) | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드 패치에 대해 몇 가지 잠재적 버그 및 개선 사항을 지적하겠습니다.
-
입력 검증 부족:
@Param('reviewId') reviewId: number로 받아오는 reviewId의 입력 검증이 없습니다. API에서 유효한 숫자가 아닌 값이 들어올 경우 'NaN'이 발생할 수 있습니다. 이에 대한 검증 로직을 추가하는 것이 좋습니다. -
HTTP 메서드 적절성:
updateReviewV2메서드는 PUT 메서드를 사용하는데, RESTful API 스타일에 따르면 영속적 리소스의 전체 수정에 적합합니다. 그러나 이 메서드가 일부 속성만 수정할 경우 PATCH 메서드를 사용하는 것이 더 적절할 수 있습니다. -
비동기 호출 처리:
await로 호출하는 서비스 메서드에서 오류가 발생할 경우, 적절한 에러 핸들링이 필요합니다. 예를 들어 try-catch 블록을 사용하여 예외를 잡고, 관련된 사용자에게 더 명확한 에러 메시지를 제공할 수 있습니다. -
리턴 타입 일관성:
updateReviewV2메서드는 항상IReviewV2.UpdateResponseDto를 반환한다고 가정하고 있는데, 이 과정에서 서비스 메서드가 무엇을 반환하는지 확인해야 합니다. 반환값의 일관성을 보장하기 위해 확인이 필요합니다. -
주석 및 문서화 부족: 각 메서드에 대한 주석이 부족합니다. 이 코드의 목적과 동작, 파라미터에 대한 설명이 있으면 이해하기 쉬워집니다.
위 사항들을 고려하여 코드를 개선하는 것이 좋습니다.
There was a problem hiding this comment.
Pull Request Overview
This PR adds two new API endpoints for the V2 reviews API: an endpoint to update an existing review and an endpoint to like/unlike reviews. Additionally, it includes Swagger documentation updates for these new endpoints and removes the lang property from a schema definition.
- Added PUT
/api/v2/reviews/{reviewId}endpoint for updating reviews - Added PATCH
/api/v2/reviews/{reviewId}/likedendpoint for liking/unliking reviews - Updated Swagger documentation to reflect new API endpoints and schemas
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/server/src/swagger.json | Added OpenAPI definitions for new review endpoints, removed lang property from a schema, and updated file path references |
| apps/server/docs/swagger.json | Mirror of swagger.json changes with identical OpenAPI documentation updates |
| apps/server/src/modules/reviews/v2/reviews.v2.service.ts | Implemented updateReviewV2 and updateReviewLiked service methods with validation and messaging queue integration |
| apps/server/src/modules/reviews/v2/reviews.v2.controller.ts | Added controller methods for PUT and PATCH endpoints with parameter validation |
| apps/server/src/common/interfaces/v2/IReviewV2.ts | Defined DTOs for update operations: UpdateDto, UpdateResponseDto, and PatchLikedDto |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").NotificationRequest" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").NotificationRequest" |
There was a problem hiding this comment.
Schema references contain absolute file paths that are specific to a developer's local machine. These should use relative paths or symbolic references to ensure the Swagger documentation works consistently across different development environments.
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").NotificationRequest" | |
| "$ref": "#/components/schemas/INotification.NotificationRequest" |
| if (reviewId !== body.reviewId) { | ||
| throw new HttpException('Path param and body id not match', HttpStatus.BAD_REQUEST) | ||
| } | ||
| return await this.reviewsV2Service.updateReviewLiked(body, user) |
There was a problem hiding this comment.
The reviewId is redundantly specified in both the path parameter and request body. Consider removing reviewId from PatchLikedDto since it's already available in the URL path, which would eliminate the need for this validation check.
| if (reviewId !== body.reviewId) { | |
| throw new HttpException('Path param and body id not match', HttpStatus.BAD_REQUEST) | |
| } | |
| return await this.reviewsV2Service.updateReviewLiked(body, user) | |
| return await this.reviewsV2Service.updateReviewLiked(reviewId, body, user) |
| "minLength": 1, | ||
| "type": "number" |
There was a problem hiding this comment.
The minLength constraint is incorrectly applied to a number type. The minLength validation is only applicable to strings, not numbers. This should be removed or replaced with minimum if a minimum value constraint is needed.
| "minLength": 1, | ||
| "type": "number" |
There was a problem hiding this comment.
The minLength constraint is incorrectly applied to a number type. The minLength validation is only applicable to strings, not numbers. This should be removed.
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").NotificationRequest" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").NotificationRequest" |
There was a problem hiding this comment.
Schema references contain absolute file paths that are specific to a developer's local machine. These should use relative paths or symbolic references to ensure the Swagger documentation works consistently across different development environments.
| "minLength": 1, | ||
| "type": "number" |
There was a problem hiding this comment.
The minLength constraint is incorrectly applied to a number type. The minLength validation is only applicable to strings, not numbers. This should be removed or replaced with minimum if a minimum value constraint is needed.
| "minLength": 1, | ||
| "type": "number" |
There was a problem hiding this comment.
The minLength constraint is incorrectly applied to a number type. The minLength validation is only applicable to strings, not numbers. This should be removed.
| default: | ||
| throw new HttpException(`Invalid action: ${body.action}`, HttpStatus.BAD_REQUEST) |
There was a problem hiding this comment.
The default case in the switch statement is unreachable due to TypeScript's type narrowing from the 'like' | 'unlike' union type. Since the action field is already validated by the DTO decorators (@isin(['like', 'unlike'])), this default case will never execute and can be removed.
| default: | |
| throw new HttpException(`Invalid action: ${body.action}`, HttpStatus.BAD_REQUEST) |
PR - IsNotEmpty check add
| @ApiProperty() | ||
| @IsNumber() | ||
| @IsNotEmpty() | ||
| @Min(0) |
There was a problem hiding this comment.
코드 변화는 특정 필드의 유효성 검사에 대한 수정으로 보입니다. 그러나 몇 가지 잠재적인 문제점과 개선 사항이 있습니다.
-
유효성 검사 강화:
@IsNotEmpty()데코레이터는 숫자 타입의 필드에 사용된 것으로 보입니다. 숫자 필드는 비어 있을 수 있는 만큼, 이 데코레이터가 숫자 타입의 요구사항을 제대로 반영하지 않을 수 있습니다. 대신@IsDefined()를 사용하는 것이 더 적절할 수 있습니다. 이는 갖추어야 할 필드를 명시적으로 요구하게 합니다. -
유효성 검사 범위:
@Min(0)및@Max(5)데코레이터와 함께 사용할 경우,load와grade의 정의가 코드의 다른 부분과 어울리는지 다시 한번 검토해야 합니다. 이 범위가 비즈니스 로직에서 적절한지 확인이 필요합니다. -
일관성 유지: 여러 필드에서 비슷한 종류의 유효성 검사 데코레이터를 추가하면, 코드의 가독성을 높이기 위해서 일관되게 적용해야 합니다. 그러므로 다른 관련 필드들도 같은 방식으로 정리되어야 합니다.
-
테스트 케이스: 추가된 유효성 검사에 따른 테스트 케이스가 적절히 추가되었는지 확인해야 합니다. 새로 추가된 조건으로 인한 오류를 미리 방지하고, 모든 사용 시나리오를 커버해야 합니다.
따라서, 병합하기 전에 위의 개선 사항을 고려하길 권장합니다.
PUT /api/v2/reviews/{reviewId}
PATCH /api/v2/reviews/{reviewId}/liked