Skip to content

fix: implement sql/driver.Pinger interface #57

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.

Sign up for GitHub

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
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: implement sql/driver.Pinger interface #57

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions sqlhooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,13 @@ func (conn *Conn) PrepareContext(ctx context.Context, query string) (driver.Stmt
func (conn *Conn) Prepare(query string) (driver.Stmt, error) { return conn.Conn.Prepare(query) }
func (conn *Conn) Close() error { return conn.Conn.Close() }
func (conn *Conn) Begin() (driver.Tx, error) { return conn.Conn.Begin() }
func (conn *Conn) Ping(ctx context.Context) error {
p, ok := conn.Conn.(driver.Pinger)
if !ok {
return driver.ErrSkip
Copy link
Collaborator

Choose a reason for hiding this comment

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

ErrSkip is not documented as something supported by Ping. Reading the documentation of driver this error is only allowed to be returned when explicitly documented as supported for that function.

To be honest, to support Ping here properly is quite a bit of effort. However, its pretty bad that sqlhooks doesn't implement Ping since pretty much everyone should be supporting it.

So either we do the hard work of correctly supporting Ping, or we document a contract that Pinger has to be supported by whatever we wrap?

Copy link
Owner

Choose a reason for hiding this comment

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

ErrSkip is not documented as something supported by Ping. Reading the documentation of driver this error is only allowed to be returned when explicitly documented as supported for that function.

As per the docs:

// ErrSkip may be returned by some optional interfaces' methods to
// indicate at runtime that the fast path is unavailable and the sql
// package should continue as if the optional interface was not
// implemented.

What I understand is that the optional interface Ping implementation should return ErrSkip when not implemented by the underlying driver.

I'd love to understand better what you mean @keegancsmith .

Also, assuming most drivers already implement Ping, what would be the downside of keeping this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

From https://pkg.go.dev/database/sql/[email protected]#pkg-variables

ErrSkip may be returned by some optional interfaces' methods to indicate at runtime that the fast path is unavailable and the sql package should continue as if the optional interface was not implemented. ErrSkip is only supported where explicitly documented.

Note the mention of explicitly documented. Then look at the documentation of Pinger and it is not explicitly documented unlike some other optional interfaces.

The downside of keeping this change is the call sites of Ping inside of driver will fail rather than handling ErrSkip for drivers that do not support Pinger. I took a quick look at the code and think non-pinger supported drivers will be broken.

However, I think this project should support this interface given in practice I assume all drivers support it.

Copy link
Owner

Choose a reason for hiding this comment

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

@wburningham are you aware of any sql driver that does not implement the Pinger interface so that we can test the ErrSkip behavior and see how bad the damage will be?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not aware of any, but commenting out the Ping method of a sqllite driver should be sufficient for local testing.

Copy link
Owner

Choose a reason for hiding this comment

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

is that something you can try and share the outcome?

}
return p.Ping(ctx)
}
func (conn *Conn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) {
return conn.Conn.(driver.ConnBeginTx).BeginTx(ctx, opts)
}
Expand Down
Loading