feat: add GetX/SetX accessors to generated Go models#160
feat: add GetX/SetX accessors to generated Go models#160erikmiller-gusto wants to merge 1 commit into
Conversation
erikmiller-gusto
requested review from
a team,
jcogilvie and
tampakrap
as code owners
|
Warning Review limit reached
More reviews will be available in 32 minutes and 2 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe generator now supports optional getter and setter emission for Go models, with config and CLI wiring to toggle it and tests covering generation and compilation. ChangesGo model accessor feature
Sequence Diagram(s)sequenceDiagram
participant Config as internal/config.Config
participant GenerateCmd as cmd/crossplane/function.generateCmd
participant ProjectCmd as cmd/crossplane/project.buildCmd
participant GoGen as internal/schemas/generator.goGenerator
participant AddAccessors as internal/schemas/generator.addAccessors
Config->>GenerateCmd: provide GenerateGoModelAccessors
Config->>ProjectCmd: provide GenerateGoModelAccessors
GenerateCmd->>GoGen: filter schemas with feature flag
ProjectCmd->>GoGen: build schema generation with feature flag
GoGen->>AddAccessors: append accessors when enabled
AddAccessors-->>GoGen: formatted source or wrapped error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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
erikmiller-gusto
force-pushed
the
main
branch
2 times, most recently
from
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
| // Bind the loaded config so commands can read feature flags at runtime. | ||
| kong.Bind(cfg), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all Run methods that take *config.Config and any direct (non-kong) callers in tests.
rg -nP --type=go '\)\s*Run\([^)]*\*config\.Config' -C1
echo '--- direct Run callers in tests ---'
rg -nP --type=go '\.Run\(' -g '**/*_test.go' -C2Repository: crossplane/cli
Length of output: 152
Binding cfg is correct, but adding a nil guard prevents future panics
The search confirms build, run, and generate rely on the kong.Bind(cfg) at line 130, and no direct .Run() calls were found in tests, so the current path is safe.
However, to make these commands robust against future programmatic invocation or test changes where Kong might not inject cfg:
- Add a simple nil check at the start of
Runmethods incmd/crossplane/project/build.go,cmd/crossplane/project/run.go, andcmd/crossplane/function/generate.go. - Return an error if
cfgis nil to avoid a panic oncfg.Featuresaccess.
This defensive pattern ensures the command fails gracefully with a clear message rather than crashing silently if called outside the Kong pipeline.
🤖 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 `@cmd/crossplane/main.go` around lines 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.
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>
1 participant
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.