Skip to content

Commit 288676b

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #428 from skaslev/ref-chain
🐛 Fix flattening chains of references to references
2 parents 820a4a2 + 5c0c6ae commit 288676b

File tree

4 files changed

+117
-7
lines changed

4 files changed

+117
-7
lines changed

‎pkg/crd/flatten_type_test.go

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@ var _ = Describe("General Schema Flattening", func() {
3838

3939
rootType = crd.TypeIdent{Name: "RootType", Package: rootPkg}
4040
subtypeWithRefs = crd.TypeIdent{Name: "SubtypeWithRefs", Package: rootPkg}
41+
leafAliasType = crd.TypeIdent{Name: "LeafAlias", Package: rootPkg}
4142
leafType = crd.TypeIdent{Name: "LeafType", Package: otherPkg}
43+
inPkgLeafType = crd.TypeIdent{Name: "InPkgLeafType", Package: rootPkg}
4244
)
4345

4446
BeforeEach(func() {
@@ -72,6 +74,76 @@ var _ = Describe("General Schema Flattening", func() {
7274
}
7375
})
7476

77+
Context("when dealing with reference chains", func() {
78+
It("should flatten them", func() {
79+
By("setting up a RootType, LeafAlias --> Alias --> Int")
80+
toLeafAlias := crd.TypeRefLink("", leafAliasType.Name)
81+
toLeaf := crd.TypeRefLink("other", leafType.Name)
82+
fl.Parser.Schemata = map[crd.TypeIdent]apiext.JSONSchemaProps{
83+
rootType: apiext.JSONSchemaProps{
84+
Properties: map[string]apiext.JSONSchemaProps{
85+
"refProp": apiext.JSONSchemaProps{Ref: &toLeafAlias},
86+
},
87+
},
88+
leafAliasType: apiext.JSONSchemaProps{Ref: &toLeaf},
89+
leafType: apiext.JSONSchemaProps{
90+
Type: "string",
91+
Pattern: "^[abc]$",
92+
},
93+
}
94+
95+
By("flattening the type hierarchy")
96+
// flattenAllOf to avoid the normalize the all-of forms to what we
97+
// really want (instead of caring about nested all-ofs)
98+
outSchema := crd.FlattenEmbedded(fl.FlattenType(rootType), rootPkg)
99+
Expect(rootPkg.Errors).To(HaveLen(0))
100+
Expect(otherPkg.Errors).To(HaveLen(0))
101+
102+
By("verifying that it was flattened to have no references")
103+
Expect(outSchema).To(Equal(&apiext.JSONSchemaProps{
104+
Properties: map[string]apiext.JSONSchemaProps{
105+
"refProp": apiext.JSONSchemaProps{
106+
Type: "string", Pattern: "^[abc]$",
107+
},
108+
},
109+
}))
110+
})
111+
112+
It("should not infinite-loop on circular references", func() {
113+
By("setting up a RootType, LeafAlias --> Alias --> LeafAlias")
114+
toLeafAlias := crd.TypeRefLink("", leafAliasType.Name)
115+
toLeaf := crd.TypeRefLink("", inPkgLeafType.Name)
116+
fl.Parser.Schemata = map[crd.TypeIdent]apiext.JSONSchemaProps{
117+
rootType: apiext.JSONSchemaProps{
118+
Properties: map[string]apiext.JSONSchemaProps{
119+
"refProp": apiext.JSONSchemaProps{Ref: &toLeafAlias},
120+
},
121+
},
122+
leafAliasType: apiext.JSONSchemaProps{Ref: &toLeaf},
123+
inPkgLeafType: apiext.JSONSchemaProps{Ref: &toLeafAlias},
124+
}
125+
126+
By("flattening the type hierarchy")
127+
// flattenAllOf to avoid the normalize the all-of forms to what we
128+
// really want (instead of caring about nested all-ofs)
129+
outSchema := crd.FlattenEmbedded(fl.FlattenType(rootType), rootPkg)
130+
131+
// This should *finish* to some degree, leaving the circular reference in
132+
// place. It should be fine to error on circular references in the future, though.
133+
Expect(rootPkg.Errors).To(HaveLen(0))
134+
Expect(otherPkg.Errors).To(HaveLen(0))
135+
136+
By("verifying that it was flattened to *something*")
137+
Expect(outSchema).To(Equal(&apiext.JSONSchemaProps{
138+
Properties: map[string]apiext.JSONSchemaProps{
139+
"refProp": apiext.JSONSchemaProps{
140+
Ref: &toLeafAlias,
141+
},
142+
},
143+
}))
144+
})
145+
})
146+
75147
It("should flatten a hierarchy of references", func() {
76148
By("setting up a series of types RootType --> SubtypeWithRef --> LeafType")
77149
toSubtype := crd.TypeRefLink("", subtypeWithRefs.Name)

‎pkg/crd/schema_visitor.go

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,16 +44,37 @@ type schemaWalker struct {
4444
visitor SchemaVisitor
4545
}
4646

47-
// walkSchema walks the given schema, saving modifications made by the
48-
// visitor (this is as simple as passing a pointer in most cases,
49-
// but special care needs to be taken to persist with maps).
47+
// walkSchema walks the given schema, saving modifications made by the visitor
48+
// (this is as simple as passing a pointer in most cases, but special care
49+
// needs to be taken to persist with maps). It also visits referenced
50+
// schemata, dealing with circular references appropriately. The returned
51+
// visitor will be used to visit all "children" of the current schema, followed
52+
// by a nil schema with the returned visitor to mark completion. If a nil visitor
53+
// is returned, traversal will no continue into the children of the current schema.
5054
func (w schemaWalker) walkSchema(schema *apiext.JSONSchemaProps) {
51-
subVisitor := w.visitor.Visit(schema)
52-
if subVisitor == nil {
53-
return
55+
// Walk a potential chain of schema references, keeping track of seen
56+
// references to avoid circular references
57+
subVisitor := w.visitor
58+
seenRefs := map[string]bool{}
59+
if schema.Ref != nil {
60+
seenRefs[*schema.Ref] = true
61+
}
62+
for {
63+
subVisitor = subVisitor.Visit(schema)
64+
if subVisitor == nil {
65+
return
66+
}
67+
// mark completion of the visitor
68+
defer subVisitor.Visit(nil)
69+
70+
// Break if schema is not a reference or a cycle is detected
71+
if schema.Ref == nil || len(*schema.Ref) == 0 || seenRefs[*schema.Ref] {
72+
break
73+
}
74+
seenRefs[*schema.Ref] = true
5475
}
55-
defer subVisitor.Visit(nil)
5676

77+
// walk sub-schemata
5778
subWalker := schemaWalker{visitor: subVisitor}
5879
if schema.Items != nil {
5980
subWalker.walkPtr(schema.Items.Schema)

‎pkg/crd/testdata/cronjob_types.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,13 +145,19 @@ type CronJobSpec struct {
145145
// A struct that can only be entirely replaced
146146
// +structType=atomic
147147
StructWithSeveralFields NestedObject `json:"structWithSeveralFields"`
148+
149+
// This tests that type references are properly flattened
150+
// +kubebuilder:validation:optional
151+
JustNestedObject *JustNestedObject `json:"justNestedObject,omitempty"`
148152
}
149153

150154
type NestedObject struct {
151155
Foo string `json:"foo"`
152156
Bar bool `json:"bar"`
153157
}
154158

159+
type JustNestedObject NestedObject
160+
155161
type RootObject struct {
156162
Nested NestedObject `json:"nested"`
157163
}

‎pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5022,6 +5022,17 @@ spec:
50225022
- template
50235023
type: object
50245024
type: object
5025+
justNestedObject:
5026+
description: This tests that type references are properly flattened
5027+
properties:
5028+
bar:
5029+
type: boolean
5030+
foo:
5031+
type: string
5032+
required:
5033+
- bar
5034+
- foo
5035+
type: object
50255036
mapOfInfo:
50265037
additionalProperties:
50275038
format: byte

0 commit comments

Comments
 (0)