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

[4기 - 김민희] SpringBoot Part3 WeeklyMission 제출합니다. #867

Open
wants to merge 27 commits into
base: KimMinheee/week01
Choose a base branch
from

Conversation

KimMinheee
Copy link

📌 과제 설명

  • Spring MVC를 적용해서 JSON을 지원하는 REST API를 개발해보세요
    • 전체 조회기능
    • 조건별 조회기능 (바우처 생성기간 및 특정 할인타입별)
    • 바우처 추가기능
    • 바우처 삭제기능
    • 바우처 아이디로 조회 기능

👩‍💻 요구 사항과 구현 내용

VoucherManagement Rest API 개발

- ExceptionHandler 이용
- BaseResponse를 이용한 응답 데이터 형식 통일화

2주차 이후 수정한 내용

- Voucher, Member, Wallet의 id값을 UUID -> ULID로 수정
- DTO를 계층별로 구분 및 Mapper 이용
- SQL 쿼리 키워드 대문자로 수정
- BaseTimeEntity를 이용하여 created_at 데이터 추가

스크린샷 2023-07-29 오후 12 24 39

- mapStruct
- spring-boot-starter-seb
- ErrorCode 내에서 관리하도록 수정
- Mapper 사용
- Controller Dto @Valid로 유효성 확인
- memberUUID -> memberId로 변경
- created_at 필드 구현
- created_at 컬럼 추가
- 매개변수로 value 값 전달받도록 수정
@KimMinheee KimMinheee self-assigned this Jul 29, 2023
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;

@Component
Copy link

Choose a reason for hiding this comment

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

이 entity는 Component가 아닌것 같아요

*/
@ExceptionHandler(MethodArgumentNotValidException.class)
protected ResponseEntity<ErrorResponse> handleMethodArgumentNotValidException(MethodArgumentNotValidException e) {
log.error("Handle MothodArgumentNotValidException", e.getMessage());
Copy link

Choose a reason for hiding this comment

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

TRACE, DEBUG, INFO, WARN, ERROR 등
로그의 레벨이 나뉘어져 있어요.
각각 나뉜 의도가 있습니다.
MethodArgumentNotValidExceptionr가 발생하면 error일까요?
그렇다면 이 로그는 error일까요?

@RequiredArgsConstructor
@RestControllerAdvice
@Slf4j
public class GlobalExceptionHandler {
Copy link

Choose a reason for hiding this comment

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

이름좋은데요!
Rest Controller에 관련된 예외를 처리하는 handler다 라고 조금 더 명확하게 주면 어떨까요?

protected ResponseEntity<ErrorResponse> handleMethodArgumentNotValidException(MethodArgumentNotValidException e) {
log.error("Handle MothodArgumentNotValidException", e.getMessage());
final ErrorResponse response = ErrorResponse.of(ErrorCode.INVALID_METHOD_ERROR, e.getBindingResult());
return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST);
Copy link

Choose a reason for hiding this comment

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

ResponseEntity.badRequest().body() 라는 정적 팩토리 메소드가 존재합니다.

protected ResponseEntity<ErrorResponse> handleJsonProcessingException(JsonProcessingException ex) {
log.error("handleJsonProcessingException", ex);
final ErrorResponse response = ErrorResponse.of(ErrorCode.REQUEST_BODY_MISSING_ERROR, ex.getMessage());
return new ResponseEntity<>(response, HttpStatus.OK);
Copy link

Choose a reason for hiding this comment

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

ResponseEntity.ok()


return switch (discountType) {
case FIXED ->
new FixedAmountVoucher(UUID.randomUUID(), discountType, new DiscountValue(voucherCreateRequest.discountValue()));
new FixedAmountVoucher(UlidCreator.getUlid().toString(), discountType, new DiscountValue(voucherCreateRequest.discountValue()));
Copy link

Choose a reason for hiding this comment

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

이렇게 static 메소드에 직접 의존하면 테스트하기도 어려울뿐더러
지금처럼 UUID -> ULID로 바뀌었을때도 상당히 많은 부분이 바뀌어야 하네요.
어떻게 해결할 수 있을까요?

* @return BaseResponse<VoucherCreateResponse>
*/
@PostMapping()
public BaseResponse<VoucherCreateResponseData> createVoucher(@Valid @RequestBody VoucherCreateRequestData data) {
Copy link

Choose a reason for hiding this comment

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

RequestData 라는 말이 저는 중복같아요보여요
Data 또는 Request 하나만 써도 될 것 같은데 어떻게 생각하세요?

* @param data
* @return BaseResponse<String>
*/
@PatchMapping("/{voucherId}")
Copy link

Choose a reason for hiding this comment

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

PATCH와 PUT는 어떤 차이점이 있을까요?

Comment on lines +8 to +9
@NotBlank
String discountType,
Copy link

Choose a reason for hiding this comment

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

enum으로도 받을 수 있습니다.

.orElseThrow(() -> new MemberException(NOT_FOUND_MEMBER));

Wallet wallet = new Wallet(UUID.randomUUID(),
Wallet wallet = new Wallet(UlidCreator.getUlid().toString(),
Copy link

Choose a reason for hiding this comment

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

직접의존!

Copy link

@WooSungHwan WooSungHwan left a comment

Choose a reason for hiding this comment

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

민희님 바우처 과제 하시느라 수고 많으셨습니다. 추후 다른 과제에서도 지금처럼 좋은 모습 보여주세요 ㅎㅎ
성실하게 마무리해주셔서 감사합니다.

Comment on lines +89 to +90
final ErrorResponse response = ErrorResponse.of(ErrorCode.INTERNAL_SERVER_ERROR, e.getMessage());
return new ResponseEntity<>(response, HttpStatus.OK);

Choose a reason for hiding this comment

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

Exception은 internal server error가 맞는 것 같아요.

Comment on lines +96 to +101
@ExceptionHandler(MemberException.class)
public ResponseEntity<ErrorResponse> handleNotFoundException(MemberException e) {
log.error("Handle NotFoundException :", e);
final ErrorResponse response = ErrorResponse.of(e.getErrorCode(), e.getMessage());
return new ResponseEntity<>(response, HttpStatus.OK);
}

Choose a reason for hiding this comment

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

404 혹은 400이 맞는 에러코드인 것 같습니다. 멤버를 못찾는 요청이 잘못된 경우(400), 찾는 멤버가 없는 경우(404)

Comment on lines +106 to +111
@ExceptionHandler(VoucherException.class)
public ResponseEntity<ErrorResponse> handlePermissionDeniedException(VoucherException e) {
log.error("Handle PermissionDeniedException :", e);
final ErrorResponse response = ErrorResponse.of(e.getErrorCode(), e.getMessage());
return new ResponseEntity<>(response, HttpStatus.OK);
}

Choose a reason for hiding this comment

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

permission denied에 맞는 http status 를 알아보시면 좋을 것 같네요. 아래도 마찬가지고요.
인증과 인가 두 개념에 대해서 http status 값에 맞는 경우를 탐구해보면 좋을 것 같습니다.

Choose a reason for hiding this comment

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

혹은 단순히 400으로 볼 수도 있고요. 순전히 요청이 잘못됐다고 서버가 판단해야하는 포괄적인 경우 http status 에 의해서 비즈니스가 노출될 수 있으니깐요.

Comment on lines +116 to +121
@ExceptionHandler(WalletException.class)
public ResponseEntity<ErrorResponse> handlePermissionDeniedException(WalletException e) {
log.error("Handle PermissionDeniedException :", e);
final ErrorResponse response = ErrorResponse.of(e.getErrorCode(), e.getMessage());
return new ResponseEntity<>(response, HttpStatus.OK);
}

Choose a reason for hiding this comment

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

permission denied에 맞는 http status 를 알아보시면 좋을 것 같네요. 아래도 마찬가지고요.
인증과 인가 두 개념에 대해서 http status 값에 맞는 경우를 탐구해보면 좋을 것 같습니다.

Choose a reason for hiding this comment

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

혹은 단순히 400으로 볼 수도 있고요. 순전히 요청이 잘못됐다고 서버가 판단해야하는 포괄적인 경우 http status 에 의해서 비즈니스가 노출될 수 있으니깐요.

@@ -23,33 +22,34 @@ public JdbcMemberReaderRepository(JdbcTemplate jdbcTemplate) {

@Override
public List<Member> findAll() {
String sql = "select member_id, member_status, name from member_table";
String sql = "SELECT member_id, member_status, name FROM member_table";

Choose a reason for hiding this comment

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

비슷한 의견으로 member 테이블의 id인데 굳이 member_id 라는 명명이 되어야 하는지도요~

Comment on lines +32 to +43
@GetMapping("/voucher/{voucherId}")
public BaseResponse<WalletGetResponses> getWalletsByVoucherId(@PathVariable String voucherId) {
WalletGetResponses responses = walletService.getWalletsByVoucherId(voucherId);
return new BaseResponse<>(responses);
}


@GetMapping("/member/{memberId}")
public BaseResponse<WalletGetResponses> getWalletsByMemberId(@PathVariable String memberId) {
WalletGetResponses responses = walletService.getWalletsByMemberId(memberId);
return new BaseResponse<>(responses);
}

Choose a reason for hiding this comment

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

지갑 목록에서 바우처 아이디, 멤버 아이디 두 파라미터를 받아 검색하는 지갑 목록 api로도 작성해볼 수 있겠네요 ㅎㅎ

Choose a reason for hiding this comment

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

사실 지금도 좋긴합니다. 오히려 ocp에는 굉장히 만족스러운 행위입니다. 다만 처음에 개발할 당시엔 공통되는 부분은 최대한 로직의 재사용을 할 수 있도록 해주면 좋아요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants