Repository navigation
fix: generate required object-typed fields as non-pointer values - #447
Open
GenerikUser wants to merge 1 commit into
Open
GenerikUser wants to merge 1 commit into
GenerikUser wants to merge 1 commit into
Conversation
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.
The compile gate also marshals a zero XAccountScaffoldSpec in both flag
states: with the flag on, the unset required parameters field must
serialize as {}; with it off, it must be omitted, as before.
Signed-off-by: Erik Miller <erik.miller@gusto.com>
Signed-off-by: GenerikUser <generikuser@users.noreply.github.com>
GenerikUser
requested review from
a team,
jcogilvie and
tampakrap
as code owners
October 23, 2026 01:06
2 of 5 tasks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The 64-character title clearly describes the main change and stays under the 72-character limit. |
| Description check | |
The description clearly explains the problem, opt-in feature flag, compatibility behavior, supporting changes, and test coverage. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Breaking Changes | |
The pull request does not match the stated failure conditions. The authoritative diff contains no changes under apis/**; under cmd/**, it only adds and wires the optional `features.generateGoRequi… |
| Feature Gate Requirement | |
The significant generated-model behavior change is gated. The diff adds Features.GenerateGoRequiredObjectFields with a false zero-value default, supports features.generateGoRequiredObjectFields in… |
Full details: Breaking Changes
-
Explanation
The pull request does not match the stated failure conditions. The authoritative diff contains no changes under
apis/**; undercmd/**, it only adds and wires the optionalfeatures.generateGoRequiredObjectFieldsconfiguration key. The config field and generator option are disabled by default, so existing behavior remains unchanged unless explicitly enabled. No public field or flag is removed or renamed, and no new required public field or flag is added.
Full details: Feature Gate Requirement
-
Explanation
The significant generated-model behavior change is gated. The diff adds
Features.GenerateGoRequiredObjectFieldswith a false zero-value default, supportsfeatures.generateGoRequiredObjectFieldsincrossplane config set, and wires it throughWithGoRequiredObjectFieldsinto the Go generator. The generator preserves the existing all-optional behavior when the flag is disabled. The flag is documented in CLI config help, and all command-levelgenerator.AllLanguagespaths that use configuration pass it through.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Author
|
NOTE this is a replacement of #387 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
The problem. Generated Go models make every field optional: every nested object becomes a pointer with
omitempty. That holds even when the CRD marks the object as required. For a managed resource this means that if a function never setsspec.forProvider, the key is dropped from the output. The API server then rejects the resource:Provider types don't hit this problem. Upjet generates
ForProvider <Kind>Parameterswith tagjson:"forProvider": a value struct with noomitempty, which marshals as{}when nobody sets it. We ran into this when we switched a function from importing a provider's Go types to these generated models.An empty
forProvideris legitimate and common in upjet providers. Their required parameters aren't schema-requiredinsideforProvider. They are CEL rules such as!('*' in self.managementPolicies || 'Create' in ...) || has(self.forProvider.location) || (has(self.initProvider) && has(self.initProvider.location)). SoforProvider: {}passes validation when:initProvider, orinternal/schemas/generator/testdata/azure_linux_function_app.yamlis a real example:specrequiresforProvider, andlocation,nameandsiteConfigare CEL rules.The change. There is a new flag, off by default:
When it's on, required properties with an object type are generated as non-pointer value structs, so they marshal as
{}. Everything else keeps the current all-pointer convention.The flag is opt-in because it breaks consumers that write
Spec: &FooSpec{...}or nil-check those fields.A required field stays a pointer when making it a value would break compilation or DeepCopy:
allOfstructs,Supporting changes:
DeepCopyIntonow deep-copies value-struct fields instead of shallow-copying them.collectStructTypesresolvestype A = Baliases.GetX() *T,SetX(*T)), so chained getters keep compiling in both flag states.runtimeObjectson, theunion json.RawMessagebytes are copied rather than aliased. This change also applies when the flag is off.Testing:
go test ./...,go test -tags compilegate ./internal/schemas/generator/...and./nix.sh flake checkall pass locally.I have:
./nix.sh flake checkto ensure this PR is ready for review.Linked a PR or a docs tracking issue to document this change.The flag is documented in the CLI's own config help (cmd/crossplane/config/help/config.md).AddedNew opt-in feature flag, so it shouldn't be backported.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.