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

#327 fridge exception 구조 변경 및 assembler 삭제 #328

Merged
merged 9 commits into from
Jul 12, 2024

Conversation

psyeon1120
Copy link
Member

📍 이슈 번호


🛠️ 작업 내용

  1. 잘못되어 있는 예외처리 조금씩 수정하면서 fridge exception 구조 변경했습니다.
  2. fridge, fridgeFood, notification assembler에서 각각 적합한 위치로 옮긴 후 삭제했습니다.
  3. 파라미터에서 idx를 id로 변경했습니다.
  4. ExceptionAdvice에 자주 발생하는 에러들을 적용시켰습니다.

🎸 기타 사항

  1. ReturnCode에서 오류메시지 조금씩 다듬고 순서 변경했습니다. 변경사항은 swagger에 모두 적용했습니다.
  2. '존재하지 않는 사용자입니다.'를 언제나 검증해야 되는지 의문입니다. API 호출할 때 loginStatus로 확인을 하는데 또 안해도 될 것 같기도 합니다. 그리고 몇몇 API는 검증하지 않는 곳도 있어서 같이 생각해봐요!
  3. '존재하지 않는 카테고리입니다'처럼 요청값이 잘못들어온건 다 INVALID_PARAM이나 다른 공통적인 오류로 처리하는 건 어떤가요? 지금은 해당 오류들의 HttpStatus를 BAD_REQUEST로 변경해놨습니다.

@psyeon1120 psyeon1120 added 📝 docs 문서 변경 ♻️ refactor 리팩토링 labels Jul 11, 2024
@psyeon1120 psyeon1120 self-assigned this Jul 11, 2024
Copy link
Member

@sojungpp sojungpp left a comment

Choose a reason for hiding this comment

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

  1. ReturnCode에서 오류메시지 조금씩 다듬고 순서 변경했습니다. 변경사항은 swagger에 모두 적용했습니다.

확인했습니다! idx 변경이랑 엄청 많았는데 넘 고생했고 감사합니답 🤍
returnCode 관련 리뷰 확인 한 번만 부탁드려요 !


  1. '존재하지 않는 사용자입니다.'를 언제나 검증해야 되는지 의문입니다. API 호출할 때 loginStatus로 확인을 하는데 또 안해도 될 것 같기도 합니다. 그리고 몇몇 API는 검증하지 않는 곳도 있어서 같이 생각해봐요!

오 저도 loginStatus로 한 번에 검증하는 거 좋은 것 같아요!
그러면 현재 loginStatus 로직에서 추가로 예외 처리(탈퇴 회원 검증 등) 해줘야 할 것 같은데 이 부분은 따로 이슈파서 제가 할까요 ?

@Nullable
@Override
public Object resolveArgument(@NotNull MethodParameter parameter,
ModelAndViewContainer mavContainer,
@NotNull NativeWebRequest webRequest,
WebDataBinderFactory binderFactory) throws Exception
{
Auth auth = parameter.getMethodAnnotation(Auth.class);
if (auth == null)
throw new BaseException(INTERNAL_SERVER_ERROR);
String accessToken = webRequest.getHeader(Objects.requireNonNull(env.getProperty("jwt.auth-header")));
if(accessToken == null || !tokenUtils.isValidToken(tokenUtils.parseJustTokenFromFullToken(accessToken)))
return LoginStatus.getNotLoginStatus();
Long userIdx = Long.valueOf(tokenUtils.getUserIdFromFullToken(accessToken));
if (!auth.optional() && userIdx == null) {
return LoginStatus.getNotLoginStatus();
}
return LoginStatus.builder().isLogin(true).userIdx(userIdx).build();
}


  1. '존재하지 않는 카테고리입니다'처럼 요청값이 잘못들어온건 다 INVALID_PARAM이나 다른 공통적인 오류로 처리하는 건 어떤가요? 지금은 해당 오류들의 HttpStatus를 BAD_REQUEST로 변경해놨습니다.

좋습니다-! 추가 의견 리뷰로 남겼습니다 확인 부탁드려요 !

Comment on lines +35 to +47
@ResponseStatus(HttpStatus.BAD_REQUEST)
@ExceptionHandler(
value = {
HttpMessageNotReadableException.class,
HttpRequestMethodNotSupportedException.class,
MissingServletRequestParameterException.class,
MethodArgumentTypeMismatchException.class,
DateTimeParseException.class
}
)
protected ResponseCustom handleBaseException(Exception e) {
return ResponseCustom.error(ReturnCode.INVALID_PARAM);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@ResponseStatus(HttpStatus.BAD_REQUEST)
@ExceptionHandler(
value = {
HttpMessageNotReadableException.class,
HttpRequestMethodNotSupportedException.class,
MissingServletRequestParameterException.class,
MethodArgumentTypeMismatchException.class,
DateTimeParseException.class
}
)
protected ResponseCustom handleBaseException(Exception e) {
return ResponseCustom.error(ReturnCode.INVALID_PARAM);
}
@ResponseStatus(HttpStatus.BAD_REQUEST)
@ExceptionHandler(
value = {
HttpMessageNotReadableException.class,
HttpRequestMethodNotSupportedException.class,
MissingServletRequestParameterException.class,
MethodArgumentTypeMismatchException.class,
DateTimeParseException.class
}
)
protected ResponseCustom handleBaseException(Exception e) {
// 제안 부분
FieldError error = Objects.requireNonNull(e.getFieldError());
return ResponseCustom.error(ReturnCode.INVALID_PARAM, String.format("%s", error.getDefaultMessage()));
}

INVALID_PARAM 으로 공통으로 처리하는 방법 간편하고 좋은 것 같아요!
여기서 추가로 error.getDefaultMessage() 통해서 어떤 컬럼에서 문제가 발생했는지 알 수 있는데,
프론트가 필요하면 ResponseCustom의 data로 같이 넘겨주고, 필요 없다면 내부 로그로 저장시켜두는거 어떤가요 ?
(테스트나 문제 발생 대비)

Copy link
Member Author

Choose a reason for hiding this comment

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

이건 또 무슨 새로운 기능인거죠?! 근데 getDefaultMessage를 하면 각 에러별로 내장 에러메세지가 나오는데 그걸 그대로 응답에 보여주는 건 좀 그럴 것 같아요! 내부 로그에만 기록하는 건 어떤가요??

Copy link
Member

Choose a reason for hiding this comment

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

로그 기록하는 거 좋아요! 로그는 관련해서 같이 따로 얘기해서 적용하는걸로 ~

Comment on lines +31 to +36
ALREADY_EXIST_FOOD_NAME("F0002", HttpStatus.CONFLICT, "이미 존재하는 식품명입니다."),
INVALID_FOOD_DELETE_STATUS("F0003", HttpStatus.BAD_REQUEST, "존재하지 않는 식품삭제 타입입니다."),

// Fridge(Refrigerator)
NOT_FOUND_FRIDGE("R0000", HttpStatus.CONFLICT, "존재하지 않는 냉장고입니다."),
STILL_MEMBER_EXIST("R0001", HttpStatus.CONFLICT, "해당 냉장고에 사용자가 존재합니다."),
Copy link
Member

Choose a reason for hiding this comment

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

보다가 생각났는데 ReturnCode 공통적으로 네이밍해서 쓸까요?
'이미 존재하는 것'에 대한 네이밍이 ALREADY_EXIST_FOOD_NAME STILL_MEMBER_EXIST 이렇게 2가지로 쓰이고 있는데 통일 시키기 ?!

Copy link
Member Author

Choose a reason for hiding this comment

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

그렇게 하면 어떤 데이터든 겹치는 데이터가 있을 때 ALREADY_EXIST("해당 데이터가 이미 존재합니다.")로 같은 오류를 내는 건가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

근데 STILL_MEMBER_EXIST는 이미 존재한다는 의미보다는 냉장고에 멤버가 아직 존재해서 삭제를 하지 못할 때 내는 오류입니다! 다른 부분에서 더 공통적으로 쓰일 일이 있으면 그런식으로 쓰면 좋을 것 같습니당

Copy link
Member

Choose a reason for hiding this comment

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

아 전자를 의미한 건 아니구 이미 냉장고에 해당 사용자가 중복으로 있을 때 사용하는 에러인 줄 알았어요.! 넹 좋습니도

@psyeon1120
Copy link
Member Author

오 저도 loginStatus로 한 번에 검증하는 거 좋은 것 같아요! 그러면 현재 loginStatus 로직에서 추가로 예외 처리(탈퇴 회원 검증 등) 해줘야 할 것 같은데 이 부분은 따로 이슈파서 제가 할까요 ?

@Nullable
@Override
public Object resolveArgument(@NotNull MethodParameter parameter,
ModelAndViewContainer mavContainer,
@NotNull NativeWebRequest webRequest,
WebDataBinderFactory binderFactory) throws Exception
{
Auth auth = parameter.getMethodAnnotation(Auth.class);
if (auth == null)
throw new BaseException(INTERNAL_SERVER_ERROR);
String accessToken = webRequest.getHeader(Objects.requireNonNull(env.getProperty("jwt.auth-header")));
if(accessToken == null || !tokenUtils.isValidToken(tokenUtils.parseJustTokenFromFullToken(accessToken)))
return LoginStatus.getNotLoginStatus();
Long userIdx = Long.valueOf(tokenUtils.getUserIdFromFullToken(accessToken));
if (!auth.optional() && userIdx == null) {
return LoginStatus.getNotLoginStatus();
}
return LoginStatus.builder().isLogin(true).userIdx(userIdx).build();
}

오오 네 좋아요!! 부탁드립니다🩷

@sojungpp
Copy link
Member

오 저도 loginStatus로 한 번에 검증하는 거 좋은 것 같아요! 그러면 현재 loginStatus 로직에서 추가로 예외 처리(탈퇴 회원 검증 등) 해줘야 할 것 같은데 이 부분은 따로 이슈파서 제가 할까요 ?

@Nullable
@Override
public Object resolveArgument(@NotNull MethodParameter parameter,
ModelAndViewContainer mavContainer,
@NotNull NativeWebRequest webRequest,
WebDataBinderFactory binderFactory) throws Exception
{
Auth auth = parameter.getMethodAnnotation(Auth.class);
if (auth == null)
throw new BaseException(INTERNAL_SERVER_ERROR);
String accessToken = webRequest.getHeader(Objects.requireNonNull(env.getProperty("jwt.auth-header")));
if(accessToken == null || !tokenUtils.isValidToken(tokenUtils.parseJustTokenFromFullToken(accessToken)))
return LoginStatus.getNotLoginStatus();
Long userIdx = Long.valueOf(tokenUtils.getUserIdFromFullToken(accessToken));
if (!auth.optional() && userIdx == null) {
return LoginStatus.getNotLoginStatus();
}
return LoginStatus.builder().isLogin(true).userIdx(userIdx).build();
}

오오 네 좋아요!! 부탁드립니다🩷

확인 완 !

@sojungpp sojungpp merged commit 98191d8 into develop Jul 12, 2024
1 check passed
@sojungpp sojungpp deleted the refactor/#327-fridge-exception branch July 12, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📝 docs 문서 변경 ♻️ refactor 리팩토링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[refactor] fridge 관련 exception 및 assembler 수정
2 participants