feat: add GetX/SetX accessors to generated Go models#160
feat: add GetX/SetX accessors to generated Go models#160erikmiller-gusto wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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.
| // to avoid colliding with methods oapi-codegen already generated. | ||
| func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName string, st *ast.StructType, skip map[string]bool) { | ||
| for _, field := range st.Fields.List { | ||
| // Skip embedded/anonymous fields; generated models don't use them. |
There was a problem hiding this comment.
Should we also skip unexported fields? Not sure there actually are any in our generated models, but seems like a good safeguard.
| 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>
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.