Skip to content

Commit a1bc0c0

Browse files
thcedclaude
andcommitted
fix: Address code review for SchemaParser composition
Reordered `parseBaseIfPresent` so that `primary` (the dominant non-null TypeName) is only computed after the two implicit-shape early-return branches (object/array keyword detection), ensuring it is never evaluated unnecessarily. Pinned the empty-schema → permissive ObjectSchema behaviour with a new `parsesEmptySchemaAsPermissiveObject` test, which documents the intentional deviation from the old NullSchema return. Added a `parsesEmptyAllOfAsPermissiveObject` regression test to confirm that `allOf: []` (zero assertions) falls through to the permissive-object fallback. Strengthened `allOfBranchesFlattenIntoOuterAllOf` with per-part instanceof assertions and concrete minLength/maxLength constraint checks on the flattened StringSchema parts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 5e9b32c commit a1bc0c0

2 files changed

Lines changed: 28 additions & 4 deletions

File tree

src/main/java/com/retailsvc/http/spec/schema/SchemaParser.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,17 +58,15 @@ private static Schema parseBaseIfPresent(Map<String, Object> raw) {
5858
if (types.isEmpty() && !hasObjectShapeKeywords(raw) && !hasArrayShapeKeywords(raw)) {
5959
return null;
6060
}
61-
62-
TypeName primary =
63-
types.stream().filter(t -> t != TypeName.NULL).findFirst().orElse(TypeName.NULL);
64-
6561
if (types.isEmpty() && hasObjectShapeKeywords(raw)) {
6662
return parseObject(raw, types);
6763
}
6864
if (types.isEmpty() && hasArrayShapeKeywords(raw)) {
6965
return parseArray(raw, types);
7066
}
7167

68+
TypeName primary =
69+
types.stream().filter(t -> t != TypeName.NULL).findFirst().orElse(TypeName.NULL);
7270
return switch (primary) {
7371
case STRING -> parseString(raw, types);
7472
case INTEGER -> parseInteger(raw, types);

src/test/java/com/retailsvc/http/spec/schema/SchemaParserTest.java

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,27 @@ void multipleCombinatorsInOneSchemaWrapInAllOf() {
235235
assertThat(all.parts().get(1)).isInstanceOf(NotSchema.class);
236236
}
237237

238+
@Test
239+
void parsesEmptySchemaAsPermissiveObject() {
240+
// {} accepts anything that's an object; this is the JSON Schema 3.1 default
241+
// and a behaviour change from the previous parser, which returned NullSchema.
242+
Schema s = SchemaParser.parse(Map.of());
243+
assertThat(s).isInstanceOf(ObjectSchema.class);
244+
ObjectSchema obj = (ObjectSchema) s;
245+
assertThat(obj.types()).isEmpty();
246+
assertThat(obj.properties()).isEmpty();
247+
assertThat(obj.required()).isEmpty();
248+
assertThat(obj.additionalProperties()).isInstanceOf(AdditionalProperties.Allowed.class);
249+
}
250+
251+
@Test
252+
void parsesEmptyAllOfAsPermissiveObject() {
253+
// allOf: [] contributes zero assertions; with no base assertion, the parser
254+
// falls back to permissive object.
255+
Schema s = SchemaParser.parse(Map.of("allOf", List.of()));
256+
assertThat(s).isInstanceOf(ObjectSchema.class);
257+
}
258+
238259
@Test
239260
void allOfBranchesFlattenIntoOuterAllOf() {
240261
Schema s =
@@ -250,6 +271,11 @@ void allOfBranchesFlattenIntoOuterAllOf() {
250271
AllOfSchema all = (AllOfSchema) s;
251272
// Base + the two allOf branches flattened.
252273
assertThat(all.parts()).hasSize(3);
274+
assertThat(all.parts().get(0)).isInstanceOf(StringSchema.class);
275+
assertThat(all.parts().get(1)).isInstanceOf(StringSchema.class);
276+
assertThat(all.parts().get(2)).isInstanceOf(StringSchema.class);
277+
assertThat(((StringSchema) all.parts().get(1)).minLength()).isEqualTo(1);
278+
assertThat(((StringSchema) all.parts().get(2)).maxLength()).isEqualTo(10);
253279
}
254280

255281
@Test

0 commit comments

Comments
 (0)