feat: add GetX/SetX accessors to generated Go models - #160
feat: add GetX/SetX accessors to generated Go models#160erikmiller-gusto wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe generator now supports optional ChangesGo model accessor feature
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/schemas/generator/accessors_test.go (1)
111-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThanks for the thorough coverage here — would you be open to nudging these toward the repo's table-driven conventions?
These tests read well, and I appreciate the compile gate especially. To align with our test conventions, would it be worth giving the case tables a
reasonfield and running them as subtests viat.Run(name, ...), and usingcmp.Difffor the type comparisons inTestAddAccessors(e.g. on thewant/gotmethod maps)? That tends to give nicer failure output and matches the rest of the suite.No action needed if you'd rather defer, but flagging since it's a documented convention. As per path instructions: "Enforce table-driven test structure ... args/want pattern, use cmp.Diff ... Check for proper test case naming and reason fields."
Also applies to: 226-282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/schemas/generator/accessors_test.go` around lines 111 - 143, The tests here should be nudged into the repo’s table-driven style: update the cases in TestGenerateFromCRDIncludesAccessors and the related TestAddAccessors coverage to use named subtests with t.Run and include a reason field in each table entry for clearer intent. For the method-map/type assertions in TestAddAccessors, switch from direct comparisons to cmp.Diff on the want/got values to improve failure output and match the suite’s conventions. Use the existing test function names and method-map helpers as the anchor points when refactoring.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/schemas/generator/accessors_test.go`:
- Around line 175-224: The compile gate in
TestGeneratedModelsCompileWithAccessors is not hermetic because the generated
module path dev.crossplane.io/models must be resolved externally during go
build. Update the generator that writes the materialized go.mod so the generated
module can resolve locally, ideally by adding a replace directive that points
dev.crossplane.io/models to the on-disk output directory, or otherwise make the
offline CI requirement explicit if that’s the intended mechanism. Use the
TestGeneratedModelsCompileWithAccessors flow and the generated go.mod output
from goGenerator{}.GenerateFromCRD as the places to verify the fix.
In `@internal/schemas/generator/accessors.go`:
- Around line 34-39: `addAccessors`/`writeStructAccessors` currently emit
`GetX`/`SetX` methods without checking for existing methods, which can collide
with `oapi-codegen`-generated accessors or union helpers. Update
`writeStructAccessors` to inspect the target type’s existing method set before
generating each accessor, and skip any `GetX`/`SetX` whose name is already
present on that struct. Keep the check localized around `writeStructAccessors`
so the generator remains defensive against `additionalProperties` and union-type
method names like `GetAdditionalProperties`, `As`, or `From`.
---
Nitpick comments:
In `@internal/schemas/generator/accessors_test.go`:
- Around line 111-143: The tests here should be nudged into the repo’s
table-driven style: update the cases in TestGenerateFromCRDIncludesAccessors and
the related TestAddAccessors coverage to use named subtests with t.Run and
include a reason field in each table entry for clearer intent. For the
method-map/type assertions in TestAddAccessors, switch from direct comparisons
to cmp.Diff on the want/got values to improve failure output and match the
suite’s conventions. Use the existing test function names and method-map helpers
as the anchor points when refactoring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 412e1a53-fb0f-44e9-977e-1b7d63eed6e7
📒 Files selected for processing (3)
internal/schemas/generator/accessors.gointernal/schemas/generator/accessors_test.gointernal/schemas/generator/go.go
fb2efaf to
f0c8846
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/crossplane/main.go`:
- Around line 129-130: The `build`, `run`, and `generate` command entrypoints
currently assume `cfg` is always injected by Kong, which could panic if they are
invoked programmatically or in tests; add a nil check at the start of each `Run`
method in `cmd/crossplane/project/build.go`, `cmd/crossplane/project/run.go`,
and `cmd/crossplane/function/generate.go`, and return a clear error before any
`cfg.Features` access when `cfg` is nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e91df577-976e-4c3f-b795-54476785f9e3
📒 Files selected for processing (12)
cmd/crossplane/config/help/config.mdcmd/crossplane/config/set.gocmd/crossplane/function/generate.gocmd/crossplane/function/generate_test.gocmd/crossplane/main.gocmd/crossplane/project/build.gocmd/crossplane/project/run.gointernal/config/config.gointernal/schemas/generator/accessors.gointernal/schemas/generator/accessors_test.gointernal/schemas/generator/go.gointernal/schemas/generator/interface.go
✅ Files skipped from review due to trivial changes (1)
- cmd/crossplane/config/help/config.md
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/schemas/generator/accessors.go
Generate pointer-in/pointer-out accessor methods for every field of every struct in the generated Go models, so consumers can abstract over resources with interfaces and generics. Folded into generateGo as a single AST post-process step so it applies uniformly across all generation paths. Gated behind a new features.generateGoModelAccessors config flag (off by default), threaded from config through the project build/run and function generate commands into the Go generator. Skips any GetX/SetX whose name already exists on a type, so the accessors never collide with methods oapi-codegen already emits (e.g. union or additionalProperties helpers). Verified by a compile gate that builds the generated module with a consumer that uses an accessor through an interface; the gate skips gracefully when module dependencies cannot be resolved offline. Signed-off-by: Erik Miller <erik.miller@gusto.com>
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
Thanks for the contribution @erikmiller-gusto!
The core of this looks good to me. Leaving a few thoughts inline. My biggest concern is unifying the approach with #162 if possible, preferably also setting us up nicely to make the additional code generation non-optional in the future with minimal changes.
| Generate GetX/SetX accessor methods on generated Go models (off by default), so | ||
| generated resources can be used through interfaces and generics: |
There was a problem hiding this comment.
Seems reasonable to disable this by default initially in case some bug in the generation results in broken Go bindings for some cases. I do think we should plan to enable it by default at some point, so that there's only one code path to test. We haven't really defined what the feature lifecycle looks like for the CLI, but maybe for now we can just create an issue to remind us to come back to this a couple releases down the line.
| return out | ||
| } | ||
|
|
||
| func renderRecv(e ast.Expr) string { |
There was a problem hiding this comment.
This is effectively the same as receiverTypeName in accessors.go. Any reason to redefine it just for tests? Seems equally likely to have a bug in either definition :-).
| var generateGoMutex sync.Mutex //nolint:gochecknoglobals // Must be global. | ||
|
|
||
| func generateGo(s *spec3.OpenAPI, version string, mutators ...func(*spec3.OpenAPI)) (string, error) { | ||
| func generateGo(s *spec3.OpenAPI, version string, accessors bool, mutators ...func(*spec3.OpenAPI)) (string, error) { |
There was a problem hiding this comment.
Would it make sense to handle the accessor generation in this PR and the runtime.Object method generation in #162 the same way? I.e., either pass a set of "code mutators" to generateGo and make it responsible for adding both sets of extra methods, or have the calling code generate the additional methods after the fact as in #162.
Totally fair if there's a reason that PR and this PR work differently, I was just surprised by it :-).
Apply accessor generation after Go post-processing at the call sites via a new applyAccessors helper, rather than folding it into generateGo. This mirrors the runtime.Object generation approach in crossplane#162 so both features use the same "post-process the generated code after the fact" pattern, and makes generateGo's signature identical across both, keeping the two changes easy to reconcile. Generating accessors after fixK8sTypeNames/removeSelfImports also means they reference the final type names. Skip unexported struct fields when emitting accessors: generated models don't currently have any, but an accessor for one would be useless to external consumers and could produce oddly-cased method names. Reuse receiverTypeName from accessors.go in the tests instead of redefining an equivalent renderRecv helper. Signed-off-by: Erik Miller <erik.miller@gusto.com>
Applying accessors in each generation loop pushed GenerateFromCRD past the gocognit limit. Extract the shared-K8s-package loop body into a generateSharedK8sPackage helper to bring it back under. Signed-off-by: Erik Miller <erik.miller@gusto.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/schemas/generator/go.go`:
- Around line 1272-1277: Update the error wrapping in both accessor-generation
call sites in internal/schemas/generator/go.go at lines 1272-1277 and 1411-1416,
replacing the generic message with “failed to generate Go model accessors.” Keep
the existing applyAccessors error propagation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac59f45c-d434-40ba-b56c-998cb7463bc2
📒 Files selected for processing (12)
cmd/crossplane/config/help/config.mdcmd/crossplane/config/set.gocmd/crossplane/function/generate.gocmd/crossplane/function/generate_test.gocmd/crossplane/main.gocmd/crossplane/project/build.gocmd/crossplane/project/run.gointernal/config/config.gointernal/schemas/generator/accessors.gointernal/schemas/generator/accessors_test.gointernal/schemas/generator/go.gointernal/schemas/generator/interface.go
🚧 Files skipped from review as they are similar to previous changes (10)
- cmd/crossplane/config/set.go
- internal/schemas/generator/interface.go
- internal/config/config.go
- cmd/crossplane/project/run.go
- cmd/crossplane/config/help/config.md
- cmd/crossplane/function/generate_test.go
- cmd/crossplane/main.go
- internal/schemas/generator/accessors.go
- cmd/crossplane/function/generate.go
- internal/schemas/generator/accessors_test.go
Reword the wrapped error at both accessor call sites from the generic "failed to add accessors" to "failed to generate Go model accessors". Signed-off-by: Erik Miller <erik.miller@gusto.com>
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
The code generation here looks good to me, and works in my quick testing. Would be great for @haarchri to take a look as well, since he's worked in the Go generator code a lot.
One issue I did notice in testing: threading through the config option is missing in a few places where we generate schemas. Specifically, the crossplane dependency commands (add and update-cache) and the render commands (both composition and operation). The latter are a little tricky since they currently use the default generator from dependency.NewManager.
Upstream main dropped the `level: error` input from the Vale step, so reviewdog now runs at `-level=info` and Vale warnings fail validate-docs. "so generated resources can be used through interfaces and generics" trips write-good.Passive. Verified with Vale 3.14.2 against the crossplane/docs config at the alert level the action uses: 0 errors, 0 warnings. Signed-off-by: Erik Miller <erik.miller@gusto.com>
2 participants
Add accessor/setter methods to generated Go models
Problem
The Go model generator produces structs that expose only fields:
Because the generated types have no methods, consumers can't abstract over resources with interfaces or write generic code that operates on "any resource with field X". Every field is a pointer (the goRemoveRequired mutator strips required, so oapi-codegen emits everything as optional *T).
What this does
Generates GetX/SetX accessor methods for every field of every generated struct, so consumers can define their own interfaces and satisfy them structurally:
Testing
go test ./... → all passing. go build, gofmt, go vet clean.
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.Addedbackport release-x.ylabels to auto-backport this PR.On the two struck items: the
crossplane confighelp text this PR extends isrendered into the generated command reference, so
crossplane/docspicks the newkey up without a separate change. And this is a new opt-in feature rather than a
fix, so it isn't a backport candidate.
Need help with this checklist? See the cheat sheet.