fix: merge group conds clause #7198
Conversation
| } | ||
|
|
||
| for _, tx := range txs { | ||
| stmt := tx.Statement |
There was a problem hiding this comment.
Would it work if we make a function func (db *DB) mergeClause(tx *DB) and then call it in the loop on line 397 to cut down on iterations? db.mergeClause(scope(cleanDB))
There was a problem hiding this comment.
This PR is to avoid the impact of multiple AddClause on the db instance, so a clean stmt needs to be cached (Clauses are not affected by AddClause)
At the same time we need to keep the order of conditions, such as .Where(...).Scope(...)
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the scope execution mechanism in GORM to fix issues with scope chaining and clause merging. The changes ensure that scopes receive a clean database instance and their clauses are properly merged back to the parent statement.
- Refactored
executeScopes()to use a clean DB instance for each scope and collect results for merging - Added
mergeClauses()method to consolidate clauses from multiple scopes (WHERE, Table, Select, etc.) - Added test case to verify complex scope scenarios with multiple table assignments and grouped conditions
- Updated return value of
executeScopes()inBuildCondition()to use the returned DB instance
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| chainable_api.go | Refactored executeScopes() to use clean DB instances and added mergeClauses() helper for merging scope results; fixed documentation formatting for Unscoped() |
| statement.go | Updated BuildCondition() to use the returned value from executeScopes() instead of ignoring it |
| tests/scopes_test.go | Added test case "group_cond" to verify proper merging of multiple table assignments, WHERE clauses, and Select statements across scopes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if stmt.TableExpr != nil { | ||
| db.Statement.TableExpr = stmt.TableExpr | ||
| } | ||
|
|
There was a problem hiding this comment.
The merge logic overwrites Table with the last non-empty value from the scopes. This means if multiple scopes set different table names (like 'languages1' and 'languages2' in the test), only the last one is retained. Consider if this is intentional, as it may lead to confusion where earlier Table assignments in scopes are silently ignored. If this overwriting behavior is desired, consider adding a comment explaining that later scopes take precedence.
| // If multiple scopes set different table names, only the last non-empty value is retained. | |
| // Later scopes take precedence and overwrite earlier Table assignments. |
| db.Statement.Selects = stmt.Selects | ||
| } | ||
|
|
||
| if stmt.Omits != nil { | ||
| db.Statement.Omits = stmt.Omits |
There was a problem hiding this comment.
Both Selects and Omits are being overwritten rather than merged. If multiple scopes specify different selects or omits, only the last one will apply. This could lead to unexpected behavior where earlier scope configurations are lost. Consider either accumulating these values or documenting that later scopes override earlier ones.
| db.Statement.Selects = stmt.Selects | |
| } | |
| if stmt.Omits != nil { | |
| db.Statement.Omits = stmt.Omits | |
| if db.Statement.Selects == nil { | |
| db.Statement.Selects = []string{} | |
| } | |
| db.Statement.Selects = append(db.Statement.Selects, stmt.Selects...) | |
| } | |
| if stmt.Omits != nil { | |
| if db.Statement.Omits == nil { | |
| db.Statement.Omits = []string{} | |
| } | |
| db.Statement.Omits = append(db.Statement.Omits, stmt.Omits...) |
2 participants
What did this pull request do?
https://gorm.io/docs/advanced_query.html#Group-Conditions
Since it is not safe to reuse a db instance, group conditions will generate duplicate where conditions when used in scope.
User Case Description
refer to #6148 (comment)
close #7136