From 92e4b51866f4f1355608d74a77469267f6fdcbae Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 25 Jun 2019 20:28:33 -0700 Subject: [PATCH] Unit test general CRD flattening This adds some unit tests for general (not AllOf) CRD flattening to complement the integration test. --- pkg/crd/flatten.go | 45 +++-- pkg/crd/flatten_type_test.go | 314 +++++++++++++++++++++++++++++++++++ pkg/crd/schema.go | 8 +- 3 files changed, 352 insertions(+), 15 deletions(-) create mode 100644 pkg/crd/flatten_type_test.go diff --git a/pkg/crd/flatten.go b/pkg/crd/flatten.go index 963ae40a8..a764a41fe 100644 --- a/pkg/crd/flatten.go +++ b/pkg/crd/flatten.go @@ -218,6 +218,8 @@ type Flattener struct { // Parser is used to lookup package and type details, and parse in new packages. Parser *Parser + LookupReference func(ref string, contextPkg *loader.Package) (TypeIdent, error) + // flattenedTypes hold the flattened version of each seen type for later reuse. flattenedTypes map[TypeIdent]apiext.JSONSchemaProps initOnce sync.Once @@ -226,6 +228,9 @@ type Flattener struct { func (f *Flattener) init() { f.initOnce.Do(func() { f.flattenedTypes = make(map[TypeIdent]apiext.JSONSchemaProps) + if f.LookupReference == nil { + f.LookupReference = identFromRef + } }) } @@ -275,29 +280,47 @@ func (f *Flattener) FlattenSchema(baseSchema apiext.JSONSchemaProps, currentPack return resSchema } -// identFromRef converts the given schema ref from the given package back -// into the TypeIdent that it represents. -func identFromRef(ref string, contextPkg *loader.Package) (TypeIdent, error) { +// RefParts splits a reference produced by the schema generator into its component +// type name and package name (if it's a cross-package reference). Note that +// referenced packages *must* be looked up relative to the current package. +func RefParts(ref string) (typ string, pkgName string, err error) { if !strings.HasPrefix(ref, defPrefix) { - return TypeIdent{}, fmt.Errorf("non-standard reference link %q", ref) + return "", "", fmt.Errorf("non-standard reference link %q", ref) } ref = ref[len(defPrefix):] // decode the json pointer encodings ref = strings.Replace(ref, "~1", "/", -1) ref = strings.Replace(ref, "~0", "~", -1) nameParts := strings.SplitN(ref, "~", 2) + if len(nameParts) == 1 { + // local reference + return nameParts[0], "", nil + } + // cross-package reference + return nameParts[1], nameParts[0], nil +} + +// identFromRef converts the given schema ref from the given package back +// into the TypeIdent that it represents. +func identFromRef(ref string, contextPkg *loader.Package) (TypeIdent, error) { + typ, pkgName, err := RefParts(ref) + if err != nil { + return TypeIdent{}, err + } + + if pkgName == "" { // a local reference return TypeIdent{ - Name: nameParts[0], + Name: typ, Package: contextPkg, }, nil } // an external reference return TypeIdent{ - Name: nameParts[1], - Package: contextPkg.Imports()[nameParts[0]], + Name: typ, + Package: contextPkg.Imports()[pkgName], }, nil } @@ -327,13 +350,13 @@ func preserveFields(dst *apiext.JSONSchemaProps, src apiext.JSONSchemaProps) { dst.Description = srcDesc } if srcTitle != "" { - src.Title = srcTitle + dst.Title = srcTitle } if srcExDoc != nil { - src.ExternalDocs = srcExDoc + dst.ExternalDocs = srcExDoc } if srcEx != nil { - src.Example = srcEx + dst.Example = srcEx } } @@ -363,7 +386,7 @@ func (f *flattenVisitor) Visit(baseSchema *apiext.JSONSchemaProps) SchemaVisitor // if we get a type that's just a ref, resolve it if baseSchema.Ref != nil && len(*baseSchema.Ref) > 0 { // resolve this ref - refIdent, err := identFromRef(*baseSchema.Ref, f.currentPackage) + refIdent, err := f.LookupReference(*baseSchema.Ref, f.currentPackage) if err != nil { f.currentPackage.AddError(err) return nil diff --git a/pkg/crd/flatten_type_test.go b/pkg/crd/flatten_type_test.go new file mode 100644 index 000000000..d1c749a19 --- /dev/null +++ b/pkg/crd/flatten_type_test.go @@ -0,0 +1,314 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package crd_test + +import ( + "fmt" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + + "golang.org/x/tools/go/packages" + "sigs.k8s.io/controller-tools/pkg/crd" + "sigs.k8s.io/controller-tools/pkg/loader" +) + +var _ = Describe("General Schema Flattening", func() { + var fl *crd.Flattener + + var ( + // just enough so we don't panic + rootPkg = &loader.Package{Package: &packages.Package{PkgPath: "root"}} + otherPkg = &loader.Package{Package: &packages.Package{PkgPath: "other"}} + + rootType = crd.TypeIdent{Name: "RootType", Package: rootPkg} + subtypeWithRefs = crd.TypeIdent{Name: "SubtypeWithRefs", Package: rootPkg} + leafType = crd.TypeIdent{Name: "LeafType", Package: otherPkg} + ) + + BeforeEach(func() { + fl = &crd.Flattener{ + Parser: &crd.Parser{ + Schemata: map[crd.TypeIdent]apiext.JSONSchemaProps{}, + PackageOverrides: map[string]crd.PackageOverride{ + "root": func(_ *crd.Parser, _ *loader.Package) {}, + "other": func(_ *crd.Parser, _ *loader.Package) {}, + }, + }, + LookupReference: func(ref string, contextPkg *loader.Package) (crd.TypeIdent, error) { + typ, pkgName, err := crd.RefParts(ref) + if err != nil { + return crd.TypeIdent{}, err + } + + // cheat and just treat these as global + switch pkgName { + case "": + return crd.TypeIdent{Name: typ, Package: contextPkg}, nil + case "root": + return crd.TypeIdent{Name: typ, Package: rootPkg}, nil + case "other": + return crd.TypeIdent{Name: typ, Package: otherPkg}, nil + default: + return crd.TypeIdent{}, fmt.Errorf("unknown package %q", pkgName) + } + + }, + } + }) + + It("should flatten a hierarchy of references", func() { + By("setting up a series of types RootType --> SubtypeWithRef --> LeafType") + toSubtype := crd.TypeRefLink("", subtypeWithRefs.Name) + toLeaf := crd.TypeRefLink("other", leafType.Name) + fl.Parser.Schemata = map[crd.TypeIdent]apiext.JSONSchemaProps{ + rootType: apiext.JSONSchemaProps{ + Properties: map[string]apiext.JSONSchemaProps{ + "refProp": apiext.JSONSchemaProps{Ref: &toSubtype}, + }, + }, + subtypeWithRefs: apiext.JSONSchemaProps{ + AdditionalProperties: &apiext.JSONSchemaPropsOrBool{ + Schema: &apiext.JSONSchemaProps{ + Ref: &toLeaf, + }, + }, + }, + leafType: apiext.JSONSchemaProps{ + Type: "string", + Pattern: "^[abc]$", + }, + } + + By("flattening the type hierarchy") + outSchema := fl.FlattenType(rootType) + Expect(rootPkg.Errors).To(HaveLen(0)) + Expect(otherPkg.Errors).To(HaveLen(0)) + + By("verifying that it was flattened to have no references") + Expect(outSchema).To(Equal(&apiext.JSONSchemaProps{ + Properties: map[string]apiext.JSONSchemaProps{ + "refProp": apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{ + { + AdditionalProperties: &apiext.JSONSchemaPropsOrBool{ + Schema: &apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{ + {Type: "string", Pattern: "^[abc]$"}, + {}, + }, + }, + }, + }, + {}, + }, + }, + }, + })) + }) + + It("should preserve the properties of each separate use of a type without modifying the cache", func() { + By("setting up a series of types RootType --> LeafType with 3 uses") + defOne := int64(1) + defThree := int64(3) + toLeaf := crd.TypeRefLink("other", leafType.Name) + fl.Parser.Schemata = map[crd.TypeIdent]apiext.JSONSchemaProps{ + rootType: apiext.JSONSchemaProps{ + Properties: map[string]apiext.JSONSchemaProps{ + "useWithOtherPattern": apiext.JSONSchemaProps{ + Ref: &toLeaf, + Pattern: "^[cde]$", + Description: "has other pattern", + }, + "useWithMinLen": apiext.JSONSchemaProps{ + Ref: &toLeaf, + MinLength: &defOne, + Description: "has min len", + }, + "useWithMaxLen": apiext.JSONSchemaProps{ + Ref: &toLeaf, + MaxLength: &defThree, + Description: "has max len", + }, + }, + }, + leafType: apiext.JSONSchemaProps{ + Type: "string", + Pattern: "^[abc]$", + }, + } + + By("flattening the type hierarchy") + outSchema := fl.FlattenType(rootType) + Expect(rootPkg.Errors).To(HaveLen(0)) + Expect(otherPkg.Errors).To(HaveLen(0)) + + By("verifying that each use has its own properties set in allof branches") + Expect(outSchema).To(Equal(&apiext.JSONSchemaProps{ + Properties: map[string]apiext.JSONSchemaProps{ + "useWithOtherPattern": apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{ + {Type: "string", Pattern: "^[abc]$"}, + {Pattern: "^[cde]$"}, + }, + Description: "has other pattern", + }, + "useWithMinLen": apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{ + {Type: "string", Pattern: "^[abc]$"}, + {MinLength: &defOne}, + }, + Description: "has min len", + }, + "useWithMaxLen": apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{ + {Type: "string", Pattern: "^[abc]$"}, + {MaxLength: &defThree}, + }, + Description: "has max len", + }, + }, + })) + }) + + It("should copy over documentation for each use of a type", func() { + By("setting up a series of types RootType --> LeafType with 3 doc-only uses") + toLeaf := crd.TypeRefLink("other", leafType.Name) + fl.Parser.Schemata = map[crd.TypeIdent]apiext.JSONSchemaProps{ + rootType: apiext.JSONSchemaProps{ + Properties: map[string]apiext.JSONSchemaProps{ + "hasTitle": apiext.JSONSchemaProps{ + Ref: &toLeaf, + Description: "has title", + Title: "some title", + }, + "hasExample": apiext.JSONSchemaProps{ + Ref: &toLeaf, + Description: "has example", + Example: &apiext.JSON{Raw: []byte("[42]")}, + }, + "hasExternalDocs": apiext.JSONSchemaProps{ + Ref: &toLeaf, + Description: "has external docs", + ExternalDocs: &apiext.ExternalDocumentation{ + Description: "somewhere else", + URL: "https://example.com", // RFC 2606 + }, + }, + }, + }, + leafType: apiext.JSONSchemaProps{ + Type: "string", + Pattern: "^[abc]$", + }, + } + + By("flattening the type hierarchy") + outSchema := fl.FlattenType(rootType) + Expect(rootPkg.Errors).To(HaveLen(0)) + Expect(otherPkg.Errors).To(HaveLen(0)) + + By("verifying that each use has its own properties set in allof branches") + Expect(outSchema).To(Equal(&apiext.JSONSchemaProps{ + Properties: map[string]apiext.JSONSchemaProps{ + "hasTitle": apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{{Type: "string", Pattern: "^[abc]$"}, {}}, + Description: "has title", + Title: "some title", + }, + "hasExample": apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{{Type: "string", Pattern: "^[abc]$"}, {}}, + Description: "has example", + Example: &apiext.JSON{Raw: []byte("[42]")}, + }, + "hasExternalDocs": apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{{Type: "string", Pattern: "^[abc]$"}, {}}, + Description: "has external docs", + ExternalDocs: &apiext.ExternalDocumentation{ + Description: "somewhere else", + URL: "https://example.com", // RFC 2606 + }, + }, + }, + })) + }) + + It("should ignore schemata that aren't references, but continue flattening", func() { + By("setting up a series of types RootType --> LeafType with non-ref properties") + toLeaf := crd.TypeRefLink("other", leafType.Name) + toSubtype := crd.TypeRefLink("", subtypeWithRefs.Name) + fl.Parser.Schemata = map[crd.TypeIdent]apiext.JSONSchemaProps{ + rootType: apiext.JSONSchemaProps{ + Properties: map[string]apiext.JSONSchemaProps{ + "isRef": apiext.JSONSchemaProps{ + Ref: &toSubtype, + }, + "notRef": apiext.JSONSchemaProps{ + Type: "int", + }, + }, + }, + subtypeWithRefs: apiext.JSONSchemaProps{ + Properties: map[string]apiext.JSONSchemaProps{ + "leafRef": apiext.JSONSchemaProps{ + Ref: &toLeaf, + }, + "alsoNotRef": apiext.JSONSchemaProps{ + Type: "bool", + }, + }, + }, + leafType: apiext.JSONSchemaProps{ + Type: "string", + Pattern: "^[abc]$", + }, + } + + By("flattening the type hierarchy") + outSchema := fl.FlattenType(rootType) + Expect(rootPkg.Errors).To(HaveLen(0)) + Expect(otherPkg.Errors).To(HaveLen(0)) + + By("verifying that each use has its own properties set in allof branches") + Expect(outSchema).To(Equal(&apiext.JSONSchemaProps{ + Properties: map[string]apiext.JSONSchemaProps{ + "isRef": apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{ + { + Properties: map[string]apiext.JSONSchemaProps{ + "leafRef": apiext.JSONSchemaProps{ + AllOf: []apiext.JSONSchemaProps{ + {Type: "string", Pattern: "^[abc]$"}, {}, + }, + }, + "alsoNotRef": apiext.JSONSchemaProps{ + Type: "bool", + }, + }, + }, + {}, + }, + }, + "notRef": apiext.JSONSchemaProps{ + Type: "int", + }, + }, + })) + + }) +}) diff --git a/pkg/crd/schema.go b/pkg/crd/schema.go index fc1b23ee1..161497a40 100644 --- a/pkg/crd/schema.go +++ b/pkg/crd/schema.go @@ -186,8 +186,8 @@ func qualifiedName(pkgName, typeName string) string { return typeName } -// definitionLink creates a definition link for the given type and package. -func definitionLink(pkgName, typeName string) string { +// TypeRefLink creates a definition link for the given type and package. +func TypeRefLink(pkgName, typeName string) string { return defPrefix + qualifiedName(pkgName, typeName) } @@ -218,7 +218,7 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *v1beta1.JSONSchem pkgPath = "" } ctx.requestSchema(pkgPath, typeNameInfo.Name()) - link := definitionLink(pkgPath, typeNameInfo.Name()) + link := TypeRefLink(pkgPath, typeNameInfo.Name()) return &v1beta1.JSONSchemaProps{ Ref: &link, } @@ -235,7 +235,7 @@ func namedToSchema(ctx *schemaContext, named *ast.SelectorExpr) *v1beta1.JSONSch typeNameInfo := typeInfo.Obj() nonVendorPath := loader.NonVendorPath(typeNameInfo.Pkg().Path()) ctx.requestSchema(nonVendorPath, typeNameInfo.Name()) - link := definitionLink(nonVendorPath, typeNameInfo.Name()) + link := TypeRefLink(nonVendorPath, typeNameInfo.Name()) return &v1beta1.JSONSchemaProps{ Ref: &link, }