Skip to content

reafactor: add nil detection when sqldb return #6373

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

Merged
merged 4 commits into from
Jun 5, 2023

Conversation

KantaHasegawa
Copy link
Contributor

@KantaHasegawa KantaHasegawa commented Jun 3, 2023

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

This pull request addresses a bug that caused a panic when connecting to the database using an incorrect DSN. The bug has already been fixed in this commit (c1ea730). However, to prevent potential future bugs, I have refactored the code to include null detection during type assertion.

User Case Description

gorm/gorm.go

Lines 374 to 387 in c1ea730

// DB returns `*sql.DB`
func (db *DB) DB() (*sql.DB, error) {
connPool := db.ConnPool
if dbConnector, ok := connPool.(GetDBConnector); ok && dbConnector != nil {
return dbConnector.GetDBConn()
}
if sqldb, ok := connPool.(*sql.DB); ok {
return sqldb, nil
}
return nil, ErrInvalidDB
}

Before merging this PR (c1ea730), connPool.(*sql.DB) returned nil, nil which had a negative impact on the called method.

Sorry, something went wrong.

@KantaHasegawa KantaHasegawa changed the title reafactor: add null detection when sqldb return reafactor: add nil detection when sqldb return Jun 3, 2023
Copy link
Member

@a631807682 a631807682 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c1ea730 is ok to me, and this change is also the same, but in order to achieve the purpose of your appeal, we also need to detect the content returned by dbConnector.GetDBConn() to ensure that if *sql.DB is nil, an error must be returned , and you can revert the error judged code in c1ea730

Verified

This commit was signed with the committer’s verified signature.
haoqunjiang Haoqun Jiang
Copy link
Member

@a631807682 a631807682 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants