Fix the issue Jakarta validation retries #1253
Conversation
- 문자열 제약이 걸린 값에 대해 더 이상 불필요한 재시도를 하지 않고 즉시 실패하도록 수정했습니다. - Jakarta/Javax 플러그인이 새 문자열 제약 정보를 올바르게 넘기도록 보강했습니다. - JakartaValidationIssue1231Test에 컨테이너, 필드 제약 회귀 테스트를 추가했습니다. - AnnotationTest/ValidationSpecs에 @SiZe 문자열 스펙을 넣어 기본 제약이 깨지지 않는지 확인했습니다.
|
|
github-actions
bot
requested review from
SooKim1110,
acktsap,
benelog,
jwChung,
mhyeon-lee and
seongahjo
| throw new ValidationFailedException( | ||
| "Given value is null but annotated with @NotNull.", | ||
| Collections.singleton(violationProperty) | ||
| ); |
There was a problem hiding this comment.
여기에서 직접적으로 ValidationFailedException를 throw하는 것보다, ValidationFailedException를 throw하는 책임은 ArbitraryValidator에게 위임하는 게 어떨까 싶습니다.
There was a problem hiding this comment.
저도 ValidateArbitraryGenerator 안에서 직접 예외를 던지는 횟수가 많아지니 책임을 분리할 필요성을 느꼈습니다.
현재 ArbitraryValidator가 validate(Object arbitrary)만 제공하다보니, 여기로 책임을 넘기면 필터 단계에서 재시도 제어를 할 방법이 없어지고 결국 무한루프를 빠져나오지 못하게 됩니다.
말씀해주신 것처럼 장기적으로는 제약 위반 정보를 담은 별도의 인터페이스를 통해 책임을 명확히 분리하는 편이 좋다고 생각합니다.
현재 변경된 코드 흐름처럼 새 인터페이스를 도입해 책임을 위임할 준비가 된다면, 그때 Validator가 필요한 메타데이터를 받아 적절한 예외를 던질 수 있도록 구조를 다듬으려고 합니다. 괜찮으시다면 책임을 분리하는 추가 커밋도 후속으로 올려도 괜찮을까요?
There was a problem hiding this comment.
ArbitraryValidator 에 책임을 직접 넘기는 것보다는 중간 계층인 FilteredCombinableArbitrary 에서 처리하는 방향을 말씀드린 거긴 한데요, 혹시 이 방향으로 고민해보셨을까요??
재귀적인 구조라 조금 더 생각할 게 많긴 한데, 바로 예외를 던지는 방향보다 FilteredCombinableArbitrary 에서 제어하는 방향으로 구현되면 좋을 것 같습니다.
| if (generated.fixed()) { | ||
| String string = (String)generated.rawValue(); | ||
| enforceMandatoryStringConstraints(javaStringConstraint, string, violationProperty); | ||
| validateStringLengthOrThrow(javaStringConstraint, string, violationProperty); |
There was a problem hiding this comment.
fixed일 때 rawValue()가 항상 같은 값을 반환하기는 하지만, 값을 미리 생성하는 게 기존 구조와 일관성을 깨는 것 같습니다.
fixed일 때의 예외처리는 FilteredCombinableArbitrary 에서 일관되게 처리하는 방향이 더 좋을 것 같습니다.
기존 흐름대로 갔을 때 문제가 있어서 예외처리를 하신 걸까요?
There was a problem hiding this comment.
이전 구조에선 ValidateArbitraryGenerator가 문자열 제약을 filter 안에서 boolean으로만 처리했습니다. 그래서 사용자가setNull("notNullElement[0]") 같이 값을 고정하면, predicate는 계속 false만 돌려주고, FilteredCombinableArbitrary는 재시도를 반복했습니다. 문제는 어느 속성이 어느 어노테이션을 어겼는지 정보가 사라지고, 1번 반복하는데 약 15초가량이 걸린다는 점입니다. fixed로 인해 데이터는 변경하지 않는데 1000번을 반복하니 무한루프처럼 보이게 된 것이었습니다.
그래서 지금은 fixed일 때만큼은 rawValue()를 한 번 꺼내 즉시 ValidationFailedException을 던지도록 바꿨습니다. 그러면 필터 루프가 재시도하지 않고 바로 빠져나오고, non-fixed 케이스는 여전히 필터에서 false를 반환해 재생성을 유도하도록 기존 재시도 흐름도 유지할 수 있게 하였습니다.
There was a problem hiding this comment.
위에 코멘트 드린 내용을 고민하다보면 불필요한 생성 반복 문제도 해결될 것 같습니다.
- add FilteringArbitraryValidator to capture last validation failure and expose validateSafely/consumeFailure - extend CombinableArbitrary.filter and FilteredCombinableArbitrary so validators propagate through nested filters and throw failures consistently - update ResolvedCombinableArbitrary to wrap the delegate validator, reuse it for every filter call, and stop throwing inside the predicate
- add a unit test for FilteringArbitraryValidator to ensure validateSafely/consumeFailure capture and clear failures as expected - add FilteredCombinableArbitraryTest to confirm validator failures propagate through the filter chain as RetryableFilterMissException
|
@seongahjo 코멘트 주신 내용을 바탕으로 코드를 수정했습니다!
즉,
|
3 participants
Summary
ISSUE LINK
JavaStringConstraint로@NotNull,@NotBlank,@NotEmpty,@Size제약을 강제하고,ValidationFailedException으로 구체적인 위반 정보를 전달합니다.(Optional): Description
FilteredCombinableArbitrary에서ValidationFailedException이 발생하면 루프를 중단하도록 수정했습니다.JavaStringConstraint에notEmpty접근자를 추가하고,ValidateArbitraryGenerator를 리팩터링해 문자열 제약 검사를 처리하고ValidationFailedException을 발생시킵니다.JakartaValidationIssue1231Test.class파일은 해당 이슈를 재현하기 위해 생성한 테스트 파일입니다. Merge 준비가 되면 삭제하도록 하겠습니다.How Has This Been Tested?
sampleSizeAnnotationGeneratesValidString()를 추가했습니다.Is the Document updated?