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
contrib/database/sql: fix support for drivers using deprecated interfaces #1167
Conversation
…aces Some drivers like the go-mssqldb driver do not implement newer database/sql interfaces like Queryer/QueryerContext. Previously the code would erroneously assume drivers that did not implement a QueryerContext interface would implement the Queryer interface. This lead to panics like in issue #1043.
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.
This looks really good, and should make our sql integration a lot more stable.
"github.com/go-sql-driver/mysql" | ||
"github.com/jackc/pgx/v4/stdlib" | ||
_ "github.com/lib/pq" | ||
"github.com/stretchr/testify/assert" | ||
mysqlgorm "gorm.io/driver/mysql" | ||
"gorm.io/driver/postgres" | ||
"gorm.io/driver/sqlserver" |
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.
Found the issue with the gorm dependency.
This dep pulls in a newer version of gorm.
We need to pin this to v1.3.0
which shouldn't cause a newer gorm to be pulled.
https://github.com/go-gorm/sqlserver/blob/v1.3.0/go.mod
504ee4a
to
9695935
Compare
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.
Looks great.
Some drivers like the go-mssqldb driver do not implement newer database/sql interfaces like Queryer/QueryerContext. Previously the code would erroneously assume drivers that did not implement a QueryerContext interface would implement the Queryer interface. This lead to panics like in issue #1043.
This PR fixes #1043 and is an improved version (based on PR comments) of PR #1163