Skip to content

Commit dec1677

Browse files
rationulladwsingh
authored andcommitted
Fix cross-branch pollution bug in SchemaConverter recursion detection
SchemaConverter would incorrectly identify member schemas as recursive when the shape graph contains multiple branches hitting the same target shape, downstream of the member shape. This change "pops" the "top of stack" shape on the way out of isRecursive(), avoiding this cross-branch pollution.
1 parent fe2c41f commit dec1677

File tree

2 files changed

+70
-1
lines changed

2 files changed

+70
-1
lines changed

dynamic-schemas/src/main/java/software/amazon/smithy/java/dynamicschemas/SchemaConverter.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ private boolean isRecursive(Set<ShapeId> visited, Shape shape) {
101101
return true;
102102
}
103103

104-
return switch (shape.getType().getCategory()) {
104+
var result = switch (shape.getType().getCategory()) {
105105
case SIMPLE, SERVICE -> false;
106106
case MEMBER -> isRecursive(visited, model.expectShape(shape.asMemberShape().orElseThrow().getTarget()));
107107
case AGGREGATE -> {
@@ -113,6 +113,10 @@ private boolean isRecursive(Set<ShapeId> visited, Shape shape) {
113113
yield false;
114114
}
115115
};
116+
117+
// "Pop" the current node off the "stack" -- other branches are still allowed to contain this shape.
118+
visited.remove(shape.getId());
119+
return result;
116120
}
117121

118122
private Schema createNonRecursiveSchema(Shape shape) {

dynamic-schemas/src/test/java/software/amazon/smithy/java/dynamicschemas/SchemaConverterTest.java

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,15 @@ enum MyEnum {
6868
foo: String
6969
}
7070
71+
structure NestedStruct {
72+
foo: SimpleStruct
73+
bar: SimpleStruct
74+
}
75+
76+
structure ParentStruct {
77+
foo: NestedStruct
78+
}
79+
7180
union SimpleUnion {
7281
foo: String
7382
}
@@ -134,9 +143,65 @@ static List<Arguments> convertsAggregateSchemasSource() {
134143
Arguments.of(ShapeType.LIST, "SimpleList"),
135144
Arguments.of(ShapeType.MAP, "SimpleMap"),
136145
Arguments.of(ShapeType.STRUCTURE, "SimpleStruct"),
146+
Arguments.of(ShapeType.STRUCTURE, "NestedStruct"),
147+
Arguments.of(ShapeType.STRUCTURE, "ParentStruct"),
137148
Arguments.of(ShapeType.UNION, "SimpleUnion"),
138149
Arguments.of(ShapeType.LIST, "RecursiveList"),
139150
Arguments.of(ShapeType.MAP, "RecursiveMap"),
140151
Arguments.of(ShapeType.STRUCTURE, "RecursiveStructure"));
141152
}
153+
154+
@MethodSource("detectsRecursiveSchemasSource")
155+
@ParameterizedTest
156+
public void detectsRecursiveSchemas(String name) {
157+
var converter = new SchemaConverter(model);
158+
var schema = converter.getSchema(model.expectShape(ShapeId.from("smithy.example#" + name)));
159+
160+
// Detecting that SchemaConverter handled this schema as recursive by checking classname is
161+
// hacky, but public API doesn't expose a cleaner way, and probably shouldn't.
162+
assertThat(schema.getClass().getSimpleName(), is("DeferredRootSchema"));
163+
}
164+
165+
static List<Arguments> detectsRecursiveSchemasSource() {
166+
return List.of(
167+
Arguments.of("RecursiveList"),
168+
Arguments.of("RecursiveMap"),
169+
Arguments.of("RecursiveStructure"));
170+
}
171+
172+
@MethodSource("detectsNonRecursiveSchemasSource")
173+
@ParameterizedTest
174+
public void detectsNonRecursiveSchemas(String name) {
175+
var converter = new SchemaConverter(model);
176+
var schema = converter.getSchema(model.expectShape(ShapeId.from("smithy.example#" + name)));
177+
178+
// Detecting that SchemaConverter handled this schema as non-recursive by checking classname is
179+
// hacky, but public API doesn't expose a cleaner way, and probably shouldn't.
180+
assertThat(schema.getClass().getSimpleName(), not("DeferredRootSchema"));
181+
}
182+
183+
static List<Arguments> detectsNonRecursiveSchemasSource() {
184+
return List.of(
185+
Arguments.of("MyDocument"),
186+
Arguments.of("MyString"),
187+
Arguments.of("MyBoolean"),
188+
Arguments.of("MyTimestamp"),
189+
Arguments.of("MyBlob"),
190+
Arguments.of("MyByte"),
191+
Arguments.of("MyShort"),
192+
Arguments.of("MyInteger"),
193+
Arguments.of("MyLong"),
194+
Arguments.of("MyFloat"),
195+
Arguments.of("MyDouble"),
196+
Arguments.of("MyBigInteger"),
197+
Arguments.of("MyBigDecimal"),
198+
Arguments.of("MyEnum"),
199+
Arguments.of("MyIntEnum"),
200+
Arguments.of("SimpleList"),
201+
Arguments.of("SimpleMap"),
202+
Arguments.of("SimpleStruct"),
203+
Arguments.of("NestedStruct"),
204+
Arguments.of("ParentStruct"),
205+
Arguments.of("SimpleUnion"));
206+
}
142207
}

0 commit comments

Comments
 (0)