From 34e2e5357e4ac139ed4325e83c5f19abd9cb7b15 Mon Sep 17 00:00:00 2001 From: Erik Miller Date: Sat, 26 Sep 2026 16:07:58 -0700 Subject: [PATCH] fix: generate required object-typed fields as non-pointer values A required, object-typed CRD property (e.g. a managed resource's spec.forProvider) generates as a pointer field with omitempty, regardless of being marked required in the source schema. goRemoveRequired unconditionally cleared every schema's required list before oapi-codegen ran, so oapi-codegen's "optional -> pointer + omitempty" rule applied uniformly. A hand/controller-gen-written Kubernetes or crossplane-runtime type declares a required nested object as a non-pointer value struct instead. Its zero value marshals as {}, satisfying the CRD's required-key admission check even when a caller never sets it. The generated pointer form fails that check either way: omitted from the JSON with omitempty, or a literal null without it. goRemoveRequired now keeps required-ness for object-typed (struct-shaped) properties only, so oapi-codegen generates those as non-pointer values with no omitempty. Every other field (scalars, arrays, map-shaped objects) keeps the existing all-pointer/all-optional convention, since the rest of this generator's Go output assumes it. isStructShapedProperty decides which properties qualify: - Struct-shaped means "has named properties," whether or not the schema also allows additionalProperties: oapi-codegen v2 emits a struct (with an extra AdditionalProperties map field) once any named properties are present, not a bare map. - A direct $ref, or an allOf with exactly one element that is itself a $ref (the shape Kubernetes' own OpenAPI uses for a required nested object, e.g. DeviceClass.spec in resource.k8s.io/v1), is resolved against components.schemas before judging its shape, since it has no inline properties of its own. - A ref that resolves to one of the k8s API machinery types this generator moves into a separately generated shared package (metav1.LabelSelector and similar) stays a pointer even though it resolves to a struct: it would become a cross-package non-pointer value, and this generator's accessors and DeepCopy machinery only special-case a non-pointer field that's a locally declared struct. - A $ref/allOf alongside sibling inline properties, or a multi-element allOf, stays a pointer too: oapi-codegen v2.8 merges those into an anonymous inline struct literal rather than a reference to a named local type, and an anonymous struct can't be given a DeepCopyInto method or be recognized by the accessors code. Only a lone $ref (direct, or the sole allOf member) with no sibling properties resolves to a named type and can safely become non-pointer. - A required $ref that would make the generated struct contain itself by value is also kept a pointer. goRequiredValueReachability precomputes, for every named component schema, which other schemas it would reach by value if every eligible required field were kept non-pointer, following nested inline structs but not slices or maps (those already break value containment). filterRequiredObjectFields uses that to reject a $ref whose target is, or can reach back to, the schema the field is on directly or through another schema's own required fields (e.g. two schemas requiring each other) - Go rejects a struct that contains itself by value, however indirectly. Two related fixes to writeFieldCopy's generated DeepCopyInto: - It now calls DeepCopyInto on a non-pointer local-struct field instead of relying on the top-level shallow *out = *in, which would alias any pointers nested inside it between the original and the copy. - It also copies the backing bytes of oapi-codegen's unexported `union json.RawMessage` field (its oneOf/anyOf plumbing) instead of aliasing them, since that type's MarshalJSON exposes the backing slice directly. This fix is independent of the flag below: it applies to the existing runtime.Object/DeepCopy generation whenever a oneOf/anyOf union is present, whether or not a required object-typed field is involved. collectStructTypes (shared by writeFieldCopy and the accessors generator) now also resolves a component-name type alias to the struct it names. oapi-codegen emits `type IoK8SApiResourceV1DeviceClassSpec = DeviceClassSpec` for a schema whose derived name differs from its customized one, and a $ref-resolved required field like DeviceClass.spec is typed by that alias, not the struct name directly. Without resolving it, the field wouldn't be recognized as a locally declared struct at all. This also fixes the same shallow-copy gap for 26 existing optional pointer fields in this repo's built-in OpenAPI testdata that are typed by such an alias, independent of requiredObjectFields. The generated GetX/SetX accessors for required object-typed fields stay pointer-shaped (address on Get, dereference on Set), so chained getters and SetX(GetX()) round-trips keep compiling regardless of which fields happen to be required. The per-field getter/setter logic is factored into writeFieldAccessors so writeStructAccessors stays under golangci-lint's gocognit threshold with the new value-struct branch. Feature-flagged, off by default: making a required object-typed field a non-pointer value is a breaking change for any consumer that constructs it as a pointer (Spec: &BucketSpec{...} no longer type-checks) or checks it for nil. It's gated behind a new features.generateGoRequiredObjectFields config flag, following the same WithGoModelAccessors/WithGoRuntimeObjects pattern already used for generateGoModelAccessors and generateGoRuntimeObjects: crossplane config set features.generateGoRequiredObjectFields true With the flag disabled (the default), no field's pointer/optional shape changes. The union json.RawMessage byte copy and the alias resolution for an ordinary optional pointer field are independent of this flag and take effect whenever runtimeObjects is on; the non-pointer alias case only arises when the flag is also on, since that's what makes a field non-pointer in the first place. Tested with new/updated unit tests (TestGenerateFromOpenAPIGoRequiredObjectFields, TestGenerateFromOpenAPIGoRequiredObjectFieldsAlias, TestIsStructShapedProperty, TestGoRemoveRequiredAllOfShapes, TestGoRemoveRequiredValueCycles, TestWriteFieldCopy, TestAddAccessorsValueStructField, TestAllLanguagesGoOptions) plus the existing and extended compile-gate suite (go test -tags compilegate ./internal/schemas/generator/...), all passing. Signed-off-by: Erik Miller --- cmd/crossplane/composition/generate.go | 1 + cmd/crossplane/config/help/config.md | 9 + cmd/crossplane/config/set.go | 9 +- cmd/crossplane/dependency/add.go | 1 + cmd/crossplane/dependency/cache.go | 1 + cmd/crossplane/function/generate.go | 1 + cmd/crossplane/project/build.go | 1 + cmd/crossplane/project/run.go | 1 + cmd/crossplane/render/op/cmd.go | 1 + cmd/crossplane/render/xr/cmd.go | 1 + internal/config/config.go | 7 + internal/schemas/generator/accessors.go | 93 ++- internal/schemas/generator/accessors_test.go | 76 +++ internal/schemas/generator/go.go | 279 ++++++++- internal/schemas/generator/go_test.go | 587 ++++++++++++++++++ internal/schemas/generator/interface.go | 21 +- internal/schemas/generator/interface_test.go | 34 +- internal/schemas/generator/runtimeobject.go | 88 ++- .../runtimeobject_compilegate_test.go | 348 ++++++++--- .../schemas/generator/runtimeobject_test.go | 112 ++++ 20 files changed, 1519 insertions(+), 152 deletions(-) diff --git a/cmd/crossplane/composition/generate.go b/cmd/crossplane/composition/generate.go index dbf7c797..a7d80936 100644 --- a/cmd/crossplane/composition/generate.go +++ b/cmd/crossplane/composition/generate.go @@ -109,6 +109,7 @@ func (c *generateCmd) AfterApply(cfg *config.Config) error { generator.AllLanguages( generator.WithGoModelAccessors(cfg.Features.GenerateGoModelAccessors), generator.WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects), + generator.WithGoRequiredObjectFields(cfg.Features.GenerateGoRequiredObjectFields), ), proj.Spec.Schemas.GetLanguages(), )), diff --git a/cmd/crossplane/config/help/config.md b/cmd/crossplane/config/help/config.md index d6dbe368..25556ab1 100644 --- a/cmd/crossplane/config/help/config.md +++ b/cmd/crossplane/config/help/config.md @@ -33,3 +33,12 @@ generated Go models (off by default), so you can register generated types with a ```shell crossplane config set features.generateGoRuntimeObjects true ``` + +Generate a required object-typed property (for example, a managed resource's +`spec.forProvider`) on generated Go models as a non-pointer value instead of a +pointer (off by default; this is a breaking change for code that constructs +such a field as a pointer or nil-checks it): + +```shell +crossplane config set features.generateGoRequiredObjectFields true +``` diff --git a/cmd/crossplane/config/set.go b/cmd/crossplane/config/set.go index 7a8d9d89..34898f76 100644 --- a/cmd/crossplane/config/set.go +++ b/cmd/crossplane/config/set.go @@ -42,10 +42,11 @@ type boolSetter func(c *config.Config, v bool) // //nolint:gochecknoglobals // This is a constant. var boolKeys = map[string]boolSetter{ - "features.enableAlpha": func(c *config.Config, v bool) { c.Features.EnableAlpha = v }, - "features.disableBeta": func(c *config.Config, v bool) { c.Features.DisableBeta = v }, - "features.generateGoModelAccessors": func(c *config.Config, v bool) { c.Features.GenerateGoModelAccessors = v }, - "features.generateGoRuntimeObjects": func(c *config.Config, v bool) { c.Features.GenerateGoRuntimeObjects = v }, + "features.enableAlpha": func(c *config.Config, v bool) { c.Features.EnableAlpha = v }, + "features.disableBeta": func(c *config.Config, v bool) { c.Features.DisableBeta = v }, + "features.generateGoModelAccessors": func(c *config.Config, v bool) { c.Features.GenerateGoModelAccessors = v }, + "features.generateGoRuntimeObjects": func(c *config.Config, v bool) { c.Features.GenerateGoRuntimeObjects = v }, + "features.generateGoRequiredObjectFields": func(c *config.Config, v bool) { c.Features.GenerateGoRequiredObjectFields = v }, } func (c *setCmd) AfterApply() error { diff --git a/cmd/crossplane/dependency/add.go b/cmd/crossplane/dependency/add.go index 6424f473..3d8e2513 100644 --- a/cmd/crossplane/dependency/add.go +++ b/cmd/crossplane/dependency/add.go @@ -94,6 +94,7 @@ func (c *addCmd) Run(logger logging.Logger, sp terminal.SpinnerPrinter, cfg *con generator.AllLanguages( generator.WithGoModelAccessors(cfg.Features.GenerateGoModelAccessors), generator.WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects), + generator.WithGoRequiredObjectFields(cfg.Features.GenerateGoRequiredObjectFields), ), proj.Spec.Schemas.GetLanguages(), )), diff --git a/cmd/crossplane/dependency/cache.go b/cmd/crossplane/dependency/cache.go index 3cecc882..887312d8 100644 --- a/cmd/crossplane/dependency/cache.go +++ b/cmd/crossplane/dependency/cache.go @@ -89,6 +89,7 @@ func (c *updateCacheCmd) Run(logger logging.Logger, sp terminal.SpinnerPrinter, generator.AllLanguages( generator.WithGoModelAccessors(cfg.Features.GenerateGoModelAccessors), generator.WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects), + generator.WithGoRequiredObjectFields(cfg.Features.GenerateGoRequiredObjectFields), ), proj.Spec.Schemas.GetLanguages(), )), diff --git a/cmd/crossplane/function/generate.go b/cmd/crossplane/function/generate.go index 3d1fc20a..784618d4 100644 --- a/cmd/crossplane/function/generate.go +++ b/cmd/crossplane/function/generate.go @@ -161,6 +161,7 @@ func (c *generateCmd) Run(sp terminal.SpinnerPrinter, cfg *config.Config) error generator.AllLanguages( generator.WithGoModelAccessors(cfg.Features.GenerateGoModelAccessors), generator.WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects), + generator.WithGoRequiredObjectFields(cfg.Features.GenerateGoRequiredObjectFields), ), c.proj.Spec.Schemas.GetLanguages(), ), diff --git a/cmd/crossplane/project/build.go b/cmd/crossplane/project/build.go index 06222ae6..85262f12 100644 --- a/cmd/crossplane/project/build.go +++ b/cmd/crossplane/project/build.go @@ -102,6 +102,7 @@ func (c *buildCmd) Run(logger logging.Logger, sp terminal.SpinnerPrinter, cfg *c generator.AllLanguages( generator.WithGoModelAccessors(cfg.Features.GenerateGoModelAccessors), generator.WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects), + generator.WithGoRequiredObjectFields(cfg.Features.GenerateGoRequiredObjectFields), ), c.proj.Spec.Schemas.GetLanguages(), ) diff --git a/cmd/crossplane/project/run.go b/cmd/crossplane/project/run.go index 73ba0ad7..c73716b3 100644 --- a/cmd/crossplane/project/run.go +++ b/cmd/crossplane/project/run.go @@ -156,6 +156,7 @@ func (c *runCmd) Run(logger logging.Logger, sp terminal.SpinnerPrinter, cfg *con generator.AllLanguages( generator.WithGoModelAccessors(cfg.Features.GenerateGoModelAccessors), generator.WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects), + generator.WithGoRequiredObjectFields(cfg.Features.GenerateGoRequiredObjectFields), ), c.proj.Spec.Schemas.GetLanguages(), ) diff --git a/cmd/crossplane/render/op/cmd.go b/cmd/crossplane/render/op/cmd.go index 2274af1d..2984c029 100644 --- a/cmd/crossplane/render/op/cmd.go +++ b/cmd/crossplane/render/op/cmd.go @@ -360,6 +360,7 @@ func (c *Cmd) loadFunctions(ctx context.Context, log logging.Logger, sp terminal generators := generator.AllLanguages( generator.WithGoModelAccessors(cfg.Features.GenerateGoModelAccessors), generator.WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects), + generator.WithGoRequiredObjectFields(cfg.Features.GenerateGoRequiredObjectFields), ) depMgr := dependency.NewManager(proj, projFS, diff --git a/cmd/crossplane/render/xr/cmd.go b/cmd/crossplane/render/xr/cmd.go index 0a41121d..3978adae 100644 --- a/cmd/crossplane/render/xr/cmd.go +++ b/cmd/crossplane/render/xr/cmd.go @@ -472,6 +472,7 @@ func (c *Cmd) loadFunctionsFromProject(ctx context.Context, log logging.Logger, generators := generator.AllLanguages( generator.WithGoModelAccessors(cfg.Features.GenerateGoModelAccessors), generator.WithGoRuntimeObjects(cfg.Features.GenerateGoRuntimeObjects), + generator.WithGoRequiredObjectFields(cfg.Features.GenerateGoRequiredObjectFields), ) depMgr := dependency.NewManager(proj, projFS, diff --git a/internal/config/config.go b/internal/config/config.go index 7c096468..78b5bd64 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -52,6 +52,13 @@ type Features struct { // per-package AddToScheme helpers on generated Go models. Disabled by // default; opt in to register generated types with a runtime.Scheme. GenerateGoRuntimeObjects bool `json:"generateGoRuntimeObjects,omitempty"` + + // GenerateGoRequiredObjectFields enables generating a required + // object-typed property (e.g. a managed resource's spec.forProvider) as a + // non-pointer value instead of the all-pointer/all-optional default. + // Disabled by default; it's a breaking change for any consumer that + // constructs such a field as a pointer or nil-checks it. + GenerateGoRequiredObjectFields bool `json:"generateGoRequiredObjectFields,omitempty"` } // Load reads a Config from path. A missing file is not an error; the zero diff --git a/internal/schemas/generator/accessors.go b/internal/schemas/generator/accessors.go index 09fdaf35..9605980e 100644 --- a/internal/schemas/generator/accessors.go +++ b/internal/schemas/generator/accessors.go @@ -60,6 +60,11 @@ func addAccessors(code string) (string, error) { // duplicate method would make the package fail to compile. existing := collectExistingMethods(f) + // writeStructAccessors needs to know which fields are non-pointer struct + // values (required object-typed fields, see goRemoveRequired in go.go), + // so it can still return/accept a pointer for them. + structs := collectStructTypes(f) + var b strings.Builder // Walk declarations in source order so the generated output is stable. for _, decl := range f.Decls { @@ -81,7 +86,7 @@ func addAccessors(code string) (string, error) { if !ok || st.Fields == nil { continue } - writeStructAccessors(&b, fset, receiverTypeExpr(ts), st, existing[ts.Name.Name]) + writeStructAccessors(&b, fset, receiverTypeExpr(ts), st, existing[ts.Name.Name], structs) } } @@ -201,7 +206,7 @@ func isNilable(e ast.Expr) bool { // (e.g. a field named PasswordData alongside a sibling field named // GetPasswordData): Go forbids a method and a field sharing a name on the same // type, and Terraform schemas occasionally produce exactly that pair. -func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName string, st *ast.StructType, skip map[string]bool) { +func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName string, st *ast.StructType, skip map[string]bool, structs map[string]bool) { fieldNames := collectFieldNames(st) for _, field := range st.Fields.List { @@ -216,8 +221,14 @@ func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName stri // a node we just parsed; skip defensively rather than panic. continue } - fieldType := typ.String() + shape := fieldAccessorShape{ + fieldType: typ.String(), + isValueStruct: isValueStructField(field.Type, structs), + isNilable: isNilable(field.Type), + skip: skip, + siblingFields: fieldNames, + } for _, name := range field.Names { // Skip unexported fields: an accessor for them would be useless to // external consumers and could produce oddly-cased method names. @@ -225,14 +236,40 @@ func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName stri if !name.IsExported() { continue } + writeFieldAccessors(b, typeName, name.Name, shape) + } + } +} - fieldName := name.Name - if !skip["Get"+fieldName] && !fieldNames["Get"+fieldName] { - writeGetter(b, typeName, fieldName, fieldType, isNilable(field.Type)) - } - if !skip["Set"+fieldName] && !fieldNames["Set"+fieldName] { - writeSetter(b, typeName, fieldName, fieldType) - } +// fieldAccessorShape carries the per-field-declaration context +// writeFieldAccessors needs, so its signature stays small as more shapes +// (see isValueStructField) are added. +type fieldAccessorShape struct { + fieldType string + isValueStruct bool + isNilable bool + skip map[string]bool + siblingFields map[string]bool +} + +// writeFieldAccessors appends the getter/setter pair for one field, in +// either the value-struct shape (see isValueStructField) or the default +// pointer shape. An accessor is omitted if its name is already claimed in +// shape.skip (an oapi-codegen-generated method) or shape.siblingFields (a +// same-named sibling field; see writeStructAccessors). +func writeFieldAccessors(b *strings.Builder, typeName, fieldName string, shape fieldAccessorShape) { + if !shape.skip["Get"+fieldName] && !shape.siblingFields["Get"+fieldName] { + if shape.isValueStruct { + writeValueStructGetter(b, typeName, fieldName, shape.fieldType) + } else { + writeGetter(b, typeName, fieldName, shape.fieldType, shape.isNilable) + } + } + if !shape.skip["Set"+fieldName] && !shape.siblingFields["Set"+fieldName] { + if shape.isValueStruct { + writeValueStructSetter(b, typeName, fieldName, shape.fieldType) + } else { + writeSetter(b, typeName, fieldName, shape.fieldType) } } } @@ -283,3 +320,39 @@ func writeSetter(b *strings.Builder, typeName, fieldName, fieldType string) { b.WriteString("\t" + accessorReceiver + "." + fieldName + " = v\n") b.WriteString("}\n") } + +// isValueStructField reports whether typ is a bare reference to a known +// local struct — the shape a required object-typed field gets (see +// goRemoveRequired), unlike every other field, which is a pointer. +func isValueStructField(typ ast.Expr, structs map[string]bool) bool { + id, ok := typ.(*ast.Ident) + return ok && structs[id.Name] +} + +// writeValueStructGetter appends a getter for a required object-typed field +// (a non-pointer value, see isValueStructField). Returns a pointer so +// callers can chain getters like every other field, and nil on a nil +// receiver instead of panicking. +func writeValueStructGetter(b *strings.Builder, typeName, fieldName, fieldType string) { + b.WriteString("\n// Get" + fieldName + " returns a pointer to the " + fieldName + " field.\n") + b.WriteString("// It returns nil if the receiver is nil.\n") + b.WriteString("func (" + accessorReceiver + " *" + typeName + ") Get" + fieldName + "() *" + fieldType + " {\n") + b.WriteString("\tif " + accessorReceiver + " == nil {\n") + b.WriteString("\t\treturn nil\n") + b.WriteString("\t}\n") + b.WriteString("\treturn &" + accessorReceiver + "." + fieldName + "\n") + b.WriteString("}\n") +} + +// writeValueStructSetter appends a setter for a required object-typed +// field. Takes a pointer to mirror the getter so SetX(GetX()) round-trips, +// and no-ops on nil since the field can only be replaced, not unset. +func writeValueStructSetter(b *strings.Builder, typeName, fieldName, fieldType string) { + b.WriteString("\n// Set" + fieldName + " sets the " + fieldName + " field from v. It does nothing if v is nil.\n") + b.WriteString("func (" + accessorReceiver + " *" + typeName + ") Set" + fieldName + "(v *" + fieldType + ") {\n") + b.WriteString("\tif v == nil {\n") + b.WriteString("\t\treturn\n") + b.WriteString("\t}\n") + b.WriteString("\t" + accessorReceiver + "." + fieldName + " = *v\n") + b.WriteString("}\n") +} diff --git a/internal/schemas/generator/accessors_test.go b/internal/schemas/generator/accessors_test.go index 53241f4e..0d0596d6 100644 --- a/internal/schemas/generator/accessors_test.go +++ b/internal/schemas/generator/accessors_test.go @@ -373,6 +373,82 @@ type FooAlias = Foo } } +// TestAddAccessorsValueStructField verifies a non-pointer struct field (the +// shape goRemoveRequired gives a required object-typed property) still gets +// a pointer-shaped Get/Set pair, so callers can chain getters and round-trip +// SetX(GetX()) like every other field. +func TestAddAccessorsValueStructField(t *testing.T) { + cases := map[string]struct { + args string + want map[string]string + reason string + }{ + "ValueStructField": { + args: `package v1alpha1 + +type Foo struct { + Bar BarValue ` + "`json:\"bar\"`" + ` +} + +type BarValue struct { + Count *int64 ` + "`json:\"count,omitempty\"`" + ` +} +`, + want: map[string]string{ + "Foo.GetBar": "*BarValue", + "Foo.SetBar": "*BarValue", + "BarValue.GetCount": "*int64", + "BarValue.SetCount": "*int64", + }, + reason: "a non-pointer struct field still gets a pointer-shaped Get/Set pair", + }, + "ValueStructFieldViaAlias": { + args: `package v1alpha1 + +type Foo struct { + Bar BarAlias ` + "`json:\"bar\"`" + ` +} + +type RealBar struct { + Count *int64 ` + "`json:\"count,omitempty\"`" + ` +} + +type BarAlias = RealBar +`, + want: map[string]string{ + "Foo.GetBar": "*BarAlias", + "Foo.SetBar": "*BarAlias", + "RealBar.GetCount": "*int64", + "RealBar.SetCount": "*int64", + }, + reason: "a field typed by a component-name alias (oapi-codegen's x-go-type-name pattern) is still recognized as the local struct it names", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got, err := addAccessors(tc.args) + if err != nil { + t.Fatalf("addAccessors returned error: %v", err) + } + + if diff := cmp.Diff(tc.want, collectMethods(t, got)); diff != "" { + t.Errorf("generated accessors (-want +got, %s):\n%s", tc.reason, diff) + } + + // The round-trip pattern every other field supports must still + // type-check for a value-struct field. + roundTrip := got + ` + +func roundTrip(f *Foo) { + f.SetBar(f.GetBar()) +} +` + typeCheck(t, roundTrip) + }) + } +} + // guardsNilReceiver reports whether the body of method recv.name opens with an // `if == nil` guard. func guardsNilReceiver(t *testing.T, src, recv, name string) bool { diff --git a/internal/schemas/generator/go.go b/internal/schemas/generator/go.go index c57784e8..5e569bb2 100644 --- a/internal/schemas/generator/go.go +++ b/internal/schemas/generator/go.go @@ -65,6 +65,18 @@ const ( k8sPkgNameAutoscaling = "autoscaling" ) +// isK8sSharedTypeRef reports whether ref names one of the k8s API machinery +// types goReferenceK8sTypeWithMetaPath moves into a separately generated +// shared package. +func isK8sSharedTypeRef(ref string) bool { + return strings.Contains(ref, k8sPkgMetaV1) || + strings.Contains(ref, k8sPkgCoreV1) || + strings.Contains(ref, k8sPkgRuntime) || + strings.Contains(ref, k8sPkgIntStr) || + strings.Contains(ref, k8sPkgResource) || + strings.Contains(ref, k8sPkgAutoscalingV1) +} + // goModContents is the contents of the go.mod we write for our generated models // module. All generated models share the same module so that we can generate a // single dependency from embedded Go functions. We always resolve this @@ -235,10 +247,24 @@ var ( // goGenerator generates Go models. accessors controls whether GetX/SetX // accessor methods are emitted for the generated structs; runtimeObjects // controls whether DeepCopy / runtime.Object methods and per-package -// AddToScheme helpers are emitted. +// AddToScheme helpers are emitted; requiredObjectFields controls whether a +// required object-typed property generates as a non-pointer value (see +// goRemoveRequired) instead of the all-pointer/all-optional default. type goGenerator struct { - accessors bool - runtimeObjects bool + accessors bool + runtimeObjects bool + requiredObjectFields bool +} + +// requiredMutator returns the required-field mutator matching g's +// requiredObjectFields setting: goRemoveRequired when enabled, or +// goRemoveRequiredAll (the pre-existing, all-fields-optional behavior) +// when disabled. +func (g goGenerator) requiredMutator() func(*spec3.OpenAPI) { + if g.requiredObjectFields { + return goRemoveRequired + } + return goRemoveRequiredAll } func (goGenerator) Language() string { @@ -294,7 +320,7 @@ func (g goGenerator) GenerateFromCRD(_ context.Context, fromFS afero.Fs, _ runne goRenameTypes, goRenameEnums, goReplaceNumberWithInt, - goRemoveRequired, + g.requiredMutator(), goReferenceK8sTypesForCRDs, goRemoveK8s, goKeepOnlyComponents, @@ -374,7 +400,7 @@ func (g goGenerator) generateSharedK8sPackage(schemaFS afero.Fs, pkg string, sch goRenameTypes, goRenameEnums, goReplaceNumberWithInt, - goRemoveRequired, + g.requiredMutator(), refMutator, ) if err != nil { @@ -959,32 +985,241 @@ func goSchemaIsValidationOnly(s *spec.Schema) bool { return true } -// goRemoveRequired removes the required fields from schemas. We want all fields -// in our generated models to be optional (so functions can set only the fields -// they wish to own). +// goRemoveRequired removes required-ness from every field except required +// object-typed properties, which keep it so oapi-codegen generates them as +// non-pointer values. That matches how real Kubernetes types declare +// required nested objects (e.g. a managed resource's Spec.ForProvider): the +// zero value marshals as `{}`, satisfying the CRD's required-key check +// without a caller ever setting it. Everything else keeps this generator's +// all-pointer/all-optional convention, since DeepCopy and accessor +// generation assume it. +// +// Gated behind the features.generateGoRequiredObjectFields config flag (see +// goGenerator.requiredObjectFields); goRemoveRequiredAll is used instead when +// the flag is disabled. func goRemoveRequired(s *spec3.OpenAPI) { + schemas := s.Components.Schemas + reaches := goRequiredValueReachability(schemas) + for name, schema := range schemas { + schema.Required = filterRequiredObjectFields(name, schema.Required, schema.Properties, schemas, reaches) + goRemovePropertiesRequired(name, schema.Properties, schemas, reaches) + if schema.Items != nil { + // Items sit behind a slice, which already breaks value + // containment, so there's no root to defend against here. + goRemovePropertiesRequired("", schema.Items.Schema.Properties, schemas, reaches) + } + } +} + +// goRemovePropertiesRequired filters props' nested required lists. root is +// the named component schema these properties are, by value, ultimately +// part of; it's threaded through unchanged into nested inline properties, +// and cleared (disabling the cycle check in filterRequiredObjectFields) when +// descending into an Items schema, since a slice element doesn't contribute +// to its container's size. +func goRemovePropertiesRequired(root string, props map[string]spec.Schema, schemas map[string]*spec.Schema, reaches map[string]map[string]bool) { + for name, prop := range props { + prop.Required = filterRequiredObjectFields(root, prop.Required, prop.Properties, schemas, reaches) + goRemovePropertiesRequired(root, prop.Properties, schemas, reaches) + if prop.Items != nil { + prop.Items.Schema.Required = filterRequiredObjectFields("", prop.Items.Schema.Required, prop.Items.Schema.Properties, schemas, reaches) + goRemovePropertiesRequired("", prop.Items.Schema.Properties, schemas, reaches) + } + + props[name] = prop + } +} + +// goRemoveRequiredAll unconditionally clears every schema's required list, so +// every generated field is optional and callers can set only the fields they +// wish to own. This is goRemoveRequired's behavior with +// features.generateGoRequiredObjectFields disabled. +func goRemoveRequiredAll(s *spec3.OpenAPI) { for _, schema := range s.Components.Schemas { schema.Required = nil - goRemovePropertiesRequired(schema.Properties) + goRemoveAllPropertiesRequired(schema.Properties) if schema.Items != nil { - goRemovePropertiesRequired(schema.Items.Schema.Properties) + goRemoveAllPropertiesRequired(schema.Items.Schema.Properties) } } } -func goRemovePropertiesRequired(props map[string]spec.Schema) { +func goRemoveAllPropertiesRequired(props map[string]spec.Schema) { for name, prop := range props { prop.Required = nil - goRemovePropertiesRequired(prop.Properties) + goRemoveAllPropertiesRequired(prop.Properties) if prop.Items != nil { prop.Items.Schema.Required = nil - goRemovePropertiesRequired(prop.Items.Schema.Properties) + goRemoveAllPropertiesRequired(prop.Items.Schema.Properties) } props[name] = prop } } +// filterRequiredObjectFields returns the subset of required naming a +// struct-shaped property (see isStructShapedProperty) that doesn't also +// close a value cycle back to root; those are the only properties allowed +// to stay required. Returns nil, not an empty slice, so the emitted OpenAPI +// has no empty `required: []`. +func filterRequiredObjectFields(root string, required []string, props map[string]spec.Schema, schemas map[string]*spec.Schema, reaches map[string]map[string]bool) []string { + var kept []string + for _, name := range required { + prop, ok := props[name] + if !ok || !isStructShapedProperty(prop, schemas) { + continue + } + if root != "" { + if target := schemaRefName(prop); target != "" && (target == root || reaches[target][root]) { + continue + } + } + kept = append(kept, name) + } + return kept +} + +// isStructShapedProperty reports whether prop is the kind of object schema +// oapi-codegen generates as a Go struct rather than a bare map. That's true +// whenever it has named properties, whether or not it also allows additional +// properties: oapi-codegen v2 emits a struct with a field per named property +// plus an extra AdditionalProperties map field, not a plain map, once any +// named properties are present. Only a map-only object (additionalProperties +// with no named properties), or a scalar/array, is excluded. +// +// A direct $ref, or an allOf with exactly one element that is itself a $ref, +// has no inline properties of its own (e.g. Kubernetes' OpenAPI shapes a +// required nested object as `{allOf: [{$ref: "#/components/schemas/Foo"}]}` +// to attach a description/default alongside the reference), so it's resolved +// against schemas first — but only when prop has no properties of its own. +// A $ref or allOf alongside sibling inline properties, or an allOf with more +// than one element, is excluded even though oapi-codegen v2.8 may resolve +// one of its members to a struct: in either shape, oapi-codegen merges the +// result into an anonymous inline struct literal, not a reference to a named +// local type. An anonymous struct can't be given a DeepCopyInto method and +// isn't a locally declared struct this generator's accessors/DeepCopy code +// can recognize, so treating it as struct-shaped would produce a shallow, +// aliasing DeepCopy. Only a lone $ref (direct, or the sole allOf member) +// with no sibling properties resolves to a named type and is safe. +// +// A ref to one of the k8s API machinery types goReferenceK8sType moves into a +// separately generated shared package is excluded even though it resolves to +// a struct: it becomes a cross-package value type, and this generator's +// accessors and DeepCopy machinery only special-case a non-pointer field that +// is a locally declared struct, so such a field must stay a pointer. +func isStructShapedProperty(prop spec.Schema, schemas map[string]*spec.Schema) bool { + if ref := schemaRef(prop); ref.String() != "" && isK8sSharedTypeRef(ref.String()) { + return false + } + if len(prop.Properties) > 0 { + return prop.Ref.String() == "" && len(prop.AllOf) == 0 + } + if len(prop.AllOf) > 1 { + return false + } + return len(resolveLocalSchemaRef(prop, schemas).Properties) > 0 +} + +// schemaRef returns prop's direct $ref, or the $ref of an allOf with exactly +// one element, or the zero Ref if prop isn't ref-shaped. +func schemaRef(prop spec.Schema) spec.Ref { + if prop.Ref.String() != "" { + return prop.Ref + } + if len(prop.AllOf) == 1 { + return prop.AllOf[0].Ref + } + return spec.Ref{} +} + +// schemaRefName returns the component name schemaRef(prop) names, or "" if +// prop isn't ref-shaped. +func schemaRefName(prop spec.Schema) string { + ref := schemaRef(prop) + return strings.TrimPrefix(ref.String(), "#/components/schemas/") +} + +// resolveLocalSchemaRef follows prop's direct $ref, or the $ref of an allOf +// with exactly one element, to the schema it names in schemas. Returns prop +// unchanged if it isn't ref-shaped, or if the ref doesn't resolve locally. +func resolveLocalSchemaRef(prop spec.Schema, schemas map[string]*spec.Schema) spec.Schema { + name := schemaRefName(prop) + if name == "" { + return prop + } + if resolved, ok := schemas[name]; ok && resolved != nil { + return *resolved + } + return prop +} + +// goRequiredValueReachability returns, for each named component schema, the +// set of other named component schemas it would contain by value if every +// eligible required object-typed field were kept non-pointer. It's computed +// once, up front, over the unmodified schema graph, so the result doesn't +// depend on the order goRemoveRequired happens to visit schemas in. +// +// filterRequiredObjectFields uses it to keep a required $ref pointer-shaped +// when the alternative would make oapi-codegen emit a Go struct that +// contains itself by value — directly, or through another schema's own +// required fields (e.g. two schemas requiring each other) — which doesn't +// compile. +func goRequiredValueReachability(schemas map[string]*spec.Schema) map[string]map[string]bool { + edges := make(map[string]map[string]bool, len(schemas)) + for name, schema := range schemas { + edges[name] = requiredValueEdges(schema.Properties, schema.Required, schemas) + } + + reaches := make(map[string]map[string]bool, len(edges)) + for name := range edges { + reaches[name] = reachableFrom(name, edges) + } + return reaches +} + +// requiredValueEdges returns the component schema names that required's +// struct-shaped fields (see isStructShapedProperty) reference by value, +// following nested inline structs but stopping at the first $ref — that's +// the next node in the graph, walked separately. It doesn't follow Items or +// AdditionalProperties: a slice or map already breaks value containment, so +// they can't be part of a value cycle. +func requiredValueEdges(props map[string]spec.Schema, required []string, schemas map[string]*spec.Schema) map[string]bool { + edges := map[string]bool{} + for _, name := range required { + prop, ok := props[name] + if !ok || !isStructShapedProperty(prop, schemas) { + continue + } + if target := schemaRefName(prop); target != "" { + edges[target] = true + continue + } + for target := range requiredValueEdges(prop.Properties, prop.Required, schemas) { + edges[target] = true + } + } + return edges +} + +// reachableFrom returns every node reachable from start by following edges, +// including start itself if some path cycles back to it. +func reachableFrom(start string, edges map[string]map[string]bool) map[string]bool { + seen := map[string]bool{} + queue := []string{start} + for len(queue) > 0 { + next := queue[0] + queue = queue[1:] + for target := range edges[next] { + if seen[target] { + continue + } + seen[target] = true + queue = append(queue, target) + } + } + return seen +} + // goReferenceK8sTypes converts all references to k8s meta/v1 schemas in the // given spec to references to the shared Go models we generate for the k8s // schemas. @@ -1010,19 +1245,9 @@ func goReferenceK8sType(schema *spec.Schema) { } func goReferenceK8sTypeWithMetaPath(schema *spec.Schema, useCorePath bool) { - // Helper function to check if a reference is a k8s type - isK8sRef := func(ref string) bool { - return strings.Contains(ref, k8sPkgMetaV1) || - strings.Contains(ref, k8sPkgCoreV1) || - strings.Contains(ref, k8sPkgRuntime) || - strings.Contains(ref, k8sPkgIntStr) || - strings.Contains(ref, k8sPkgResource) || - strings.Contains(ref, k8sPkgAutoscalingV1) - } - // Handle direct reference ref := schema.Ref.String() - if isK8sRef(ref) { + if isK8sSharedTypeRef(ref) { tryReplaceK8sTypeWithMetaPath(schema, ref, useCorePath) // Clear the original reference after replacement schema.Ref = spec.Ref{} @@ -1031,7 +1256,7 @@ func goReferenceK8sTypeWithMetaPath(schema *spec.Schema, useCorePath bool) { // Handle AllOf - if all schemas in AllOf are k8s refs, we can replace the whole schema allK8s := true for _, one := range schema.AllOf { - if one.Ref.String() == "" || !isK8sRef(one.Ref.String()) { + if one.Ref.String() == "" || !isK8sSharedTypeRef(one.Ref.String()) { allK8s = false break } @@ -1552,7 +1777,7 @@ func generateK8sPackageCode(pkg string, schemas map[string]*spec.Schema, schemaF goRenameTypes, goRenameEnums, goReplaceNumberWithInt, - goRemoveRequired, + g.requiredMutator(), goReferenceK8sTypes, goAddDefaults, ) @@ -1736,7 +1961,7 @@ func generateGVKGroupCode(gvkKey string, schemas map[string]*spec.Schema, openAP goRenameTypes, goRenameEnums, goReplaceNumberWithInt, - goRemoveRequired, + g.requiredMutator(), goReferenceK8sTypes, goRemoveK8s, goKeepOnlyComponents, diff --git a/internal/schemas/generator/go_test.go b/internal/schemas/generator/go_test.go index 358f84a1..b3020825 100644 --- a/internal/schemas/generator/go_test.go +++ b/internal/schemas/generator/go_test.go @@ -19,6 +19,7 @@ package generator import ( "embed" "go/ast" + "go/format" "go/parser" "go/token" "path/filepath" @@ -28,6 +29,8 @@ import ( "github.com/google/go-cmp/cmp" "github.com/spf13/afero" "golang.org/x/mod/modfile" + "k8s.io/kube-openapi/pkg/spec3" + "k8s.io/kube-openapi/pkg/validation/spec" ) //go:embed testdata/*.yaml @@ -118,6 +121,251 @@ func TestGenerateFromCRDGo(t *testing.T) { } } +// TestGenerateFromCRDGoRequiredObjectFields verifies the feature gate: only +// with it enabled does a required object-typed property (spec.parameters, +// mirroring a provider's spec.forProvider) generate as a non-pointer value +// with no `omitempty`, so its zero value marshals as `{}` and satisfies the +// CRD's required-key check. Disabled (the default) keeps this generator's +// all-pointer/all-optional convention instead — this is a breaking change for +// any consumer constructing such a field as a pointer or nil-checking it, so +// it must not apply without opting in. Either way, a sibling optional object +// (spec.compositionRef) and a required scalar (parameters.name) are +// unaffected. +func TestGenerateFromCRDGoRequiredObjectFields(t *testing.T) { + cases := map[string]struct { + args bool + want goStructField + reason string + }{ + "Enabled": { + args: true, + want: goStructField{typ: "AccountScaffoldSpecParameters", tag: `json:"parameters"`}, + reason: "a required object-typed property generates as a non-pointer value with no omitempty", + }, + "DisabledByDefault": { + args: false, + want: goStructField{typ: "*AccountScaffoldSpecParameters", tag: `json:"parameters,omitempty"`}, + reason: "with the feature off, the default pointer/omitempty shape is unchanged", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataFS}, "testdata") + schemaFS, err := goGenerator{requiredObjectFields: tc.args}.GenerateFromCRD(t.Context(), inputFS, nil) + if err != nil { + t.Fatal(err) + } + + contents, err := afero.ReadFile(schemaFS, "models/co/acme/platform/v1alpha1/accountscaffold.go") + if err != nil { + t.Fatal(err) + } + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "", contents, parser.ParseComments) + if err != nil { + t.Fatalf("failed to parse generated source: %v\n%s", err, contents) + } + + fields := goStructFields(t, f, "AccountScaffoldSpec") + if got := fields["Parameters"]; got != tc.want { + t.Errorf("AccountScaffoldSpec.Parameters = %+v, want %+v (%s)", got, tc.want, tc.reason) + } + if got := fields["CompositionRef"]; got.typ != "*AccountScaffoldSpecCompositionRef" || got.tag != `json:"compositionRef,omitempty"` { + t.Errorf("AccountScaffoldSpec.CompositionRef = %+v, want its pointer/omitempty shape unchanged", got) + } + + paramFields := goStructFields(t, f, "AccountScaffoldSpecParameters") + if got := paramFields["Name"]; got.typ != "*string" || got.tag != `json:"name,omitempty"` { + t.Errorf("AccountScaffoldSpecParameters.Name = %+v, want its pointer/omitempty shape unchanged even though it's required", got) + } + }) + } +} + +// TestIsStructShapedProperty covers the shapes filterRequiredObjectFields +// relies on to decide whether a required property may keep its +// required-ness: named properties (with or without also allowing additional +// properties, since oapi-codegen generates a struct either way), a map-only +// object, a $ref/single-element-allOf wrapper with no sibling properties that +// must be resolved against schemas before its shape can be judged (the form +// Kubernetes' own OpenAPI uses for a required nested object, e.g. +// DeviceClass.spec), a ref to a k8s API machinery type that generates in a +// separate package, and every other $ref/allOf shape — a multi-element +// allOf, or any $ref/allOf alongside sibling inline properties — which is +// excluded because oapi-codegen v2.8 merges those into an anonymous struct +// this generator's accessors/DeepCopy code can't recognize. +func TestIsStructShapedProperty(t *testing.T) { + schemas := map[string]*spec.Schema{ + "pkg.Struct": {SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{"name": {}}, + }}, + } + + cases := map[string]struct { + args spec.Schema + want bool + reason string + }{ + "NamedPropertiesOnly": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{"name": {}}, + }}, + want: true, + reason: "oapi-codegen generates a Go struct for named properties", + }, + "AdditionalPropertiesOnly": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + AdditionalProperties: &spec.SchemaOrBool{Allows: true}, + }}, + want: false, + reason: "a map-only object generates as a Go map, not a struct", + }, + "NamedPropertiesAndAdditionalProperties": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{"index": {}}, + AdditionalProperties: &spec.SchemaOrBool{Allows: true}, + }}, + want: true, + reason: "oapi-codegen still generates a struct, plus an AdditionalProperties map field, once named properties are present", + }, + "ScalarOrArray": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"string"}, + }}, + want: false, + reason: "a scalar has no named properties to become a struct field", + }, + "DirectRef": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Ref: spec.MustCreateRef("#/components/schemas/pkg.Struct"), + }}, + want: true, + reason: "a direct $ref must be resolved to what it points to", + }, + "SingleAllOfRef": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + AllOf: []spec.Schema{{SchemaProps: spec.SchemaProps{ + Ref: spec.MustCreateRef("#/components/schemas/pkg.Struct"), + }}}, + Default: map[string]any{}, + }}, + want: true, + reason: "Kubernetes wraps a required object ref in a single-element allOf to attach a description/default alongside it", + }, + "UnresolvedRef": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Ref: spec.MustCreateRef("#/components/schemas/DoesNotExist"), + }}, + want: false, + reason: "an unresolved reference must not be assumed struct-shaped", + }, + "K8sSharedTypeRef": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + AllOf: []spec.Schema{{SchemaProps: spec.SchemaProps{ + Ref: spec.MustCreateRef("#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.LabelSelector"), + }}}, + }}, + want: false, + reason: "a ref to a k8s API machinery type becomes a cross-package value; accessors and DeepCopy only special-case a locally declared struct, so it must stay a pointer", + }, + "MultiElementAllOf": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + AllOf: []spec.Schema{ + {SchemaProps: spec.SchemaProps{Ref: spec.MustCreateRef("#/components/schemas/pkg.Struct")}}, + {SchemaProps: spec.SchemaProps{Required: []string{"name"}}}, + }, + }}, + want: false, + reason: "oapi-codegen merges a multi-element allOf into an anonymous inline struct, not a reference to a named local type; this generator's accessors/DeepCopy code can't recognize that shape, so it must stay excluded (and thus a pointer) even though a member resolves to a struct", + }, + "MultiElementAllOfWithInlineProperties": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{"name": {}}, + AllOf: []spec.Schema{ + {SchemaProps: spec.SchemaProps{Required: []string{"name"}}}, + {SchemaProps: spec.SchemaProps{Required: []string{"other"}}}, + }, + }}, + want: false, + reason: "inline properties alongside any allOf, even validation-only members, merge into an anonymous inline struct, not a named local type", + }, + "SingleAllOfRefWithInlineProperties": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{"extra": {}}, + AllOf: []spec.Schema{ + {SchemaProps: spec.SchemaProps{Ref: spec.MustCreateRef("#/components/schemas/pkg.Struct")}}, + }, + }}, + want: false, + reason: "a single allOf $ref member alongside sibling inline properties also merges into an anonymous inline struct, unlike the same $ref with no sibling properties", + }, + "DirectRefWithInlineProperties": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Ref: spec.MustCreateRef("#/components/schemas/pkg.Struct"), + Properties: map[string]spec.Schema{"extra": {}}, + }}, + want: false, + reason: "a direct $ref alongside sibling inline properties is excluded conservatively, matching the allOf-with-siblings case", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + if got := isStructShapedProperty(tc.args, schemas); got != tc.want { + t.Errorf("isStructShapedProperty() = %v, want %v (%s)", got, tc.want, tc.reason) + } + }) + } +} + +// goStructField is a struct field's rendered type and raw tag text. +type goStructField struct { + typ string + tag string +} + +// goStructFields returns typeName's fields by name, for asserting on their +// exact type and tag. +func goStructFields(t *testing.T, f *ast.File, typeName string) map[string]goStructField { + t.Helper() + fset := token.NewFileSet() + for _, decl := range f.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok || gen.Tok != token.TYPE { + continue + } + for _, spec := range gen.Specs { + ts, ok := spec.(*ast.TypeSpec) + if !ok || ts.Name.Name != typeName { + continue + } + st, ok := ts.Type.(*ast.StructType) + if !ok { + continue + } + out := map[string]goStructField{} + for _, field := range st.Fields.List { + var typ strings.Builder + if err := format.Node(&typ, fset, field.Type); err != nil { + t.Fatalf("failed to render field type: %v", err) + } + tag := "" + if field.Tag != nil { + tag = strings.Trim(field.Tag.Value, "`") + } + for _, name := range field.Names { + out[name.Name] = goStructField{typ: typ.String(), tag: tag} + } + } + return out + } + } + t.Fatalf("type %s not found in generated source", typeName) + return nil +} + // TestGenerateFromCRDGoScaleSubresource ensures CRDs with a scale subresource // generate a model for the resource itself and don't pull the autoscaling/v1 // Scale schemas into the generated models. crd.ToOpenAPI drops the scale @@ -307,3 +555,342 @@ func TestGenerateFromOpenAPIGo(t *testing.T) { } } } + +// TestGenerateFromOpenAPIGoRequiredObjectFields verifies the OpenAPI +// generation path resolves a required property wrapped in a single-element +// allOf $ref — the shape Kubernetes' own built-in OpenAPI uses for a required +// nested object — before judging whether it's struct-shaped. DeviceClass.spec +// in resource.k8s.io/v1 is `{allOf: [{$ref: ".../DeviceClassSpec"}]}` with no +// inline properties of its own, so this only passes if the ref is followed. +func TestGenerateFromOpenAPIGoRequiredObjectFields(t *testing.T) { + cases := map[string]struct { + args bool + want goStructField + reason string + }{ + "Enabled": { + args: true, + want: goStructField{typ: "IoK8SApiResourceV1DeviceClassSpec", tag: `json:"spec"`}, + reason: "a required property whose allOf ref resolves to a struct must generate as a non-pointer value", + }, + "DisabledByDefault": { + args: false, + want: goStructField{typ: "*IoK8SApiResourceV1DeviceClassSpec", tag: `json:"spec,omitempty"`}, + reason: "with the feature off, the default pointer/omitempty shape is unchanged", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataJSONFS}, "testdata") + schemaFS, err := goGenerator{requiredObjectFields: tc.args}.GenerateFromOpenAPI(t.Context(), inputFS, nil) + if err != nil { + t.Fatal(err) + } + + contents, err := afero.ReadFile(schemaFS, "models/io/k8s/api/resource/v1/resource.go") + if err != nil { + t.Fatal(err) + } + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "", contents, parser.ParseComments) + if err != nil { + t.Fatalf("failed to parse generated source: %v\n%s", err, contents) + } + + fields := goStructFields(t, f, "DeviceClass") + if got := fields["Spec"]; got != tc.want { + t.Errorf("DeviceClass.Spec = %+v, want %+v (%s)", got, tc.want, tc.reason) + } + }) + } +} + +// TestGoRemoveRequiredAllOfShapes is the generator-level gate for +// isStructShapedProperty's $ref/allOf/additionalProperties handling: it runs +// a required property named "bar" through the real GVK-group generation +// pipeline (generateGVKGroupCode's mutator list) and asserts on the actual +// generated field, rather than only on the isStructShapedProperty predicate +// in isolation, since several cases depend on oapi-codegen v2.8's +// allOf-merge behavior (see isStructShapedProperty's doc comment). +func TestGoRemoveRequiredAllOfShapes(t *testing.T) { + cases := map[string]struct { + args spec.Schema // the "bar" property's schema + want bool // whether Foo.Bar should generate non-pointer (struct-shaped) + reason string + }{ + "NamedPropertiesOnly": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{"name": {SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{"string"}}}}, + }}, + want: true, + reason: "plain named properties, no ref or allOf, generate as a non-pointer named local struct", + }, + "NamedPropertiesAndAdditionalProperties": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{"name": {SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{"string"}}}}, + AdditionalProperties: &spec.SchemaOrBool{Allows: true}, + }}, + want: true, + reason: "oapi-codegen still generates a non-pointer named struct (plus an AdditionalProperties map field) when named properties are present", + }, + "SingleAllOfRefNoSiblingProperties": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + AllOf: []spec.Schema{{SchemaProps: spec.SchemaProps{Ref: spec.MustCreateRef("#/components/schemas/pkg.Local")}}}, + }}, + want: true, + reason: "a lone allOf $ref with no sibling properties resolves to the named referenced type", + }, + "SingleAllOfRefWithSiblingProperties": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{"name": {SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{"string"}}}}, + AllOf: []spec.Schema{{SchemaProps: spec.SchemaProps{Ref: spec.MustCreateRef("#/components/schemas/pkg.Local")}}}, + }}, + want: false, + reason: "a single allOf $ref alongside sibling properties merges into an anonymous struct, so it must stay a pointer", + }, + "TwoValidationOnlyAllOfMembersWithSiblingProperties": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{"name": {SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{"string"}}}}, + AllOf: []spec.Schema{ + {SchemaProps: spec.SchemaProps{Required: []string{"x"}}}, + {SchemaProps: spec.SchemaProps{Required: []string{"y"}}}, + }, + }}, + want: false, + reason: "validation-only allOf members alongside sibling properties also merge into an anonymous struct", + }, + "MultiElementAllOfNoSiblingProperties": { + args: spec.Schema{SchemaProps: spec.SchemaProps{ + AllOf: []spec.Schema{ + {SchemaProps: spec.SchemaProps{Ref: spec.MustCreateRef("#/components/schemas/pkg.Local")}}, + {SchemaProps: spec.SchemaProps{Required: []string{"y"}}}, + }, + }}, + want: false, + reason: "a multi-element allOf with no sibling properties also merges into an anonymous struct", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + s := &spec3.OpenAPI{ + Version: "3.0.0", + Components: &spec3.Components{ + Schemas: map[string]*spec.Schema{ + "pkg.Local": {SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"object"}, + Properties: map[string]spec.Schema{ + "name": {SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{"string"}}}, + }, + }}, + "pkg.Foo": {SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"object"}, + Required: []string{"bar"}, + Properties: map[string]spec.Schema{"bar": tc.args}, + }}, + }, + }, + } + + code, err := generateGo(s, "v1", + goRemoveValidationOnlyCombinators, + goRenameTypes, + goRenameEnums, + goReplaceNumberWithInt, + goGenerator{requiredObjectFields: true}.requiredMutator(), + goReferenceK8sTypes, + goRemoveK8s, + goKeepOnlyComponents, + goAddDefaults, + ) + if err != nil { + t.Fatalf("generateGo: %v", err) + } + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "", code, parser.ParseComments) + if err != nil { + t.Fatalf("failed to parse generated source: %v\n%s", err, code) + } + got := goStructFields(t, f, "Foo")["Bar"] + + isNonPointerNamedStruct := !strings.HasPrefix(got.typ, "*") && !strings.Contains(got.typ, "struct") && got.tag == `json:"bar"` + if isNonPointerNamedStruct != tc.want { + t.Errorf("Foo.Bar = %+v, want non-pointer named struct = %v (%s)", got, tc.want, tc.reason) + } + if !tc.want && (!strings.HasPrefix(got.typ, "*") || got.tag != `json:"bar,omitempty"`) { + t.Errorf("Foo.Bar = %+v, want its default pointer/omitempty shape (%s)", got, tc.reason) + } + }) + } +} + +// TestGoRemoveRequiredValueCycles is the generator-level gate for +// goRequiredValueReachability: a required, struct-shaped $ref field that +// would make oapi-codegen emit a Go struct containing itself by value — +// directly, through another schema requiring it back, or through a nested +// inline struct — must stay a pointer instead, since Go rejects an +// infinitely-sized value type. +func TestGoRemoveRequiredValueCycles(t *testing.T) { + cases := map[string]struct { + schemas map[string]*spec.Schema + want map[string]map[string]bool // type name -> field name -> want non-pointer + reason string + }{ + "DirectSelfReference": { + schemas: map[string]*spec.Schema{ + "pkg.Node": {SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"object"}, + Required: []string{"next"}, + Properties: map[string]spec.Schema{ + "next": {SchemaProps: spec.SchemaProps{Ref: spec.MustCreateRef("#/components/schemas/pkg.Node")}}, + }, + }}, + }, + want: map[string]map[string]bool{"Node": {"Next": false}}, + reason: "Node.Next required non-pointer would make Node contain itself by value", + }, + "MutualReference": { + schemas: map[string]*spec.Schema{ + "pkg.A": {SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"object"}, + Required: []string{"b"}, + Properties: map[string]spec.Schema{ + "b": {SchemaProps: spec.SchemaProps{Ref: spec.MustCreateRef("#/components/schemas/pkg.B")}}, + }, + }}, + "pkg.B": {SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"object"}, + Required: []string{"a"}, + Properties: map[string]spec.Schema{ + "a": {SchemaProps: spec.SchemaProps{Ref: spec.MustCreateRef("#/components/schemas/pkg.A")}}, + }, + }}, + }, + want: map[string]map[string]bool{"A": {"B": false}, "B": {"A": false}}, + reason: "A requiring B by value and B requiring A by value would still make each contain itself by value, transitively; both directions must stay pointers, not just one", + }, + "NestedInlineToRoot": { + schemas: map[string]*spec.Schema{ + "pkg.Root": {SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"object"}, + Required: []string{"meta"}, + Properties: map[string]spec.Schema{ + "meta": {SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"object"}, + Required: []string{"owner"}, + Properties: map[string]spec.Schema{ + "owner": {SchemaProps: spec.SchemaProps{Ref: spec.MustCreateRef("#/components/schemas/pkg.Root")}}, + }, + }}, + }, + }}, + }, + want: map[string]map[string]bool{ + "Root": {"Meta": true}, // the inline struct itself isn't a $ref, so it's unaffected + "RootMeta": {"Owner": false}, // but its own required field pointing back to Root must stay a pointer + }, + reason: "Meta is embedded in Root by value, so a required field inside Meta pointing back to Root would still make Root contain itself by value", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + s := &spec3.OpenAPI{ + Version: "3.0.0", + Components: &spec3.Components{Schemas: tc.schemas}, + } + + code, err := generateGo(s, "v1", + goRemoveValidationOnlyCombinators, + goRenameTypes, + goRenameEnums, + goReplaceNumberWithInt, + goGenerator{requiredObjectFields: true}.requiredMutator(), + goReferenceK8sTypes, + goRemoveK8s, + goKeepOnlyComponents, + goAddDefaults, + ) + if err != nil { + t.Fatalf("generateGo: %v", err) + } + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "", code, parser.ParseComments) + if err != nil { + t.Fatalf("failed to parse generated source: %v\n%s", err, code) + } + + for typeName, fields := range tc.want { + got := goStructFields(t, f, typeName) + for fieldName, wantNonPointer := range fields { + field, ok := got[fieldName] + if !ok { + t.Fatalf("%s.%s not found in generated source (%s):\n%s", typeName, fieldName, tc.reason, code) + } + isNonPointer := !strings.HasPrefix(field.typ, "*") + if isNonPointer != wantNonPointer { + t.Errorf("%s.%s = %+v, want non-pointer = %v (%s)", typeName, fieldName, field, wantNonPointer, tc.reason) + } + } + } + }) + } +} + +// TestGenerateFromOpenAPIGoRequiredObjectFieldsAlias is the generator-level +// gate for collectStructTypes' alias resolution: DeviceClass.spec resolves +// (via TestGenerateFromOpenAPIGoRequiredObjectFields) to +// IoK8SApiResourceV1DeviceClassSpec, which oapi-codegen declares as a true Go +// alias (`type IoK8SApiResourceV1DeviceClassSpec = DeviceClassSpec`), not the +// struct itself — the real struct is separately declared as DeviceClassSpec. +// Without resolving that alias, writeFieldCopy and addAccessors wouldn't +// recognize the field as a locally declared struct at all. +func TestGenerateFromOpenAPIGoRequiredObjectFieldsAlias(t *testing.T) { + inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataJSONFS}, "testdata") + schemaFS, err := goGenerator{requiredObjectFields: true, runtimeObjects: true, accessors: true}.GenerateFromOpenAPI(t.Context(), inputFS, nil) + if err != nil { + t.Fatal(err) + } + + contents, err := afero.ReadFile(schemaFS, "models/io/k8s/api/resource/v1/resource.go") + if err != nil { + t.Fatal(err) + } + code := string(contents) + + // This test's premise is that DeviceClass.spec's resolved type is a true + // Go alias of the real struct, not the struct itself. If that's no + // longer true (e.g. a future oapi-codegen version stops emitting the + // alias), the cases below would pass vacuously, so pin the premise first. + if !strings.Contains(code, "type IoK8SApiResourceV1DeviceClassSpec = DeviceClassSpec") { + t.Fatalf("expected DeviceClass.spec's resolved type to be a Go alias of the real struct; generated source no longer matches this test's premise:\n%s", code) + } + + cases := map[string]struct { + want []string + reason string + }{ + "DeepCopyThroughAlias": { + want: []string{"in.Spec.DeepCopyInto(&out.Spec)"}, + reason: "DeviceClass.DeepCopyInto must call in.Spec.DeepCopyInto(&out.Spec) through the alias, not a shallow copy", + }, + "AccessorThroughAlias": { + want: []string{"func (o *DeviceClass) GetSpec() *IoK8SApiResourceV1DeviceClassSpec {"}, + reason: "DeviceClass.GetSpec must return a pointer to the aliased type, matching every other field's chainable getter shape", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + for _, want := range tc.want { + if !strings.Contains(code, want) { + t.Errorf("expected generated source to contain %q (%s)", want, tc.reason) + } + } + }) + } +} diff --git a/internal/schemas/generator/interface.go b/internal/schemas/generator/interface.go index d26519d2..650d4d3a 100644 --- a/internal/schemas/generator/interface.go +++ b/internal/schemas/generator/interface.go @@ -36,8 +36,9 @@ type Interface interface { // options holds configurable behavior shared across generators. type options struct { - goModelAccessors bool - goRuntimeObjects bool + goModelAccessors bool + goRuntimeObjects bool + goRequiredObjectFields bool } // Option configures the generators returned by AllLanguages. @@ -58,6 +59,16 @@ func WithGoRuntimeObjects(enabled bool) Option { return func(o *options) { o.goRuntimeObjects = enabled } } +// WithGoRequiredObjectFields enables generating a required object-typed +// property (e.g. a managed resource's spec.forProvider) as a non-pointer +// value instead of this generator's default all-pointer/all-optional +// convention. Disabled by default, since it's a breaking change for any +// consumer constructing such a field as a pointer or nil-checking it; gated +// behind the features.generateGoRequiredObjectFields config flag. +func WithGoRequiredObjectFields(enabled bool) Option { + return func(o *options) { o.goRequiredObjectFields = enabled } +} + // AllLanguages returns generators for all supported languages. The set of // supported language identifiers is defined by // devv1alpha1.SupportedSchemaLanguages. @@ -67,7 +78,11 @@ func AllLanguages(opts ...Option) []Interface { opt(o) } return []Interface{ - &goGenerator{accessors: o.goModelAccessors, runtimeObjects: o.goRuntimeObjects}, + &goGenerator{ + accessors: o.goModelAccessors, + runtimeObjects: o.goRuntimeObjects, + requiredObjectFields: o.goRequiredObjectFields, + }, &jsonGenerator{}, &kclGenerator{}, &pythonGenerator{}, diff --git a/internal/schemas/generator/interface_test.go b/internal/schemas/generator/interface_test.go index e48a353d..54b96392 100644 --- a/internal/schemas/generator/interface_test.go +++ b/internal/schemas/generator/interface_test.go @@ -43,17 +43,18 @@ func TestAllLanguagesGoOptions(t *testing.T) { t.Parallel() cases := map[string]struct { - reason string - opts []Option - wantRuntimeObjects bool - wantAccessors bool + reason string + opts []Option + wantRuntimeObjects bool + wantAccessors bool + wantRequiredObjectFields bool }{ "OffByDefault": { - reason: "both Go generator features are opt-in", + reason: "all three Go generator features are opt-in", }, "Disabled": { reason: "explicitly disabled flags leave the Go generator alone", - opts: []Option{WithGoRuntimeObjects(false), WithGoModelAccessors(false)}, + opts: []Option{WithGoRuntimeObjects(false), WithGoModelAccessors(false), WithGoRequiredObjectFields(false)}, }, "RuntimeObjectsOnly": { reason: "the option reaches the Go generator, which is what emits the code", @@ -61,15 +62,21 @@ func TestAllLanguagesGoOptions(t *testing.T) { wantRuntimeObjects: true, }, "AccessorsOnly": { - reason: "the two options are independent", + reason: "the options are independent", opts: []Option{WithGoModelAccessors(true)}, wantAccessors: true, }, - "Both": { - reason: "neither option clobbers the other", - opts: []Option{WithGoModelAccessors(true), WithGoRuntimeObjects(true)}, - wantRuntimeObjects: true, - wantAccessors: true, + "RequiredObjectFieldsOnly": { + reason: "the option reaches the Go generator, which is what emits the code", + opts: []Option{WithGoRequiredObjectFields(true)}, + wantRequiredObjectFields: true, + }, + "All": { + reason: "no option clobbers another", + opts: []Option{WithGoModelAccessors(true), WithGoRuntimeObjects(true), WithGoRequiredObjectFields(true)}, + wantRuntimeObjects: true, + wantAccessors: true, + wantRequiredObjectFields: true, }, } @@ -92,6 +99,9 @@ func TestAllLanguagesGoOptions(t *testing.T) { if got.accessors != tc.wantAccessors { t.Errorf("accessors = %v, want %v (%s)", got.accessors, tc.wantAccessors, tc.reason) } + if got.requiredObjectFields != tc.wantRequiredObjectFields { + t.Errorf("requiredObjectFields = %v, want %v (%s)", got.requiredObjectFields, tc.wantRequiredObjectFields, tc.reason) + } }) } } diff --git a/internal/schemas/generator/runtimeobject.go b/internal/schemas/generator/runtimeobject.go index 1acf5c6e..9ca39872 100644 --- a/internal/schemas/generator/runtimeobject.go +++ b/internal/schemas/generator/runtimeobject.go @@ -125,9 +125,16 @@ func addRuntimeObjects(code string) (string, bool, error) { return string(formatted), hasRoots, nil } -// collectStructTypes returns the set of struct type names declared in the file. +// collectStructTypes returns the set of names that are, or alias, a struct +// type declared in the file. oapi-codegen emits `type ComponentName = GoName` +// for a schema whose x-go-type-name overrides its default derived name (see +// goRenameSchemaType in go.go) — e.g. a $ref field is typed by the component +// name, which is a true Go alias (fully interchangeable, same method set) for +// the actual struct. Without following that alias, a field using the +// component name wouldn't be recognized as the local struct it actually is. func collectStructTypes(f *ast.File) map[string]bool { out := map[string]bool{} + aliasOf := map[string]string{} for _, decl := range f.Decls { gen, ok := decl.(*ast.GenDecl) if !ok || gen.Tok != token.TYPE { @@ -135,7 +142,13 @@ func collectStructTypes(f *ast.File) map[string]bool { } for _, spec := range gen.Specs { ts, ok := spec.(*ast.TypeSpec) - if !ok || ts.Assign.IsValid() { + if !ok { + continue + } + if ts.Assign.IsValid() { + if id, ok := ts.Type.(*ast.Ident); ok { + aliasOf[ts.Name.Name] = id.Name + } continue } if _, ok := ts.Type.(*ast.StructType); ok { @@ -143,9 +156,35 @@ func collectStructTypes(f *ast.File) map[string]bool { } } } + markStructAliases(out, aliasOf) return out } +// markStructAliases adds name to out for every name in aliasOf whose alias +// chain (following `type A = B` links) terminates at a name already in out. +func markStructAliases(out map[string]bool, aliasOf map[string]string) { + for name := range aliasOf { + if !resolvesToStruct(name, aliasOf, out, map[string]bool{}) { + continue + } + out[name] = true + } +} + +// resolvesToStruct follows cur's alias chain in aliasOf and reports whether +// it terminates at a name already in out. seen guards against a cycle. +func resolvesToStruct(cur string, aliasOf map[string]string, out, seen map[string]bool) bool { + if seen[cur] { + return false + } + seen[cur] = true + target, isAlias := aliasOf[cur] + if !isAlias { + return out[cur] + } + return resolvesToStruct(target, aliasOf, out, seen) +} + // collectCollectionAliases returns local named types whose underlying type is a // map or slice (both `type X map[..]` and `type X = map[..]`). Fields of these // types must be deep-copied like a literal map/slice, not shallow-copied as a @@ -202,6 +241,17 @@ const ( fkStruct ) +// isJSONRawMessage reports whether typ is exactly json.RawMessage — the +// unexported backing field oapi-codegen generates for a oneOf/anyOf union. +func isJSONRawMessage(typ ast.Expr) bool { + sel, ok := typ.(*ast.SelectorExpr) + if !ok { + return false + } + pkg, ok := sel.X.(*ast.Ident) + return ok && pkg.Name == "json" && sel.Sel.Name == "RawMessage" +} + // classifyElem classifies the element type expr (the type with any leading // pointer/slice/map already stripped) as scalar or struct. func classifyElem(e ast.Expr, structs map[string]bool) fieldKind { @@ -250,15 +300,37 @@ func writeDeepCopy(b *strings.Builder, fset *token.FileSet, name string, st *ast b.WriteString("\tin.DeepCopyInto(out)\n\treturn out\n}\n") } -// writeFieldCopy appends the deep-copy snippet for a single field. All generated -// fields are pointers; the leading pointer is handled here, then the pointee -// (scalar, struct, slice or map) is copied appropriately. Named aliases to a -// map or slice are deep-copied like their literal form. +// writeFieldCopy appends the deep-copy snippet for a single field. Most +// generated fields are pointers; the leading pointer is handled here, then +// the pointee (scalar, struct, slice or map) is copied appropriately. Named +// aliases to a map or slice are deep-copied like their literal form. +// +// A required object-typed field (see goRemoveRequired) is a non-pointer +// struct value instead. The top-level `*out = *in` in writeDeepCopy shallow- +// copies it, aliasing any pointer fields nested inside it, so it needs its +// own DeepCopyInto call. func writeFieldCopy(b *strings.Builder, fset *token.FileSet, field string, typ ast.Expr, structs map[string]bool, aliases map[string]ast.Expr) { star, ok := typ.(*ast.StarExpr) if !ok { - // Non-pointer fields are copied by the `*out = *in` shallow assignment. - // Generated models use pointers throughout, but guard defensively. + // A required object-typed field (a bare reference to a known local + // struct, see goRemoveRequired) needs DeepCopyInto here. + if id, ok := typ.(*ast.Ident); ok && structs[id.Name] { + fmt.Fprintf(b, "\tin.%s.DeepCopyInto(&out.%s)\n", field, field) + return + } + // oapi-codegen's unexported `union json.RawMessage` field (its + // oneOf/anyOf plumbing) needs its bytes copied explicitly: its + // MarshalJSON exposes the backing slice directly, so the shallow + // struct assignment would let mutating the copy's raw JSON bytes + // corrupt the original. Every other non-pointer, non-struct field + // (scalars, named string/int aliases) is already correctly copied by + // the shallow assignment. + if isJSONRawMessage(typ) { + fmt.Fprintf(b, "\tif in.%s != nil {\n", field) + fmt.Fprintf(b, "\t\tout.%s = make(json.RawMessage, len(in.%s))\n", field, field) + fmt.Fprintf(b, "\t\tcopy(out.%s, in.%s)\n", field, field) + b.WriteString("\t}\n") + } return } diff --git a/internal/schemas/generator/runtimeobject_compilegate_test.go b/internal/schemas/generator/runtimeobject_compilegate_test.go index 954d2ef2..782c280b 100644 --- a/internal/schemas/generator/runtimeobject_compilegate_test.go +++ b/internal/schemas/generator/runtimeobject_compilegate_test.go @@ -82,24 +82,61 @@ func resolveGeneratedModuleDeps(t *testing.T, modelsDir string) { } } -// TestGeneratedRuntimeObjectsCompile materializes the generated module (flag on), -// adds a consumer that registers the types in a runtime.Scheme and exercises an -// accessor through the runtime.Object interface, and compiles the whole module. +// TestGeneratedRuntimeObjectsCompile materializes the generated module with +// runtimeObjects on, adds a consumer that registers the types in a +// runtime.Scheme and exercises an accessor through the runtime.Object +// interface, and compiles the whole module. It's a table over +// requiredObjectFields, not just the flag-on case: runtimeObjects on with +// requiredObjectFields off (the default for every existing caller of +// WithGoRuntimeObjects) needs its own compile/behavior gate too. func TestGeneratedRuntimeObjectsCompile(t *testing.T) { - inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataFS}, "testdata") - schemaFS, err := goGenerator{runtimeObjects: true}.GenerateFromCRD(t.Context(), inputFS, nil) - if err != nil { - t.Fatal(err) + cases := map[string]struct { + args bool + want string // extra behavioral check appended inside the consumer's TestGeneratedRuntimeObject + reason string + }{ + "Enabled": { + args: true, + // Required object-typed field independence: with the feature + // on, Parameters is a non-pointer local struct (see + // goRemoveRequired), which exercises writeFieldCopy's + // DeepCopyInto branch rather than the top-level shallow struct + // copy. + want: ` + withParams := &v1alpha1.XAccountScaffoldSpec{ + Parameters: v1alpha1.XAccountScaffoldSpecParameters{Name: ptr("a")}, + } + paramsCopy := withParams.DeepCopy() + *paramsCopy.Parameters.Name = "b" + if *withParams.Parameters.Name != "a" { + t.Fatalf("DeepCopy (non-pointer struct) not independent: original mutated to %q", *withParams.Parameters.Name) + } +`, + reason: "a required object-typed field is a non-pointer local struct and must still DeepCopy independently", + }, + "Disabled": { + args: false, + reason: "runtimeObjects on with requiredObjectFields off (today's default for every WithGoRuntimeObjects caller) must still compile and DeepCopy correctly", + }, } - dir := t.TempDir() - roMaterialize(t, schemaFS, dir) + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataFS}, "testdata") + schemaFS, err := goGenerator{runtimeObjects: true, requiredObjectFields: tc.args}.GenerateFromCRD(t.Context(), inputFS, nil) + if err != nil { + t.Fatal(err) + } + + dir := t.TempDir() + roMaterialize(t, schemaFS, dir) - // A behavioral test inside the generated module: it compiles the whole - // module (build gate) and asserts runtime.Object satisfaction, AddToScheme - // GVK round-tripping, SetGroupVersionKind writing the typed fields, and - // DeepCopy independence. - consumer := `package consumer + // A behavioral test inside the generated module: it compiles the + // whole module (build gate) and asserts runtime.Object + // satisfaction, AddToScheme GVK round-tripping, + // SetGroupVersionKind writing the typed fields, and DeepCopy + // independence. + consumer := `package consumer import ( "testing" @@ -163,49 +200,108 @@ func TestGeneratedRuntimeObject(t *testing.T) { if (*sel.MatchLabels)["k"] != "v" { t.Fatalf("DeepCopy (*map) not independent: original mutated to %q", (*sel.MatchLabels)["k"]) } -} +` + tc.want + `} ` - consumerDir := filepath.Join(dir, "models", "consumer") - if err := os.MkdirAll(consumerDir, 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(consumerDir, "consumer_test.go"), []byte(consumer), 0o644); err != nil { - t.Fatal(err) - } + consumerDir := filepath.Join(dir, "models", "consumer") + if err := os.MkdirAll(consumerDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(consumerDir, "consumer_test.go"), []byte(consumer), 0o644); err != nil { + t.Fatal(err) + } - modelsDir := filepath.Join(dir, "models") + modelsDir := filepath.Join(dir, "models") - resolveGeneratedModuleDeps(t, modelsDir) + resolveGeneratedModuleDeps(t, modelsDir) - // `go test ./...` builds every generated package and runs the behavioral - // test above. - cmd := exec.CommandContext(t.Context(), "go", "test", "./...") - cmd.Dir = modelsDir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("generated runtime.Object models failed to build/test: %v\n%s", err, out) + // `go test ./...` builds every generated package and runs the + // behavioral test above. + cmd := exec.CommandContext(t.Context(), "go", "test", "./...") + cmd.Dir = modelsDir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("generated runtime.Object models failed to build/test (%s): %v\n%s", tc.reason, err, out) + } + }) } } -// TestGenerateFromOpenAPIRuntimeObjectsCompile exercises the OpenAPI generation -// path (the shared k8s and GVK packages, which include union and intstr types) -// with the feature on, compiles the result, and registers every generated -// built-in package in one scheme. +// TestGenerateFromOpenAPIRuntimeObjectsCompile exercises the OpenAPI +// generation path (the shared k8s and GVK packages, which include union and +// intstr types) with runtimeObjects on, compiles the result, and registers +// every generated built-in package in one scheme. It's a table over +// requiredObjectFields: with it on, this is the real Kubernetes built-in +// spec, so it's the gate that compiles a required object-typed field wrapped +// in a single-element allOf $ref (e.g. DeviceClass.spec in +// resource.k8s.io/v1) end to end, resolved through the type alias +// collectStructTypes now follows (see TestGenerateFromOpenAPIGoRequiredObjectFieldsAlias), +// and a required field that resolves to a shared k8s type (e.g. a +// LabelSelector), which must stay a pointer rather than become a +// cross-package non-pointer value. With it off (the default for every +// existing WithGoRuntimeObjects caller), DeviceClass.spec's DeepCopy must +// still be independent through that same alias — a pre-existing shallow-copy +// gap this PR also fixes, unconditionally. func TestGenerateFromOpenAPIRuntimeObjectsCompile(t *testing.T) { - inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataJSONFS}, "testdata") - schemaFS, err := goGenerator{runtimeObjects: true}.GenerateFromOpenAPI(t.Context(), inputFS, nil) - if err != nil { - t.Fatal(err) + // TestDeviceClassSpecDeepCopyIndependence's body differs by shape (each + // case's want): with requiredObjectFields on, Spec is a non-pointer + // value (constructed directly); off, it's a pointer (constructed with &). + cases := map[string]struct { + args bool + want string + reason string + }{ + "Enabled": { + args: true, + want: ` + name := "gpu" + orig := &resourcev1.DeviceClass{ + Spec: resourcev1.IoK8SApiResourceV1DeviceClassSpec{ + ExtendedResourceName: &name, + }, + } + cp := orig.DeepCopy() + *cp.Spec.ExtendedResourceName = "mutated" + if *orig.Spec.ExtendedResourceName != "gpu" { + t.Fatalf("DeepCopy of a required object-typed field resolved through a type alias was not independent: original mutated to %q", *orig.Spec.ExtendedResourceName) + } +`, + reason: "DeviceClass.spec is a required object-typed field resolved through a type alias to a non-pointer struct", + }, + "Disabled": { + args: false, + want: ` + name := "gpu" + orig := &resourcev1.DeviceClass{ + Spec: &resourcev1.IoK8SApiResourceV1DeviceClassSpec{ + ExtendedResourceName: &name, + }, + } + cp := orig.DeepCopy() + *cp.Spec.ExtendedResourceName = "mutated" + if *orig.Spec.ExtendedResourceName != "gpu" { + t.Fatalf("DeepCopy of an optional pointer field resolved through a type alias was not independent: original mutated to %q", *orig.Spec.ExtendedResourceName) + } +`, + reason: "runtimeObjects on with requiredObjectFields off (today's default) must still DeepCopy an optional field resolved through the same type alias independently", + }, } - dir := t.TempDir() - roMaterialize(t, schemaFS, dir) + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataJSONFS}, "testdata") + schemaFS, err := goGenerator{runtimeObjects: true, requiredObjectFields: tc.args}.GenerateFromOpenAPI(t.Context(), inputFS, nil) + if err != nil { + t.Fatal(err) + } + + dir := t.TempDir() + roMaterialize(t, schemaFS, dir) - // Registering every built-in package in a single scheme is the check that - // the API groups we write into groupversion_info.go are right: - // AddKnownTypes panics if two Go types claim the same GVK, which is what - // would happen if two packages sharing the core (empty) group also shared a - // kind name. - consumer := `package consumer + // Registering every built-in package in a single scheme is the + // check that the API groups we write into groupversion_info.go + // are right: AddKnownTypes panics if two Go types claim the same + // GVK, which is what would happen if two packages sharing the + // core (empty) group also shared a kind name. + consumer := `package consumer import ( "testing" @@ -218,8 +314,16 @@ import ( metav1 "dev.crossplane.io/models/io/k8s/core/meta/v1" corev1 "dev.crossplane.io/models/io/k8s/core/v1" policyv1 "dev.crossplane.io/models/io/k8s/policy/v1" + resourcev1 "dev.crossplane.io/models/io/k8s/api/resource/v1" ) +// TestDeviceClassSpecDeepCopyIndependence covers DeviceClass.spec, which +// resolves to IoK8SApiResourceV1DeviceClassSpec — a true Go alias of the +// real DeviceClassSpec struct, not the struct itself. Without +// collectStructTypes resolving that alias, DeepCopyInto falls back to the +// top-level shallow struct copy and this test fails. +func TestDeviceClassSpecDeepCopyIndependence(t *testing.T) {` + tc.want + `} + func TestBuiltInGroupVersions(t *testing.T) { s := runtime.NewScheme() for _, add := range []func(*runtime.Scheme) error{ @@ -258,22 +362,24 @@ func TestBuiltInGroupVersions(t *testing.T) { } } ` - consumerDir := filepath.Join(dir, "models", "consumer") - if err := os.MkdirAll(consumerDir, 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(consumerDir, "consumer_test.go"), []byte(consumer), 0o644); err != nil { - t.Fatal(err) - } + consumerDir := filepath.Join(dir, "models", "consumer") + if err := os.MkdirAll(consumerDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(consumerDir, "consumer_test.go"), []byte(consumer), 0o644); err != nil { + t.Fatal(err) + } - modelsDir := filepath.Join(dir, "models") + modelsDir := filepath.Join(dir, "models") - resolveGeneratedModuleDeps(t, modelsDir) + resolveGeneratedModuleDeps(t, modelsDir) - cmd := exec.CommandContext(t.Context(), "go", "test", "./...") - cmd.Dir = modelsDir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("generated OpenAPI runtime.Object models failed to build/test: %v\n%s", err, out) + cmd := exec.CommandContext(t.Context(), "go", "test", "./...") + cmd.Dir = modelsDir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("generated OpenAPI runtime.Object models failed to build/test (%s): %v\n%s", tc.reason, err, out) + } + }) } } @@ -301,48 +407,114 @@ func TestGeneratedModelsCompileWithoutRuntimeObjects(t *testing.T) { } // TestGeneratedModelsCompileWithAccessorsAndRuntimeObjects builds the output -// with both generator features on. They emit methods onto the same structs, so -// a name they both claim — GetObjectKind against a field named objectKind, say — -// would be a duplicate method that only a real build catches. Neither feature's -// own gate covers the combination. +// with accessors and runtimeObjects on, as a table over requiredObjectFields. +// The two features emit methods onto the same structs, so a name they both +// claim — GetObjectKind against a field named objectKind, say — would be a +// duplicate method that only a real build catches. Neither feature's own +// gate covers the combination, and requiredObjectFields off (today's +// default) needs its own build gate for the combination too. func TestGeneratedModelsCompileWithAccessorsAndRuntimeObjects(t *testing.T) { - inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataFS}, "testdata") - schemaFS, err := goGenerator{accessors: true, runtimeObjects: true}.GenerateFromCRD(t.Context(), inputFS, nil) - if err != nil { - t.Fatal(err) + cases := map[string]struct { + args bool + reason string + }{ + "Enabled": {args: true, reason: "all three features on must not produce duplicate methods"}, + "Disabled": {args: false, reason: "accessors+runtimeObjects on with requiredObjectFields off (today's default) must also build"}, } - dir := t.TempDir() - roMaterialize(t, schemaFS, dir) - modelsDir := filepath.Join(dir, "models") + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataFS}, "testdata") + schemaFS, err := goGenerator{accessors: true, runtimeObjects: true, requiredObjectFields: tc.args}.GenerateFromCRD(t.Context(), inputFS, nil) + if err != nil { + t.Fatal(err) + } - resolveGeneratedModuleDeps(t, modelsDir) + dir := t.TempDir() + roMaterialize(t, schemaFS, dir) + modelsDir := filepath.Join(dir, "models") - cmd := exec.CommandContext(t.Context(), "go", "build", "./...") - cmd.Dir = modelsDir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("generated models failed to compile with both features on: %v\n%s", err, out) + resolveGeneratedModuleDeps(t, modelsDir) + + cmd := exec.CommandContext(t.Context(), "go", "build", "./...") + cmd.Dir = modelsDir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("generated models failed to compile (%s): %v\n%s", tc.reason, err, out) + } + }) } } // TestGenerateFromOpenAPIWithAccessorsAndRuntimeObjects is the same check for -// the OpenAPI path, which generates the far larger built-in Kubernetes packages. +// the OpenAPI path, which generates the far larger built-in Kubernetes +// packages, as a table over requiredObjectFields. The chained getter through +// DeviceClass.Spec — resolved through a type alias either way (see +// TestDeviceClassSpecDeepCopyIndependence) — proves GetSpec returns a +// pointer usable exactly like every other field's getter, whether Spec +// itself is a pointer (requiredObjectFields off) or a non-pointer struct +// value (on); accessors and DeepCopy only special-case a non-pointer field +// that's a locally declared struct — see isK8sSharedTypeRef in go.go — so +// requiredObjectFields on is also the gate for a required field resolving to +// a struct in another generated package. func TestGenerateFromOpenAPIWithAccessorsAndRuntimeObjects(t *testing.T) { - inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataJSONFS}, "testdata") - schemaFS, err := goGenerator{accessors: true, runtimeObjects: true}.GenerateFromOpenAPI(t.Context(), inputFS, nil) - if err != nil { - t.Fatal(err) + cases := map[string]struct { + args bool + reason string + }{ + "Enabled": {args: true, reason: "GetSpec must return a pointer even when Spec itself is a non-pointer struct value"}, + "Disabled": {args: false, reason: "accessors+runtimeObjects on with requiredObjectFields off (today's default) must also chain through GetSpec"}, } - dir := t.TempDir() - roMaterialize(t, schemaFS, dir) - modelsDir := filepath.Join(dir, "models") + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + inputFS := afero.NewBasePathFs(afero.FromIOFS{FS: testdataJSONFS}, "testdata") + schemaFS, err := goGenerator{accessors: true, runtimeObjects: true, requiredObjectFields: tc.args}.GenerateFromOpenAPI(t.Context(), inputFS, nil) + if err != nil { + t.Fatal(err) + } - resolveGeneratedModuleDeps(t, modelsDir) + dir := t.TempDir() + roMaterialize(t, schemaFS, dir) + modelsDir := filepath.Join(dir, "models") - cmd := exec.CommandContext(t.Context(), "go", "build", "./...") - cmd.Dir = modelsDir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("generated OpenAPI models failed to compile with both features on: %v\n%s", err, out) + consumer := `package consumer + +import ( + resourcev1 "dev.crossplane.io/models/io/k8s/api/resource/v1" +) + +func ChainOnEmptyDeviceClass() *string { + return (&resourcev1.DeviceClass{}).GetSpec().GetExtendedResourceName() +} +` + consumerDir := filepath.Join(dir, "models", "consumer") + if err := os.MkdirAll(consumerDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(consumerDir, "consumer.go"), []byte(consumer), 0o644); err != nil { + t.Fatal(err) + } + consumerTest := `package consumer + +import "testing" + +func TestChainOnEmptyDeviceClassDoesNotPanic(t *testing.T) { + if got := ChainOnEmptyDeviceClass(); got != nil { + t.Errorf("expected nil from a chain over an empty resource, got %v", *got) + } +} +` + if err := os.WriteFile(filepath.Join(consumerDir, "consumer_test.go"), []byte(consumerTest), 0o644); err != nil { + t.Fatal(err) + } + + resolveGeneratedModuleDeps(t, modelsDir) + + cmd := exec.CommandContext(t.Context(), "go", "test", "./...") + cmd.Dir = modelsDir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("generated OpenAPI models failed to compile/test (%s): %v\n%s", tc.reason, err, out) + } + }) } } diff --git a/internal/schemas/generator/runtimeobject_test.go b/internal/schemas/generator/runtimeobject_test.go index 4fdd4369..9c071645 100644 --- a/internal/schemas/generator/runtimeobject_test.go +++ b/internal/schemas/generator/runtimeobject_test.go @@ -20,6 +20,7 @@ import ( "go/ast" "go/parser" "go/token" + "strings" "testing" ) @@ -195,6 +196,117 @@ type Foo struct { } } +// TestWriteFieldCopy covers writeFieldCopy's field cases: a required +// object-typed field (see goRemoveRequired) needs a real DeepCopyInto call +// rather than the top-level shallow `*out = *in`, which would alias nested +// pointers; that struct may be typed by a component-name alias rather than +// its own name (oapi-codegen's x-go-type-name pattern), for both a +// non-pointer required field and an ordinary optional pointer field; +// oapi-codegen's unexported `union json.RawMessage` field needs its backing +// bytes copied for the same reason (its MarshalJSON exposes that slice +// directly); and a plain scalar field needs no special-casing, since a value +// type has no separate backing storage to alias. +func TestWriteFieldCopy(t *testing.T) { + cases := map[string]struct { + args string + wantContains []string + wantAbsent []string + reason string + }{ + "ValueStructField": { + args: `package v1alpha1 + +type BarValue struct { + Count *int64 ` + "`json:\"count,omitempty\"`" + ` +} + +type Foo struct { + Bar BarValue ` + "`json:\"bar\"`" + ` +} +`, + wantContains: []string{"in.Bar.DeepCopyInto(&out.Bar)"}, + reason: "a required object-typed field gets a real DeepCopyInto call", + }, + "ValueStructFieldViaAlias": { + args: `package v1alpha1 + +type RealBar struct { + Count *int64 ` + "`json:\"count,omitempty\"`" + ` +} + +type BarAlias = RealBar + +type Foo struct { + Bar BarAlias ` + "`json:\"bar\"`" + ` +} +`, + wantContains: []string{"in.Bar.DeepCopyInto(&out.Bar)"}, + reason: "oapi-codegen types a $ref field by its component-name alias (e.g. IoK8SApiResourceV1DeviceClassSpec = DeviceClassSpec), so a field typed by that alias must still be recognized as the local struct it names", + }, + "PointerToAliasStructField": { + args: `package v1alpha1 + +type RealBar struct { + Count *int64 ` + "`json:\"count,omitempty\"`" + ` +} + +type Bar = RealBar + +type Foo struct { + Bar *Bar ` + "`json:\"bar,omitempty\"`" + ` +} +`, + wantContains: []string{"(*in).DeepCopyInto(*out)"}, + reason: "this alias shape isn't unique to required object-typed fields: an ordinary optional pointer field typed by a component-name alias must call DeepCopyInto too, not fall back to the shallow **out = **in that classifyElem uses for an unrecognized identifier", + }, + "RawMessageField": { + args: `package v1alpha1 + +import "encoding/json" + +type Foo struct { + Union json.RawMessage ` + "`json:\"-\"`" + ` +} +`, + wantContains: []string{ + "out.Union = make(json.RawMessage, len(in.Union))", + "copy(out.Union, in.Union)", + }, + reason: "the union field's backing bytes are copied independently, not aliased", + }, + "PlainScalarField": { + args: `package v1alpha1 + +type Foo struct { + Count int64 ` + "`json:\"count\"`" + ` +} +`, + wantAbsent: []string{"in.Count.DeepCopyInto", "out.Count = make"}, + reason: "a plain scalar field needs no special-cased copy code", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got, _, err := addRuntimeObjects(tc.args) + if err != nil { + t.Fatalf("addRuntimeObjects: %v", err) + } + + for _, want := range tc.wantContains { + if !strings.Contains(got, want) { + t.Errorf("expected generated DeepCopyInto to contain %q (%s), got:\n%s", want, tc.reason, got) + } + } + for _, notWant := range tc.wantAbsent { + if strings.Contains(got, notWant) { + t.Errorf("did not expect generated DeepCopyInto to contain %q (%s), got:\n%s", notWant, tc.reason, got) + } + } + }) + } +} + // countMethod returns how many times recv.method is declared in src. func countMethod(t *testing.T, src, recv, method string) int { t.Helper()