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

[3주차] 심화 과제 #13

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[3주차] 심화 과제 #13

wants to merge 9 commits into from

Conversation

Jin409
Copy link
Member

@Jin409 Jin409 commented May 5, 2023

📝 관련 이슈

closed #11

👩🏻‍💻 내용

메이커스 레포지토리를 참고했습니다!

  • user 도메인 생성
  • post 도메인 생성
  • 유저 등록 기능 구현
  • post 등록 기능 구현
  • post 조회 기능 구현
  • 응답 형태 커스터마이징
  • 예외 처리 기능 구현
  1. 유저 등록 시 / 게시글 등록 시
    스크린샷 2023-05-06 오전 2 30 55

  2. 게시글 조회 시
    스크린샷 2023-05-06 오전 2 31 18

  3. 게시글이 존재하지 않는 경우
    스크린샷 2023-05-06 오전 2 31 40

  4. 유저가 존재하지 않는 경우
    스크린샷 2023-05-06 오전 2 31 59

😸 중점을 둔 부분

  • service 의 분리
    • 유저 조회는 유저 서비스에서 이루어지도록
    • 포스트 조회는 포스트 서비스에서 이루어지도록
  • 간결한 코드 작성
    • orElseThrow 사용
    • 최대한 문자열을 그냥 작성하지 않도록

💡 배운 점

에러 핸들링 오류 발생

  • 로그에도 찍히는데 계속 status code 가 커스텀한 404나 400이 아닌 500으로 떴음
    • 이유: 커스텀한 리스폰스에 @Getter 가 없어서!
      • http 응답 시에 객체를 본문으로 변경하는데, 이 변환타입은 accept 헤더와 일치하는 미디어타입으로 결정한다. getter 가 없으면 해당 객체의 필드값을 찾을 수 없기에 http 본문으로 변환할 수 없기에 HttpMediaTypeNotAcceptableException가 발생한다. 따라서 responseBody 에 사용되는 객체라면 getter 가 있어야 한다.

😹 궁금한 점

  • 보통 unique 와 같은 validation 어노테이션을 dto 에서 쓰는 걸로 알고 있는데, 도메인 단에도 이중으로 쓰는지? 아니면 그냥 dto 단에서만 사용하는지

@Jin409 Jin409 self-assigned this May 5, 2023
@Jin409 Jin409 marked this pull request as ready for review May 5, 2023 17:43
Copy link

@jinsu4755 jinsu4755 left a comment

Choose a reason for hiding this comment

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

세미나 과제라는 점을 보면서 세미나 이외로 그 이상 추가적으로 더 공부해보시거나 혹은 보다 자세히 알아보면 좋을 것 같은 부분에 리뷰 남겨두었어요 :)

내용이 너무 어렵거나 이게 뭔소리지 싶으시면 언제든 질문해주세요~

@AllArgsConstructor
@JsonInclude(JsonInclude.Include.NON_NULL)
@Getter
public class ApiResponse {

Choose a reason for hiding this comment

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

ApiResponse 라는 객체는 정적 팩토리 메서드로도 생성이 가능하지만 동시에 AllArgsConstructor 를 통해서도 생성이 가능하다 생각이 드네요

객체가 여러 방법으로 생성될 수 있기 때문에 함께 작업하는 협업자에게 단순 ApiResponse 를 사용하라 하면 예를 들어
success : "false"
message: "성공"
data: null
과 같은 형태가 만들어지는 휴먼에러 케이스가 발생할 수 있지 않을까요?

객체 자체의 생성을 제한하여 성공/실패 두 가지 경우만 나올 수 있도록 해보는 것은 어떨까요?

Choose a reason for hiding this comment

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

물론 생성자와 팩토리 메서드는 각각의 장단점이 있기 때문에 이를 파악해보시면 좋을 것 같습니다.
Effective Java에서는 "생성자 대신 정적 팩토리 메서드를 고려하라" 라는 부분이 있는 만큼 단순히 제공하는 객체 생성 방식이 아니라 보다 의미있는 객체 생성 방식을 제공하기 위해 고려해보면 좋을 것 같습니다

Comment on lines +24 to +26
public static ApiResponse fail(String message) {
return new ApiResponse(false, message, null);
}

Choose a reason for hiding this comment

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

정적 팩토리 메서드를 사용하는 경우 javadoc 에서 확인이 어렵기 때문에 네이밍 규칙이 존재하는 것으로 알고 있어요

그 중 정적 팩토리 메서드를 사용하는 경우에 하위 타입으로 쉽게 분류한 객체를 생성하는 방식이 있는데 현재의 경우가 "성공 응답"/ "실패 응답" 이라는 하위 타입으로도 만들 수 있는 상황이라 생각됩니다.

실제 타입으로 구분하여 구분하지 않지만 이 경우 사용되는 get[타입 이름]/new[타입이름] 의 형식 혹은 타입 이름을 간단하게 명시하곤 합니다.

success 에서 사용한 것과 같이 fail 또한 동사가 아닌 하위 상태 객체를 만든다는 입장에서 명사의 타입이름을 지어주면 어떨까요?

Comment on lines +12 to +27
@RestControllerAdvice
@Slf4j
public class ExceptionController {

@ExceptionHandler(NotFoundException.class)
public ResponseEntity<ApiResponse> handleNotFoundException(NotFoundException e) {
log.error("handleNotFoundException", e);
return ResponseEntity.status(HttpStatus.NOT_FOUND).body(ApiResponse.fail(e.getMessage()));
}

@ExceptionHandler(MethodArgumentNotValidException.class)
public ResponseEntity<ApiResponse> handleMethodArgumentNotValidException(MethodArgumentNotValidException e) {
log.error("handleMethodArgumentNotValidException", e);
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(ApiResponse.fail(e.getMessage()));
}
}

Choose a reason for hiding this comment

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

현재 처리된 NotFoundException, MethodArgumentNotValidException이외 예외가 발생하면 2 예외와 다른 형태를 가지지 않을까요?

스프링에서는 기본적으로 예외를 ResponseEntityExceptionHandler에서 미리 처리해두기 때문에 해당 ExceptionController에서 처리되지 않은 Exception 은 모두 DefaultHandlerExceptionResolver로 처리하여 서로 다른 처리를 가져가 스프링에서 기본적으로 제공하는 처리로 인해 응답이 날아간다면 클라이언트에서 일괄되지 않은 형태의 에러 응답을 받아 처리가 힘들어지는 경우가 발생할 수 있어요

Choose a reason for hiding this comment

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

ResponseEntityExceptionHandler에서는 아래와 같은 ServletException을 제공합니다.

HttpRequestMethodNotSupportedException.class,
HttpMediaTypeNotSupportedException.class,
HttpMediaTypeNotAcceptableException.class,
MissingPathVariableException.class,
MissingServletRequestParameterException.class,
ServletRequestBindingException.class,
ConversionNotSupportedException.class,
TypeMismatchException.class,
HttpMessageNotReadableException.class,
HttpMessageNotWritableException.class,
MethodArgumentNotValidException.class,
MissingServletRequestPartException.class,
BindException.class,
NoHandlerFoundException.class,
AsyncRequestTimeoutException.class

특히 NotFound의 경우 NoHandlerFoundException 으로 지원하기 때문에 이를 별도로 구현하는 것이 아닌 기본 제공하는 친구를 이용하거나
ResponseEntityExceptionHandler 자체를 상속하여 handleExceptionInternal를 이용하여 모든 예외 응답이 동일하게 처리 될 수 있도록 해보는 것은 어떨까요?

Choose a reason for hiding this comment

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

해당 파일은 과제에서는 큰 문제 없지만 합동 세미나 및 실제 DB를 운영하는 경우 주의가 필요한 파일이에요 :)

해당 글을 읽어보면 좋을 것 같아요

참고로 깃허브에 올라오는 모든 commit은 지워지지 않는다고 보셔도 무방합니다. 따라서 db password가 올라간 경우 파일을 삭제한다 해도 커밋 hash 를 찾으면 조회가 가능하기 때문에 add, commit 전에 불필요한 파일이 snapshot에 추가 되진 않는지 확인해보면 좋습니다

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.

[3주차] 심화 과제
2 participants