Skip to content

Commit 978e498

Browse files
authored
[Bugfix] Detect more schema type reference cycles. (#335)
[Bugfix] Detect more schema type reference cycles. ### Motivation The newly introduced recursive type support had a slight optimization to minimize the number of boxed types, but it contained a bug that only surfaced on larger real-world documents. It's not really necessary, so removed it and the number of boxed types is still reasonable, plus it doesn't lead to non-compiling code anymore. ### Modifications Removed an optimization that tried to further reduce the number of boxed types, because it actually missed some cycles and produced non-compiling code for some larger OpenAPI documents. ### Result Code now compiles even for more complex cycles. ### Test Plan Added more unit tests, plus tested on some real-world docs with cycles that failed before this change. Reviewed by: glbrntt Builds: ✔︎ pull request validation (5.10) - Build finished. ✔︎ pull request validation (5.8) - Build finished. ✔︎ pull request validation (5.9) - Build finished. ✔︎ pull request validation (compatibility test) - Build finished. ✔︎ pull request validation (docc test) - Build finished. ✔︎ pull request validation (integration test) - Build finished. ✔︎ pull request validation (nightly) - Build finished. ✔︎ pull request validation (soundness) - Build finished. #335
1 parent c67892a commit 978e498

File tree

3 files changed

+64
-32
lines changed

3 files changed

+64
-32
lines changed

Sources/_OpenAPIGeneratorCore/Translator/Recursion/RecursionDetector.swift

Lines changed: 22 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -85,12 +85,7 @@ struct RecursionDetector {
8585
// document.
8686
// - Walk all references and keep track of names already visited.
8787
// - If visiting a schema that is already in the stack, we found a cycle.
88-
// - In the cycle, first identify the set of types involved in it, and
89-
// check if any of the types is already recorded as a recursive type.
90-
// If so, no action needed and terminate this branch and continue with
91-
// the next one.
92-
// - If no type in the cycle is already included in the set of recursive
93-
// types, find the first boxable type starting from the current one
88+
// - Find the first boxable type starting from the current one
9489
// ("causing" the recursion) following the cycle, and add it to this
9590
// set, and then terminate this branch and continue.
9691
// - At the end, return the set of recursive types.
@@ -102,21 +97,23 @@ struct RecursionDetector {
10297

10398
func visit(_ node: Node) throws {
10499
let name = node.name
100+
let previousStackSet = stackSet
101+
102+
// Add to the stack.
103+
stack.append(node)
104+
stackSet.insert(name)
105+
defer {
106+
stackSet.remove(name)
107+
stack.removeLast()
108+
}
105109

106110
// Check if we've seen this node yet.
107111
if !seen.contains(name) {
108112

109-
// Add to the stack.
110-
stack.append(node)
111-
stackSet.insert(name)
112-
defer {
113-
stackSet.remove(name)
114-
stack.removeLast()
115-
}
116-
117113
// Not seen this node yet, so add it to seen, and then
118114
// visit its edges.
119115
seen.insert(name)
116+
120117
for edge in node.edges {
121118
try visit(container.lookup(edge))
122119
}
@@ -125,42 +122,38 @@ struct RecursionDetector {
125122

126123
// We have seen this node.
127124

128-
// If the name is not in the stack, this is not a cycle.
129-
if !stackSet.contains(name) {
125+
// If the name is not in the stack twice, this is not a cycle.
126+
if !previousStackSet.contains(name) {
130127
return
131128
}
132129

133-
// It is in the stack, so we just closed a cycle.
130+
// It is in the stack twice, so we just closed a cycle.
134131

135132
// Identify the names involved in the cycle.
136133
// Right now, the stack must have the current node there twice.
137134
// Ignore everything before the first occurrence.
138-
139135
let cycleNodes = stack.drop(while: { $0.name != name })
140-
let cycleNames = Set(cycleNodes.map(\.name))
141-
142-
// Check if any of the names are already boxed.
143-
if cycleNames.contains(where: { boxed.contains($0) }) {
144-
// Found one, so we know this cycle will already be broken.
145-
// No need to add any other type, just return from this
146-
// visit.
147-
return
148-
}
149136

150137
// We now choose which node will be marked as recursive.
151138
// Only consider boxable nodes, trying from the start of the cycle.
152139
guard let firstBoxable = cycleNodes.first(where: \.isBoxable) else {
153140
throw RecursionError.invalidRecursion(name.description)
154141
}
155142

143+
let nameToAdd = firstBoxable.name
144+
145+
// Check if we're already going to box this type, if so, we're done.
146+
if boxed.contains(nameToAdd) {
147+
return
148+
}
149+
156150
// None of the types are boxed yet, so add the current node.
157-
boxed.insert(firstBoxable.name)
151+
boxed.insert(nameToAdd)
158152
}
159153

160154
for node in rootNodes {
161155
try visit(node)
162156
}
163-
164157
return boxed
165158
}
166159
}

Sources/swift-openapi-generator/Documentation.docc/Development/Supporting-recursive-types.md

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,9 +118,7 @@ The algorithm outputs a list of type names that require boxing.
118118

119119
It iterates over the types defined in `#/components/schemas`, in the order defined in the OpenAPI document, and for each type walks all of its references.
120120

121-
Once it detects a reference cycle, it checks whether any of the types involved in the current cycle are already in the list, and if so, considers this cycle to already be addressed.
122-
123-
If no type in the current cycle is found in the list, it adds the first type in the cycle, in other words the one to which the last reference closed the cycle.
121+
Once it detects a reference cycle, it adds the first type in the cycle, in other words the one to which the last reference closed the cycle.
124122

125123
For example, walking the following:
126124
```

Tests/OpenAPIGeneratorCoreTests/Translator/TypeAssignment/Test_RecursionDetector_Generic.swift

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,28 @@ class Test_RecursionDetector_Generic: Test_Core {
214214
)
215215
}
216216

217+
func testMultipleCycles3() throws {
218+
try _test(
219+
rootNodes: [
220+
"A",
221+
"B",
222+
"C",
223+
"D",
224+
],
225+
putIntoContainer: [
226+
"A -> C",
227+
"B -> D,A",
228+
"C -> B,D",
229+
"D -> B,C",
230+
],
231+
expected: [
232+
"A",
233+
"B",
234+
"C",
235+
]
236+
)
237+
}
238+
217239
func testNested() throws {
218240
try _test(
219241
rootNodes: [
@@ -273,6 +295,25 @@ class Test_RecursionDetector_Generic: Test_Core {
273295
)
274296
}
275297

298+
func testDifferentCyclesForSameNode() throws {
299+
try _test(
300+
rootNodes: [
301+
"C",
302+
"A",
303+
"B",
304+
],
305+
putIntoContainer: [
306+
"A -> B",
307+
"B -> C,A",
308+
"C -> A",
309+
],
310+
expected: [
311+
"C",
312+
"A",
313+
]
314+
)
315+
}
316+
276317
// MARK: - Private
277318

278319
private func _test(

0 commit comments

Comments
 (0)