Fix generics model pointer usage and add query APIs#7730
Fix generics model pointer usage and add query APIs#7730MarlinKuhn wants to merge 14 commits intogo-gorm:masterfrom
Conversation
|
Fix generics model pointer usage and add query APIs This PR updates the generics query helpers to use pointer models ( Key Changes• Switches generics model usage to pointers (e.g., Affected Areas• This summary was automatically generated by @propel-code-bot |
There was a problem hiding this comment.
Join handling may drop subqueries, requiring adjustment for correctness.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- Handle subquery joins in Join to avoid silent drops:
generics.go
Review Details
📁 1 files reviewed | 💬 1 comments
👍 / 👎 individual comments to help improve reviews for you
| } | ||
| } | ||
|
|
||
| join := clause.Join{ |
There was a problem hiding this comment.
[Logic] Join ignores jt.Subquery. If callers use JoinTarget.AssociationFrom or populate Subquery, this will silently drop the subquery and join on jt.Association as a table name. Is that intentional? If subquery joins should work here, add handling similar to Joins (building join.Expression with a NamedExpr over jt.Subquery).
Context for Agents
`Join` ignores `jt.Subquery`. If callers use `JoinTarget.AssociationFrom` or populate `Subquery`, this will silently drop the subquery and join on `jt.Association` as a table name. Is that intentional? If subquery joins should work here, add handling similar to `Joins` (building `join.Expression` with a `NamedExpr` over `jt.Subquery`).
File: generics.go
Line: 452…proved query building
…proved query flexibility
There was a problem hiding this comment.
Review found a critical logic bug where unscoped behavior isn’t applied in new preload helper.
Status: Action Required | Risk: High
Issues Identified & Suggestions
- Unscoped ignores returned DB; assign to apply unscoped query:
generics.go
Review Details
📁 1 files reviewed | 💬 1 comments
👍 / 👎 individual comments to help improve reviews for you
| } | ||
|
|
||
| func (q *preloadBuilder) Unscoped() PreloadBuilder { | ||
| q.db.Unscoped() |
There was a problem hiding this comment.
[Logic] The new PreloadBuilder.Unscoped() doesn't actually apply unscoped behavior. db.Unscoped() returns a new *DB instance, but the result is ignored here, so the preload query remains scoped.
Fix: assign the returned *DB back to q.db.
| q.db.Unscoped() | |
| q.db = q.db.Unscoped() |
Context for Agents
The new `PreloadBuilder.Unscoped()` doesn't actually apply unscoped behavior. `db.Unscoped()` returns a new *DB instance, but the result is ignored here, so the preload query remains scoped.
Fix: assign the returned *DB back to `q.db`.
```suggestion
q.db = q.db.Unscoped()
```
File: generics.go
Line: 373…ions in generics.go
There was a problem hiding this comment.
One logic concern: switching to UpdateColumns bypasses update hooks and callbacks.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- UpdateColumns skips update hooks; use Updates to keep callbacks:
generics.go
Review Details
📁 1 files reviewed | 💬 1 comments
👍 / 👎 individual comments to help improve reviews for you
| var r T | ||
| res := s.c.g.apply(ctx).Model(r).Clauses(clause.Set(s.assigns)).Updates(map[string]interface{}{}) | ||
| r := new(T) | ||
| res := s.c.g.apply(ctx).Model(r).Clauses(clause.Set(s.assigns)).UpdateColumns(map[string]interface{}{}) |
There was a problem hiding this comment.
[Logic] UpdateColumns skips update hooks and Before/AfterUpdate callbacks. The prior Updates(...) path would run hooks, and the PR intent mentions fixing Set with hooks. With this change, Set(...).Update() will not trigger hooks anymore.
If hooks should still run, switch back to Updates so callbacks execute.
| res := s.c.g.apply(ctx).Model(r).Clauses(clause.Set(s.assigns)).UpdateColumns(map[string]interface{}{}) | |
| res := s.c.g.apply(ctx).Model(r).Clauses(clause.Set(s.assigns)).Updates(map[string]interface{}{}) |
Context for Agents
`UpdateColumns` skips update hooks and `Before/AfterUpdate` callbacks. The prior `Updates(...)` path would run hooks, and the PR intent mentions fixing `Set` with hooks. With this change, `Set(...).Update()` will not trigger hooks anymore.
If hooks should still run, switch back to `Updates` so callbacks execute.
```suggestion
res := s.c.g.apply(ctx).Model(r).Clauses(clause.Set(s.assigns)).Updates(map[string]interface{}{})
```
File: generics.go
Line: 800…ions using clause.Expression
There was a problem hiding this comment.
One important logic issue found regarding join association assignment on error.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- Avoid overwriting association on table-name error; return early:
generics.go
Review Details
📁 3 files reviewed | 💬 1 comments
👍 / 👎 individual comments to help improve reviews for you
| if jt.Model != nil { | ||
| tableName, err := schema.TableName(jt.Model, db.cacheStore, db.NamingStrategy) | ||
| if err != nil { | ||
| db.AddError(errors.New("failed to get table name from model")) | ||
| } | ||
|
|
||
| jt.Association = tableName | ||
| } |
There was a problem hiding this comment.
[Logic] If schema.TableName returns an error, this still overwrites jt.Association with an empty string, which then produces a join on an empty table name (and discards any association the caller may have set). This will generate invalid SQL while only recording the error. Return early or only set jt.Association on success.
| if jt.Model != nil { | |
| tableName, err := schema.TableName(jt.Model, db.cacheStore, db.NamingStrategy) | |
| if err != nil { | |
| db.AddError(errors.New("failed to get table name from model")) | |
| } | |
| jt.Association = tableName | |
| } | |
| if jt.Model != nil { | |
| tableName, err := schema.TableName(jt.Model, db.cacheStore, db.NamingStrategy) | |
| if err != nil { | |
| db.AddError(errors.New("failed to get table name from model")) | |
| return db | |
| } | |
| jt.Association = tableName | |
| } |
Context for Agents
If `schema.TableName` returns an error, this still overwrites `jt.Association` with an empty string, which then produces a join on an empty table name (and discards any association the caller may have set). This will generate invalid SQL while only recording the error. Return early or only set `jt.Association` on success.
```suggestion
if jt.Model != nil {
tableName, err := schema.TableName(jt.Model, db.cacheStore, db.NamingStrategy)
if err != nil {
db.AddError(errors.New("failed to get table name from model"))
return db
}
jt.Association = tableName
}
```
File: generics.go
Line: 497
What did this pull request do?
Fixes #7713
User Case Description
If you use generics and want to use the Set to update values on a model with hooks it fails currently.