Skip to content

Commit 2fc24b5

Browse files
thcedclaude
andcommitted
fix: Address code review for combinator validation
- Corrected misleading comment in validateOneOf (was "count misses", now "branch did not match; continue") - Added empty-options regression tests for allOf, anyOf, and oneOf to pin JSON Schema vacuous-truth / immediate-failure behaviour - Added null-value tests for not(NullSchema) and anyOf-with-NullSchema to document combinator null-passthrough behaviour - Replaced verbose fully-qualified org.assertj.core.api.Assertions.assertThat calls with static import in DefaultValidatorDispatchTest Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 5f08f50 commit 2fc24b5

2 files changed

Lines changed: 48 additions & 5 deletions

File tree

src/main/java/com/retailsvc/http/validate/DefaultValidator.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ private void validateOneOf(Object value, List<Schema> options, String pointer) {
314314
validate(value, o, pointer);
315315
matched++;
316316
} catch (ValidationException ignored) {
317-
// count misses
317+
// branch did not match; continue
318318
}
319319
}
320320
if (matched != 1) {

src/test/java/com/retailsvc/http/validate/DefaultValidatorDispatchTest.java

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.retailsvc.http.validate;
22

3+
import static org.assertj.core.api.Assertions.assertThat;
34
import static org.assertj.core.api.Assertions.assertThatThrownBy;
45

56
import com.retailsvc.http.ValidationException;
@@ -96,8 +97,8 @@ void oneOfFailsWhenZeroBranchesMatch() {
9697
.satisfies(
9798
t -> {
9899
var err = ((ValidationException) t).error();
99-
org.assertj.core.api.Assertions.assertThat(err.keyword()).isEqualTo("oneOf");
100-
org.assertj.core.api.Assertions.assertThat(err.message()).contains("matched 0 of 2");
100+
assertThat(err.keyword()).isEqualTo("oneOf");
101+
assertThat(err.message()).contains("matched 0 of 2");
101102
});
102103
}
103104

@@ -110,8 +111,8 @@ void oneOfFailsWhenTwoBranchesMatch() {
110111
.satisfies(
111112
t -> {
112113
var err = ((ValidationException) t).error();
113-
org.assertj.core.api.Assertions.assertThat(err.keyword()).isEqualTo("oneOf");
114-
org.assertj.core.api.Assertions.assertThat(err.message()).contains("matched 2 of 2");
114+
assertThat(err.keyword()).isEqualTo("oneOf");
115+
assertThat(err.message()).contains("matched 2 of 2");
115116
});
116117
}
117118

@@ -129,4 +130,46 @@ void notFailsWhenInnerPasses() {
129130
.extracting(t -> ((ValidationException) t).error().keyword())
130131
.isEqualTo("not");
131132
}
133+
134+
@Test
135+
void allOfWithEmptyPartsAlwaysPasses() {
136+
// Empty allOf is vacuously true per JSON Schema 2020-12.
137+
v.validate("anything", new AllOfSchema(List.of()), "/v");
138+
}
139+
140+
@Test
141+
void anyOfWithEmptyOptionsAlwaysFails() {
142+
assertThatThrownBy(() -> v.validate("anything", new AnyOfSchema(List.of()), "/v"))
143+
.isInstanceOf(ValidationException.class)
144+
.extracting(t -> ((ValidationException) t).error().keyword())
145+
.isEqualTo("anyOf");
146+
}
147+
148+
@Test
149+
void oneOfWithEmptyOptionsAlwaysFails() {
150+
assertThatThrownBy(() -> v.validate("anything", new OneOfSchema(List.of()), "/v"))
151+
.isInstanceOf(ValidationException.class)
152+
.satisfies(
153+
t -> {
154+
var err = ((ValidationException) t).error();
155+
assertThat(err.keyword()).isEqualTo("oneOf");
156+
assertThat(err.message()).contains("matched 0 of 0");
157+
});
158+
}
159+
160+
@Test
161+
void notWithNullSchemaRejectsNull() {
162+
// not(NullSchema) — inner accepts null, outer must reject.
163+
assertThatThrownBy(() -> v.validate(null, new NotSchema(new NullSchema()), "/v"))
164+
.isInstanceOf(ValidationException.class)
165+
.extracting(t -> ((ValidationException) t).error().keyword())
166+
.isEqualTo("not");
167+
}
168+
169+
@Test
170+
void anyOfMatchesNullViaNullSchema() {
171+
// anyOf containing a NullSchema branch must pass for a null value.
172+
var schema = new AnyOfSchema(List.of(stringSchema(1, null), new NullSchema()));
173+
v.validate(null, schema, "/v");
174+
}
132175
}

0 commit comments

Comments
 (0)