fix: resolve import alias collisions and NewRESTFunc signature in generator#6
Conversation
…erator Two bugs in the register generator: 1. Import alias collisions: aliases are formed by joining the last two path segments (e.g. storage/v1 → storagev1). When two modules share the same trailing segments, this produces duplicate aliases. Fixed by tracking seen aliases per APIGroup and disambiguating with the module name when a collision is detected. 2. NewRESTFunc signature: the template emitted `func() rest.Storage` but subresource closures called `RESTFunc(Factory)`. Fixed the signature to `func(factory managerfactory.SharedManagerFactory) rest.Storage` and updated all call sites to pass Factory.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughVersion manifest downgraded from 1.1.0 to 1.0.0, CHANGELOG.md entries removed, import alias handling refactored in parse.go with centralized collision detection, and NewRESTFunc signature updated to accept SharedManagerFactory in unversioned_generator.go. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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)
pkg/generate/unversioned_generator.go (1)
78-86: Wrapper closures still accept an unusedgetter generic.RESTOptionsGetter.The top-level wrappers around
New{{ $api.REST }}Func/New{{ $api.StatusREST }}Functakegetter generic.RESTOptionsGetterbut never use it — onlyFactoryis forwarded. That parameter exists to satisfy the signature expected by the builders registration, so this is not a correctness bug, but if the intent is to migrate fully toSharedManagerFactory-based construction, it's worth documenting (or eventually dropping thegetteronce the builders side is updated) to avoid confusing future readers of the generated code. No change required for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/generate/unversioned_generator.go` around lines 78 - 86, The wrapper closures New{{ $api.REST }} and New{{ $api.StatusREST }} accept a getter generic.RESTOptionsGetter but don't use it (they only call New{{ $api.REST }}Func / New{{ $api.StatusREST }}Func with Factory); add a concise comment above these wrappers explaining that the getter is intentionally unused because construction is driven by SharedManagerFactory (mention New{{ $api.REST }}, New{{ $api.StatusREST }}, New{{ $api.REST }}Func, New{{ $api.StatusREST }}Func and Factory) so future readers know it’s kept only to satisfy the builders registration signature, and remove the parameter later when builders are migrated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/generate/parse.go`:
- Around line 54-82: The resolveImportAlias function can overwrite earlier
mappings when a disambiguated alias collides; update APIGroup.resolveImportAlias
to ensure any produced alias is either already bound to the same pkgPath or
unique before writing to g.importAliases: compute base as before, then on
collision build a prefix from parts (handle paths with fewer than 3 segments by
falling back to a deterministic fallback like base or the available part), and
loop to extend the alias (e.g., append additional path segments or a numeric
suffix) until alias is unused or maps to the same pkgPath; only then set
g.importAliases[alias] = pkgPath and return alias. Ensure you reference and
check g.importAliases for both base and derived alias collisions and avoid
unconditional overwrite of alias.
In `@pkg/generate/unversioned_generator.go`:
- Around line 63-67: The UnversionedAPITemplate references a non-existent import
and symbols: update the generator so the template either imports a real package
or remove the bogus import and types; specifically ensure that the
managerfactory package and the SharedManagerFactory type exist (or change
NewRESTFunc to use the correct existing factory type) and that the generated
package defines the Factory variable before any generated code uses it. Locate
UnversionedAPITemplate and the NewRESTFunc signature, then either create
github.com/devsy-org/apiserver/pkg/managerfactory with type SharedManagerFactory
and expose it, or change the template to reference the actual factory type and
add a generated top-level Factory variable in the output package so compilation
succeeds.
---
Nitpick comments:
In `@pkg/generate/unversioned_generator.go`:
- Around line 78-86: The wrapper closures New{{ $api.REST }} and New{{
$api.StatusREST }} accept a getter generic.RESTOptionsGetter but don't use it
(they only call New{{ $api.REST }}Func / New{{ $api.StatusREST }}Func with
Factory); add a concise comment above these wrappers explaining that the getter
is intentionally unused because construction is driven by SharedManagerFactory
(mention New{{ $api.REST }}, New{{ $api.StatusREST }}, New{{ $api.REST }}Func,
New{{ $api.StatusREST }}Func and Factory) so future readers know it’s kept only
to satisfy the builders registration signature, and remove the parameter later
when builders are migrated.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d86126d2-33c0-4027-ad04-c495eba723b1
📒 Files selected for processing (4)
.release-please-manifest.jsonCHANGELOG.mdpkg/generate/parse.gopkg/generate/unversioned_generator.go
💤 Files with no reviewable changes (1)
- CHANGELOG.md
| func (g *APIGroup) resolveImportAlias(pkgPath string) string { | ||
| base := path.Base(path.Dir(pkgPath)) + path.Base(pkgPath) | ||
|
|
||
| if existing, ok := g.importAliases[base]; ok && existing == pkgPath { | ||
| return base | ||
| } | ||
|
|
||
| if _, ok := g.importAliases[base]; !ok { | ||
| g.importAliases[base] = pkgPath | ||
| return base | ||
| } | ||
|
|
||
| // Collision: disambiguate using the module name (third segment of the import path). | ||
| parts := strings.Split(pkgPath, "/") | ||
| moduleName := "" | ||
| if len(parts) >= 3 { | ||
| moduleName = parts[2] | ||
| } | ||
| prefix := strings.ReplaceAll(moduleName, "-", "") | ||
| prefix = strings.TrimSuffix(prefix, "apis") | ||
| prefix = strings.TrimSuffix(prefix, "api") | ||
| if prefix == "" || prefix == base { | ||
| prefix = strings.ReplaceAll(moduleName, "-", "") | ||
| } | ||
|
|
||
| alias := prefix + base | ||
| g.importAliases[alias] = pkgPath | ||
| return alias | ||
| } |
There was a problem hiding this comment.
Disambiguated alias is stored without a collision check — multi-way collisions can still map different packages to the same alias.
On the collision branch, alias := prefix + base is written to g.importAliases unconditionally (line 80). If a third package later hashes to the same base and its derived prefix also produces the same alias (e.g., two modules with the same stripped name shape, like foo-apis and foo-api both reducing to prefix foo), the map silently overwrites the earlier mapping and two different pkgPath values end up sharing one alias — exactly the collision this function is meant to prevent. Since generated import statements are emitted per-field via fmt.Sprintf("%s \"%s\"", importAlias, pkg), a sets.String of imports would then contain two different alias "path" lines sharing the same alias token, producing malformed Go.
Also minor: parts[2] as "module name" assumes a VCS-style path (host/org/module/...). For paths shorter than 3 segments the code falls back to "" and then to base (which returns base unchanged, i.e., no disambiguation). Consider handling that fallback explicitly.
Suggested hardening: check that the chosen alias is either unused or already bound to this pkgPath, and otherwise keep extending the prefix until unique.
🛠️ Proposed fix
func (g *APIGroup) resolveImportAlias(pkgPath string) string {
base := path.Base(path.Dir(pkgPath)) + path.Base(pkgPath)
if existing, ok := g.importAliases[base]; ok && existing == pkgPath {
return base
}
if _, ok := g.importAliases[base]; !ok {
g.importAliases[base] = pkgPath
return base
}
// Collision: disambiguate using the module name (third segment of the import path).
parts := strings.Split(pkgPath, "/")
moduleName := ""
if len(parts) >= 3 {
moduleName = parts[2]
}
prefix := strings.ReplaceAll(moduleName, "-", "")
prefix = strings.TrimSuffix(prefix, "apis")
prefix = strings.TrimSuffix(prefix, "api")
if prefix == "" || prefix == base {
prefix = strings.ReplaceAll(moduleName, "-", "")
}
- alias := prefix + base
- g.importAliases[alias] = pkgPath
- return alias
+ alias := prefix + base
+ // If the disambiguated alias is still taken by a different package,
+ // keep extending until we find a free slot (or return the existing one if it matches).
+ for {
+ existing, ok := g.importAliases[alias]
+ if !ok {
+ g.importAliases[alias] = pkgPath
+ return alias
+ }
+ if existing == pkgPath {
+ return alias
+ }
+ alias = prefix + alias
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/generate/parse.go` around lines 54 - 82, The resolveImportAlias function
can overwrite earlier mappings when a disambiguated alias collides; update
APIGroup.resolveImportAlias to ensure any produced alias is either already bound
to the same pkgPath or unique before writing to g.importAliases: compute base as
before, then on collision build a prefix from parts (handle paths with fewer
than 3 segments by falling back to a deterministic fallback like base or the
available part), and loop to extend the alias (e.g., append additional path
segments or a numeric suffix) until alias is unused or maps to the same pkgPath;
only then set g.importAliases[alias] = pkgPath and return alias. Ensure you
reference and check g.importAliases for both base and derived alias collisions
and avoid unconditional overwrite of alias.
| "github.com/devsy-org/apiserver/pkg/managerfactory", | ||
| } | ||
|
|
||
| var UnversionedAPITemplate = ` | ||
| type NewRESTFunc func() rest.Storage | ||
| type NewRESTFunc func(factory managerfactory.SharedManagerFactory) rest.Storage |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the managerfactory package and confirm SharedManagerFactory is defined & exported.
fd -t d managerfactory
rg -nP -C2 '\btype\s+SharedManagerFactory\b'
rg -nP -C1 '\bSharedManagerFactory\b' -g '!**/generate/**'Repository: devsy-org/apiserver
Length of output: 45
🏁 Script executed:
# Search for any files/directories with "manager" in the name
fd -i manager
# Try simpler grep patterns for SharedManagerFactory
rg 'SharedManagerFactory' -i
# Check if there are any Go files in pkg directory
fd -t f '\.go$' pkg/
# Look for factory-related files
fd -i factoryRepository: devsy-org/apiserver
Length of output: 816
🏁 Script executed:
# Read the unversioned_generator.go file with line numbers
cat -n pkg/generate/unversioned_generator.go | head -100
# Check imports in that file
rg -n 'import|managerfactory' pkg/generate/unversioned_generator.go -A 5 -B 5Repository: devsy-org/apiserver
Length of output: 5112
🏁 Script executed:
# List all directories under pkg
ls -la pkg/
# Search for any mention of managerfactory in comments or other contexts
rg 'managerfactory' -i
# Check if Factory variable is defined anywhere
rg '\bFactory\b' -t goRepository: devsy-org/apiserver
Length of output: 921
Missing managerfactory package and SharedManagerFactory type will cause compilation failures in all generated packages.
The generated code imports a non-existent package (github.com/devsy-org/apiserver/pkg/managerfactory) and references an undefined type (SharedManagerFactory). The template also references an undefined Factory variable. Every generated unversioned API package will fail to compile until the managerfactory package is created with the SharedManagerFactory type, and the Factory variable is provided in the generated package scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/generate/unversioned_generator.go` around lines 63 - 67, The
UnversionedAPITemplate references a non-existent import and symbols: update the
generator so the template either imports a real package or remove the bogus
import and types; specifically ensure that the managerfactory package and the
SharedManagerFactory type exist (or change NewRESTFunc to use the correct
existing factory type) and that the generated package defines the Factory
variable before any generated code uses it. Locate UnversionedAPITemplate and
the NewRESTFunc signature, then either create
github.com/devsy-org/apiserver/pkg/managerfactory with type SharedManagerFactory
and expose it, or change the template to reference the actual factory type and
add a generated top-level Factory variable in the output package so compilation
succeeds.
…CI step Place unexported resolveImportAlias method after the exported DoType method to satisfy funcorder linter. Remove unnecessary jlumbroso/free-disk-space step from lint workflow.
1 participant
Summary
Import alias collisions: The register generator forms import aliases by joining the last two path segments (e.g.
storage/v1→storagev1). When two modules expose packages with identical trailing segments (e.g.devsy-org/api/.../storage/v1anddevsy-org/agentapi/.../storage/v1), this produces duplicate aliases that break compilation. Fixed by tracking aliases perAPIGroupviaresolveImportAlias()and disambiguating with the module name on collision.NewRESTFunc signature: The template emitted
func() rest.Storagebut subresource closures calledRESTFunc(Factory), requiring consumers to post-process generated files. Fixed the type tofunc(factory managerfactory.SharedManagerFactory) rest.Storageand updated all template call sites to passFactory.Summary by CodeRabbit
Chores
Refactor