Merge pull request #564 from jpbetz/backward-compat-model-names

Compatiblity fix: Always use OpenAPIModelName() functions if they are avalable

Author: k8s-ci-robot

pkg/generators/config.go

@@ -69,7 +69,6 @@ func GetOpenAPITargets(context *generator.Context, args *args.Args, boilerplate
 					newOpenAPIGen(
 						args.OutputFile,
 						args.OutputPkg,
-						len(args.OutputModelNameFile) > 0,
 					),
 					newAPIViolationGen(),
 				}

pkg/generators/openapi.go

@@ -127,19 +127,17 @@ const (
 type openAPIGen struct {
 	generator.GoGenerator
 	// TargetPackage is the package that will get GetOpenAPIDefinitions function returns all open API definitions.
-	targetPackage          string
-	imports                namer.ImportTracker
-	useGeneratedModelNames bool
+	targetPackage string
+	imports       namer.ImportTracker
 }
 
-func newOpenAPIGen(outputFilename string, targetPackage string, useGeneratedModelNames bool) generator.Generator {
+func newOpenAPIGen(outputFilename string, targetPackage string) generator.Generator {
 	return &openAPIGen{
 		GoGenerator: generator.GoGenerator{
 			OutputFilename: outputFilename,
 		},
-		imports:                generator.NewImportTrackerForPackage(targetPackage),
-		targetPackage:          targetPackage,
-		useGeneratedModelNames: useGeneratedModelNames,
+		imports:       generator.NewImportTrackerForPackage(targetPackage),
+		targetPackage: targetPackage,
 	}
 }
 
@@ -181,7 +179,7 @@ func (g *openAPIGen) Init(c *generator.Context, w io.Writer) error {
 	sw.Do("return map[string]$.OpenAPIDefinition|raw${\n", argsFromType(nil))
 
 	for _, t := range c.Order {
-		err := newOpenAPITypeWriter(sw, c, g.useGeneratedModelNames).generateCall(t)
+		err := newOpenAPITypeWriter(sw, c).generateCall(t)
 		if err != nil {
 			return err
 		}
@@ -196,7 +194,7 @@ func (g *openAPIGen) Init(c *generator.Context, w io.Writer) error {
 func (g *openAPIGen) GenerateType(c *generator.Context, t *types.Type, w io.Writer) error {
 	klog.V(5).Infof("generating for type %v", t)
 	sw := generator.NewSnippetWriter(w, c, "$", "$")
-	err := newOpenAPITypeWriter(sw, c, g.useGeneratedModelNames).generate(t)
+	err := newOpenAPITypeWriter(sw, c).generate(t)
 	if err != nil {
 		return err
 	}
@@ -235,16 +233,14 @@ type openAPITypeWriter struct {
 	refTypes               map[string]*types.Type
 	enumContext            *enumContext
 	GetDefinitionInterface *types.Type
-	useGeneratedModelNames bool
 }
 
-func newOpenAPITypeWriter(sw *generator.SnippetWriter, c *generator.Context, useGeneratedModelNames bool) openAPITypeWriter {
+func newOpenAPITypeWriter(sw *generator.SnippetWriter, c *generator.Context) openAPITypeWriter {
 	return openAPITypeWriter{
-		SnippetWriter:          sw,
-		context:                c,
-		refTypes:               map[string]*types.Type{},
-		enumContext:            newEnumContext(c),
-		useGeneratedModelNames: useGeneratedModelNames,
+		SnippetWriter: sw,
+		context:       c,
+		refTypes:      map[string]*types.Type{},
+		enumContext:   newEnumContext(c),
 	}
 }
 
@@ -299,6 +295,40 @@ func hasOpenAPIV3OneOfMethod(t *types.Type) bool {
 	return false
 }
 
+func hasOpenAPIModelName(t *types.Type) bool {
+	for mn, mt := range t.Methods {
+		if mn != "OpenAPIModelName" {
+			continue
+		}
+		return methodReturnsValue(mt, "", "string")
+	}
+	return false
+}
+
+func (g openAPITypeWriter) shouldUseOpenAPIModelName(t *types.Type) bool {
+	// Finds non-generated OpenAPIModelName() functions.
+	// Generated OpenAPIModelName() are ignored due to the 'ignore_autogenerated' build tag
+	// but are handled below by checking for use of the +k8s:openapi-model-package.
+	// This approach allows code generators to be called in any order.
+	if hasOpenAPIModelName(t) {
+		return true
+	}
+
+	value, err := extractOpenAPISchemaNamePackage(t.CommentLines)
+	if err != nil {
+		klog.Fatalf("Type %v: invalid %s:%v", t, tagModelPackage, err)
+	}
+	if value != "" {
+		return true
+	}
+	pkg := g.context.Universe.Package(t.Name.Package)
+	value, err = extractOpenAPISchemaNamePackage(pkg.Comments)
+	if err != nil {
+		klog.Fatalf("Package %v: invalid %s:%v", pkg, tagModelPackage, err)
+	}
+	return value != ""
+}
+
 // typeShortName returns short package name (e.g. the name x appears in package x definition) dot type name.
 func typeShortName(t *types.Type) string {
 	// `path` vs. `filepath` because packages use '/'
@@ -349,7 +379,7 @@ func (g openAPITypeWriter) generateCall(t *types.Type) error {
 
 		args := argsFromType(t)
 
-		if g.useGeneratedModelNames {
+		if g.shouldUseOpenAPIModelName(t) {
 			g.Do("$.|raw${}.OpenAPIModelName(): ", t)
 		} else {
 			// Legacy case: use the "canonical type name"
@@ -685,7 +715,7 @@ func (g openAPITypeWriter) generate(t *types.Type) error {
 			g.Do("Dependencies: []string{\n", args)
 			for _, k := range deps {
 				t := g.refTypes[k]
-				if g.useGeneratedModelNames {
+				if g.shouldUseOpenAPIModelName(t) {
 					g.Do("$.|raw${}.OpenAPIModelName(),", t)
 				} else {
 					g.Do("\"$.$\",", k)
@@ -1051,7 +1081,7 @@ func (g openAPITypeWriter) generateSimpleProperty(typeString, format string) {
 
 func (g openAPITypeWriter) generateReferenceProperty(t *types.Type) {
 	g.refTypes[t.Name.String()] = t
-	if g.useGeneratedModelNames {
+	if g.shouldUseOpenAPIModelName(t) {
 		g.Do("Ref: ref($.|raw${}.OpenAPIModelName()),\n", t)
 	} else {
 		g.Do("Ref: ref(\"$.$\"),\n", t.Name.String())

pkg/generators/openapi_test.go

@@ -68,11 +68,11 @@ func testOpenAPITypeWriter(t *testing.T, cfg *packages.Config) (error, error, *b
 
 	callBuffer := &bytes.Buffer{}
 	callSW := generator.NewSnippetWriter(callBuffer, context, "$", "$")
-	callError := newOpenAPITypeWriter(callSW, context, false).generateCall(blahT)
+	callError := newOpenAPITypeWriter(callSW, context).generateCall(blahT)
 
 	funcBuffer := &bytes.Buffer{}
 	funcSW := generator.NewSnippetWriter(funcBuffer, context, "$", "$")
-	funcError := newOpenAPITypeWriter(funcSW, context, false).generate(blahT)
+	funcError := newOpenAPITypeWriter(funcSW, context).generate(blahT)
 
 	return callError, funcError, callBuffer, funcBuffer, imports.ImportLines()
 }

test/integration/naming_test.go

@@ -0,0 +1,53 @@
+/*
+Copyright 2024 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 integration
+
+import (
+	"fmt"
+	"testing"
+
+	"k8s.io/kube-openapi/pkg/util"
+	"k8s.io/kube-openapi/pkg/validation/spec"
+	generated "k8s.io/kube-openapi/test/integration/pkg/generated/namedmodels"
+	"k8s.io/kube-openapi/test/integration/testdata/namedmodels"
+)
+
+func TestCanonicalTypeNames(t *testing.T) {
+	defs := generated.GetOpenAPIDefinitions(func(path string) spec.Ref {
+		return spec.MustCreateRef(path)
+	})
+
+	// Define the types we want to check.
+	typeTestCases := []any{namedmodels.Struct{}, namedmodels.ContainedStruct{}, namedmodels.AtomicStruct{}}
+
+	for _, v := range typeTestCases {
+		t.Run(fmt.Sprintf("%T", v), func(t *testing.T) {
+			// Get the canonical name using the generator's logic.
+			canonicalName := util.GetCanonicalTypeName(v)
+
+			// Check if the canonical name exists as a key in the generated map.
+			if _, ok := defs[canonicalName]; !ok {
+				t.Errorf("canonical type name %q for type %q not found in GetOpenAPIDefinitions", canonicalName, v)
+			}
+		})
+	}
+
+	// Additionally, verify that the number of generated definitions matches our expectation.
+	if len(defs) != len(typeTestCases) {
+		t.Errorf("expected %d definitions, but got %d", len(typeTestCases), len(defs))
+	}
+}

test/integration/pkg/generated/namedmodels/openapi_generated.go

@@ -1,3 +1,2 @@
 // +k8s:openapi-gen=true
-// +k8s:openapi-model-package=io.k8s.kube-openapi.test.integration.testdata.namedmodels
 package namedmodels

test/integration/testdata/namedmodels/doc.go

@@ -1,12 +1,18 @@
 package namedmodels
 
-type Struct struct {
+// +k8s:openapi-model-package=io.k8s.kube-openapi.test.integration.testdata.namedmodels
+type Struct struct { // The generator should see the +k8s:openapi-model-package and assume that a OpenAPIModelName is (or will be) generated.
 	Field      ContainedStruct
 	OtherField int
 }
 
-type ContainedStruct struct{}
+type ContainedStruct struct{} // The generator should use the go package name since there is no OpenAPIModelName declared.
 
-type AtomicStruct struct {
+type AtomicStruct struct { // The generator should respect a manually declared OpenAPIModelName.
 	Field int
 }
+
+// OpenAPIModelName returns the OpenAPI model name for this type.
+func (in AtomicStruct) OpenAPIModelName() string {
+	return "io.k8s.kube-openapi.test.integration.testdata.namedmodels.AtomicStruct"
+}