Conversation
Summary by CodeRabbit
WalkthroughGetName() and InjectRepository now use reflection to call an optional model method RepositoryName(); if present its return value is used as the repository base name, otherwise code falls back to the struct name. Tests and CI workflow were updated to cover and always run these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Repo as Repository[M]
participant Reflect as reflect.Type/Value
participant Model as M.RepositoryName()
participant Fallback as common.GetStructName
Caller->>Repo: GetName() / InjectRepository
Repo->>Reflect: construct model value, look up "RepositoryName"
alt RepositoryName exists
Repo->>Model: call RepositoryName()
Model-->>Repo: "service" (string)
Repo-->>Caller: return "service"
else not exists
Repo->>Fallback: GetStructName(model)
Fallback-->>Repo: defaultName
Repo-->>Caller: return defaultName
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
repository_test.go (1)
72-85: Add a test for pointer-receiver RepositoryName()Ensure GetName() also works when models implement RepositoryName() on a pointer receiver.
Proposed addition:
type Def struct { sqlorm.Model `gorm:"embedded"` Name string `gorm:"type:varchar(255);not null"` } func (*Def) RepositoryName() string { return "service_ptr" } func TestNameRepoPtr(t *testing.T) { repo := sqlorm.NewRepo(Def{}) require.Equal(t, "service_ptr", repo.GetName()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
repository.go(1 hunks)repository_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
repository_test.go (2)
model.go (1)
Model(11-16)repository.go (1)
NewRepo(16-18)
🔇 Additional comments (1)
repository_test.go (1)
72-85: Good positive-path coverageThe test clearly validates the custom name path. Looks good.
|
|
||
| ctModel := reflect.ValueOf(&model).Elem() | ||
| fnc := ctModel.MethodByName("RepositoryName") | ||
| var name string | ||
| if fnc.IsValid() { | ||
| name = fnc.Call(nil)[0].String() | ||
| } else { | ||
| name = common.GetStructName(model) | ||
| } | ||
| return name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Support pointer receivers and remove panic risks by replacing reflection with a simple interface check
Current code misses methods with pointer receivers and can panic if the method’s return type isn’t string. Prefer a type assertion that handles both value and pointer receivers and falls back cleanly.
Apply this diff:
func (r *Repository[M]) GetName() string {
var model M
-
- ctModel := reflect.ValueOf(&model).Elem()
- fnc := ctModel.MethodByName("RepositoryName")
- var name string
- if fnc.IsValid() {
- name = fnc.Call(nil)[0].String()
- } else {
- name = common.GetStructName(model)
- }
- return name
+ // Prefer interface assertion over reflection; supports both value and pointer receivers.
+ type repositoryNamer interface{ RepositoryName() string }
+ if rn, ok := any(model).(repositoryNamer); ok {
+ if n := rn.RepositoryName(); n != "" {
+ return n
+ }
+ }
+ if rn, ok := any(&model).(repositoryNamer); ok {
+ if n := rn.RepositoryName(); n != "" {
+ return n
+ }
+ }
+ return common.GetStructName(model)
}If you prefer to keep reflection, minimally switch to pointer first and validate signature:
- ctModel := reflect.ValueOf(&model).Elem()
- fnc := ctModel.MethodByName("RepositoryName")
+ ptr := reflect.ValueOf(&model)
+ fnc := ptr.MethodByName("RepositoryName")
+ if !fnc.IsValid() {
+ fnc = ptr.Elem().MethodByName("RepositoryName")
+ }
- if fnc.IsValid() {
- name = fnc.Call(nil)[0].String()
+ if fnc.IsValid() && fnc.Type().NumIn() == 0 && fnc.Type().NumOut() >= 1 && fnc.Type().Out(0).Kind() == reflect.String {
+ out := fnc.Call(nil)
+ if len(out) > 0 {
+ s := out[0].String()
+ if s != "" {
+ return s
+ }
+ }
} else {
- name = common.GetStructName(model)
+ return common.GetStructName(model)
}
- return name
+ return common.GetStructName(model)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctModel := reflect.ValueOf(&model).Elem() | |
| fnc := ctModel.MethodByName("RepositoryName") | |
| var name string | |
| if fnc.IsValid() { | |
| name = fnc.Call(nil)[0].String() | |
| } else { | |
| name = common.GetStructName(model) | |
| } | |
| return name | |
| func (r *Repository[M]) GetName() string { | |
| var model M | |
| // Prefer interface assertion over reflection; supports both value and pointer receivers. | |
| type repositoryNamer interface{ RepositoryName() string } | |
| if rn, ok := any(model).(repositoryNamer); ok { | |
| if n := rn.RepositoryName(); n != "" { | |
| return n | |
| } | |
| } | |
| if rn, ok := any(&model).(repositoryNamer); ok { | |
| if n := rn.RepositoryName(); n != "" { | |
| return n | |
| } | |
| } | |
| return common.GetStructName(model) | |
| } |
🤖 Prompt for AI Agents
In repository.go around lines 26 to 35, the current reflection-based logic
misses methods with pointer receivers and may panic if the method signature
isn't string; replace it by defining a small interface type (e.g.
RepositoryNamer with RepositoryName() string) and perform a type assertion:
first try asserting the concrete model value, then try asserting its pointer
(address) if available; if either assertion succeeds, call RepositoryName() and
use the returned string, otherwise fall back to common.GetStructName(model).
This avoids reflection, supports pointer receivers, and removes the panic risk
by only calling the method after a successful interface assertion.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
module.go (2)
81-81: Drop redundant type conversion.GetRepoName already returns core.Provide.
- modelName := core.Provide(GetRepoName(name)) + modelName := GetRepoName(name)
72-80: Alternative: avoid reflection entirely (simpler and faster).If you’re open to it, interface assertions cover both pointer/value receivers cleanly.
- ctModel := reflect.ValueOf(&model).Elem() - fnc := ctModel.MethodByName("RepositoryName") - var name string - if fnc.IsValid() { - name = fnc.Call(nil)[0].String() - } else { - name = common.GetStructName(model) - } + var name string + if n, ok := any(&model).(interface{ RepositoryName() string }); ok { + name = n.RepositoryName() + } else if n, ok := any(model).(interface{ RepositoryName() string }); ok { + name = n.RepositoryName() + } else { + name = common.GetStructName(model) + }If you choose this, remove the reflect import.
module_test.go (1)
249-270: Add a test for pointer-receiver RepositoryName to prevent regressions.Current code won’t detect pointer receivers; add a test to cover that path once fixed.
func Test_RefName_PtrReceiver(t *testing.T) { type P struct { sqlorm.Model `gorm:"embedded"` Name string } // Pointer-receiver variant func (p *P) RepositoryName() string { return "ptrsvc" } require.NotPanics(t, func() { createDatabaseForTest("test_ptr") }) dsn := "host=localhost user=postgres password=postgres dbname=test_ptr port=5432 sslmode=disable TimeZone=Asia/Shanghai" appModule := func() core.Module { return core.NewModule(core.NewModuleOptions{ Imports: []core.Modules{ sqlorm.ForRoot(sqlorm.Config{ Dialect: postgres.Open(dsn), Models: []interface{}{&P{}}, }), sqlorm.ForFeature(sqlorm.NewRepo(P{})), }, }) } require.NotNil(t, sqlorm.InjectRepository[P](appModule())) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/go.yml(0 hunks)module.go(2 hunks)module_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/go.yml
🧰 Additional context used
🧬 Code graph analysis (1)
module_test.go (3)
module.go (4)
ForRoot(31-43)Config(20-27)ForFeature(90-113)InjectRepository(70-88)repository_test.go (2)
Abc(73-76)Abc(78-80)repository.go (1)
NewRepo(16-18)
🔇 Additional comments (2)
module.go (1)
6-6: Import looks correct.reflect is used below for optional RepositoryName lookup.
module_test.go (1)
249-270: Good coverage for custom repository naming.Nice sanity check that InjectRepository picks up RepositoryName().
|
|
||
| ctModel := reflect.ValueOf(&model).Elem() | ||
| fnc := ctModel.MethodByName("RepositoryName") | ||
| var name string | ||
| if fnc.IsValid() { | ||
| name = fnc.Call(nil)[0].String() | ||
| } else { | ||
| name = common.GetStructName(model) | ||
| } | ||
| modelName := core.Provide(GetRepoName(name)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Support pointer receivers and make reflection safe to avoid panics.
RepositoryName implemented with a pointer receiver (e.g., func (T* ) RepositoryName() string)) won’t be discovered, and fnc.Call(nil)[0] can panic if the signature differs. Handle both value/pointer receivers and validate the method signature before calling.
Apply:
-
- ctModel := reflect.ValueOf(&model).Elem()
- fnc := ctModel.MethodByName("RepositoryName")
- var name string
- if fnc.IsValid() {
- name = fnc.Call(nil)[0].String()
- } else {
- name = common.GetStructName(model)
- }
- modelName := core.Provide(GetRepoName(name))
+ var name string
+ ctModel := reflect.ValueOf(&model).Elem()
+ // Try value receiver first
+ if m := ctModel.MethodByName("RepositoryName"); m.IsValid() &&
+ m.Type().NumIn() == 0 && m.Type().NumOut() >= 1 && m.Type().Out(0).Kind() == reflect.String {
+ name = m.Call(nil)[0].String()
+ } else if m := ctModel.Addr().MethodByName("RepositoryName"); m.IsValid() &&
+ m.Type().NumIn() == 0 && m.Type().NumOut() >= 1 && m.Type().Out(0).Kind() == reflect.String {
+ name = m.Call(nil)[0].String()
+ } else {
+ name = common.GetStructName(model)
+ }
+ modelName := GetRepoName(name)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In module.go around lines 72 to 81, the reflection logic currently misses
methods with pointer receivers and can panic when calling methods with
unexpected signatures; update it to first obtain both the value and pointer
reflect.Values (ensure the value is addressable: v := reflect.ValueOf(model); if
v.Kind()==ptr use v.Elem(); pv := v; if !pv.CanAddr() {pv =
reflect.New(v.Type()) ; pv.Elem().Set(v)} then check for RepositoryName on pv
(pointer) and v (value) in that order), verify the method IsValid and that
method.Type().NumIn() == 0 and method.Type().NumOut() == 1 and
method.Type().Out(0).Kind() == reflect.String before calling, and only call when
those conditions hold; otherwise fall back to common.GetStructName(model) to
avoid panics.
2 participants
No description provided.