refactor: 입력 형식과 관련된 검증은 Dto 내부에서 하게 한다. #187
Conversation
| } | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
isValidChoice로 분리하신 점이 좋네요👍 addPropertyNode로 문제 필드 알려주는 것도 좋을 것 같습니다.
- 선택 중 중복 대학을 검증하는 메서드
- n번째 없이 n+1 요소만 선택된 경우를 검증하는 메서드
처럼 메서드를 나누어 검증하는 것도 좋을 것 같네요.
There was a problem hiding this comment.
확인하면서 1번째 대학 없이 2번째 대학이 있는 경우는 없나 생각했는데, 1번은
@NotNull(message = "1지망 대학교를 입력해주세요.")
로 검증되고 있었네요.
첫번째 대학도 필드 레벨 검증이 아닌 ValidUniversityChoiceValidator에서 같이 검증하도록 하는 것에 대해서는 어떻게 생각하시나요?
There was a problem hiding this comment.
제 생각에 addPropertyNode 를 사용하는건 구현이 복잡해질 것 같고, 지금 메세지만으로도 충분히 디버깅할 수 있다 생각해요.
이 부분은 규혁님이 판단하셔서 선택해주세요~
There was a problem hiding this comment.
첫번째 대학도 필드 레벨 검증이 아닌 ValidUniversityChoiceValidator에서 같이 검증하도록 하는 것에 대해서는 어떻게 생각하시나요?
저도 같은 생각입니다😊
"유효한 대학 선택인지"에 대한 검증이 한 군데에 모여있어야 더 응집도 있을 것 같습니다.
There was a problem hiding this comment.
그렇네요 기존코드를 유지한 채 한다는 생각을 자꾸 가지다보니 오히려 더 헷갈리게 만들었네요.. 😂 반영했습니다!
| public static final String ERROR_THIRD_CHOICE_WITHOUT_SECOND = "2지망 없이 3지망을 선택할 수 없습니다."; | ||
| public static final String ERROR_DUPLICATE_CHOICE = "지망 선택이 중복되었습니다"; |
There was a problem hiding this comment.
public 으로 하신건 테스트 코드에서 받아오기 위해서인가요?
// dto constraint
ERROR_THIRD_CHOICE_WITHOUT_SECOND
ERROR_DUPLICATE_CHOICE
ErrorCode enum 에서 위와 같이 선언하지 않은 이유가 따로 있는지 궁금해요~
There was a problem hiding this comment.
앗 그게 훨씬 좋네요..🥲 테스트코드에서 쓰려고 한 게 맞는데 ErrorCode로 바로 옮겼습니다!
| } | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
제 생각에 addPropertyNode 를 사용하는건 구현이 복잡해질 것 같고, 지금 메세지만으로도 충분히 디버깅할 수 있다 생각해요.
이 부분은 규혁님이 판단하셔서 선택해주세요~
| } | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
첫번째 대학도 필드 레벨 검증이 아닌 ValidUniversityChoiceValidator에서 같이 검증하도록 하는 것에 대해서는 어떻게 생각하시나요?
저도 같은 생각입니다😊
"유효한 대학 선택인지"에 대한 검증이 한 군데에 모여있어야 더 응집도 있을 것 같습니다.
| if (request.thirdChoiceUniversityId() != null && request.secondChoiceUniversityId() == null) { | ||
| context.disableDefaultConstraintViolation(); |
There was a problem hiding this comment.
검증 실패 시 발생하는 메세지에 기본 메세지
(지금은 ValidUniversityChoice의 String message() default "2지망 없이 3지망을 선택할 수 없습니다")
를 항상 포함하지 않을거라면, 기본 메세지의 의미가 없을 것 같아요🫨
차라리 기본 메세지를 없애던가,
기본 메세지를 "유효하지 않은 지망 대학 선택입니다"로 하고
디폴트 메세지와 함께 구체적인 메세지를 주게 하는건 어떤가요?
| private boolean isValidChoice(Long choiceId, Set<Long> uniqueIds, ConstraintValidatorContext context) { | ||
| if (choiceId == null) | ||
| return true; | ||
|
|
||
| if (!uniqueIds.add(choiceId)) { | ||
| context.disableDefaultConstraintViolation(); | ||
| context.buildConstraintViolationWithTemplate(ERROR_DUPLICATE_CHOICE) | ||
| .addConstraintViolation(); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
함수 이름이 더 의미를 드러냈으면 좋겠어요!
e.g. isNotUnique or isDuplicate
그리고 어느 필드에서 중복이 발생했는지를 찾는 목적이 아니라면, 이 함수 하나로 충분하지 않을까요?
private boolean isDuplicate(UniversityChoiceRequest request) {
return Stream.of(request.firstChoiceUniversityId(), request.secondChoiceUniversityId(),
request.thirdChoiceUniversityId()).filter(Objects::nonNull).count() < 3;
}There was a problem hiding this comment.
이건 선택한 대학 ID(1지망, 2지망, 3지망) 중 null이 아닌 값이 3개 미만인지 확인하는 함수 같은데 isDuplicate랑 관련이 없지 않나요?
There was a problem hiding this comment.
제가 구현한 함수명이 조금 불분명한 거 같긴 하네요 😅
관련 이슈
작업 내용
기존 서비스 로직에 있던 대학 중복 선택 검증을 지우고 ConstraintValidator를 활용하여 DTO에서 검증하도록 변경하였습니다.
추가로 기존에 검증하지 않던 2지망 없이 1, 3지망만 선택하는 case의 예외처리도 ConstraintValidator에 추가하였습니다.
특이 사항
기존 코드가 잘 동작하는지 확인하고 있었는데 UniversityChoiceRequest의 1지망 @NotNull 어노테이션이 아예 동작하지 않는 것을 발견했었습니다. 왜그런지 구글링을 해보니 중첩 DTO일 때 저런 검증 어노테이션이 동작하려면
이런식으로 첫 DTO에 @Valid를 붙여야하는 것을 확인했습니다. 그동안 @Valid와 @validated의 차이를 몰랐는데 이참에 알게되었네요. 이건 @Valid로만 적용가능하다고 합니다.
중첩 DTO 관련 참고 자료
리뷰 요구사항 (선택)
이 커스텀Validator 테스트 코드 작성방법을 몰라서 구글링을 해봤는데 자료가 잘 나오질 않네요. 혹시 다른 더 좋은 방안이 있다면 참고하겠습니다.
저는 커스텀Validator 테스트 코드 관련 참고 자료 여기서 참고했습니다. 여기 제시 방안은 3가지가 있었는데 ValidatorFactory 사용하는 게 실제 환경과 제일 비슷하게 검증하는 것 같아 ValidatorFactory를 사용해서 테스트코드 작성하였습니다.