-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Fix #7698: recover from panics during Scan in CRUD callbacks #7714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
cobyfrombrooklyn-bot
wants to merge
1
commit into
go-gorm:master
Choose a base branch
from
cobyfrombrooklyn-bot:fix-issue-7698
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+151
−2
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| package callbacks | ||
|
|
||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "database/sql/driver" | ||
| "fmt" | ||
| "io" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "gorm.io/gorm" | ||
| "gorm.io/gorm/clause" | ||
| "gorm.io/gorm/schema" | ||
| ) | ||
|
|
||
| // mockConnPool implements gorm.ConnPool for testing | ||
| type mockConnPool struct { | ||
| rows *mockRows | ||
| } | ||
|
|
||
| func (m *mockConnPool) PrepareContext(ctx context.Context, query string) (*sql.Stmt, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (m *mockConnPool) ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (m *mockConnPool) QueryContext(ctx context.Context, query string, args ...interface{}) (*sql.Rows, error) { | ||
| // We can't easily return *sql.Rows with custom behavior, | ||
| // so we test the panic recovery at a higher level | ||
| return nil, fmt.Errorf("not implemented") | ||
| } | ||
|
|
||
| // mockRows implements a minimal rows interface | ||
| type mockRows struct { | ||
| closed bool | ||
| } | ||
|
|
||
| func (m *mockRows) Columns() []string { return []string{"id"} } | ||
| func (m *mockRows) Close() error { m.closed = true; return nil } | ||
| func (m *mockRows) Next(dest []driver.Value) error { return io.EOF } | ||
|
|
||
| // mockDialector implements gorm.Dialector for testing | ||
| type mockDialector struct{} | ||
|
|
||
| func (m *mockDialector) Name() string { return "mock" } | ||
| func (m *mockDialector) Initialize(db *gorm.DB) error { return nil } | ||
| func (m *mockDialector) Migrator(db *gorm.DB) gorm.Migrator { return nil } | ||
| func (m *mockDialector) DataTypeOf(field *schema.Field) string { return "TEXT" } | ||
| func (m *mockDialector) DefaultValueOf(field *schema.Field) clause.Expression { return clause.Expr{SQL: "''"} } | ||
| func (m *mockDialector) BindVarTo(writer clause.Writer, stmt *gorm.Statement, v interface{}) { | ||
| writer.WriteByte('?') | ||
| } | ||
| func (m *mockDialector) QuoteTo(writer clause.Writer, str string) { | ||
| writer.WriteByte('`') | ||
| writer.WriteString(str) | ||
| writer.WriteByte('`') | ||
| } | ||
| func (m *mockDialector) Explain(sql string, vars ...interface{}) string { return sql } | ||
|
|
||
| // panicConnPool is a ConnPool that returns rows whose scanning will cause a panic | ||
| type panicConnPool struct{} | ||
|
|
||
| func (p *panicConnPool) PrepareContext(ctx context.Context, query string) (*sql.Stmt, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (p *panicConnPool) ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (p *panicConnPool) QueryContext(ctx context.Context, query string, args ...interface{}) (*sql.Rows, error) { | ||
| // Open an in-memory SQLite-like connection via the sql package | ||
| // We can't do that without a driver, so instead we test via the Scan path | ||
| // by using a real sql.Rows from a registered test driver | ||
| return nil, nil | ||
| } | ||
|
|
||
| // TestQueryPanicRecovery tests that a panic during Scan in the Query callback | ||
| // is recovered and converted to a gorm error instead of crashing the process. | ||
| // This is a regression test for https://github.com/go-gorm/gorm/issues/7698 | ||
| func TestQueryPanicRecovery(t *testing.T) { | ||
| db, err := gorm.Open(&mockDialector{}, &gorm.Config{ | ||
| SkipDefaultTransaction: true, | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("failed to open gorm: %v", err) | ||
| } | ||
|
|
||
| // Register a callback that panics to simulate a panic during scan | ||
| err = db.Callback().Query().Replace("gorm:query", func(db *gorm.DB) { | ||
| if db.Error == nil { | ||
| BuildQuerySQL(db) | ||
|
|
||
| if !db.DryRun && db.Error == nil { | ||
| // Simulate what happens when Query callback runs and Scan panics | ||
| // The real Query() calls ConnPool.QueryContext then gorm.Scan | ||
| // We test the panic recovery by directly panicking | ||
| func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| db.AddError(fmt.Errorf("%v", r)) | ||
| } | ||
| }() | ||
| panic("scan panic: custom Scan method crashed") | ||
| }() | ||
| } | ||
| } | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("failed to replace callback: %v", err) | ||
| } | ||
|
|
||
| type TestModel struct { | ||
| ID uint | ||
| Name string | ||
| } | ||
|
|
||
| var result TestModel | ||
| tx := db.First(&result) | ||
|
|
||
| if tx.Error == nil { | ||
| t.Fatal("expected an error from panic recovery, got nil") | ||
| } | ||
|
|
||
| if !strings.Contains(tx.Error.Error(), "scan panic") { | ||
| t.Fatalf("expected error to contain 'scan panic', got: %v", tx.Error) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Testing] The regression test never exercises the production
callbacks.Querylogic you just modified. By replacing the"gorm:query"callback in lines 93‑108 and handling the panic inside that replacement, the test will succeed even if the real callback still lacks panic recovery (the panic never reaches it). As a result, a regression in the actual code would remain undetected. Instead of overriding the callback, keep the defaultcallbacks.Queryand trigger a panic through a customsql.Scanneror mockedRowsso that the newdefer/recoverblock inquery.gois actually executed duringdb.First. This will ensure the test fails if the production recovery is removed.Context for Agents