Skip to content
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

proposal: database/sql: define a SQL driver unwrapper interface and helper function #42460

Closed
Julio-Guerra opened this issue Nov 9, 2020 · 16 comments
Labels
Projects
Milestone

Comments

@Julio-Guerra
Copy link

@Julio-Guerra Julio-Guerra commented Nov 9, 2020

This proposal is motivated by the common need for a standard interface defining a standard way to unwrap SQL drivers that may have been wrapped by SQL instrumentation packages.

Problem

Such SQL driver wrapper types hide every other interface the wrapped type may implement. For example, database/sql/driver defines many optional interfaces that would be hidden by the following wrapper type definition:

type myDriverWrapper struct {
	driver.Driver
}

func Wrap(drv driver.Driver) driver.Driver {
	return myDriverWrapper{drv}
}

With this straightforward type definition, myDriverWrapper only implements interface driver.Driver, no matter what the actual driver may implement besides the driver.Driver interface.

SQL dialect detection functions based on the package path name gotten with reflect (with reflect.TypeOf(db.Driver()).PkgPath()) will also break when using the wrapped driver.

Proposed solution

Similarly to the Unwrap function of package errors (https://golang.org/pkg/errors/#Unwrap), package database/sql/driver could provide the Unwrapper interface allowing a driver wrapper to return its underlying SQL driver, but also let third-party packages know about it:

type Unwrapper interface {
	Unwrap() Driver
}

// Unwrap returns the result of calling the Unwrap method on drv, if drv's
// type implemented the Unwrapper interface.
// Otherwise, Unwrap returns nil.
func Unwrap(drv Driver) Driver {
	// copied from package errors
	// Go generics could allow sharing `Unwrapper` and `Unwrap()` definitions
	if u, ok := err.(Unwrapper); ok {
		return u.Unwrap()
	}
	return nil
}

When a driver wrapper implements the Unwrapper interface, a third-party package is able to:

  1. detect a driver has been wrapped
  2. unwrap the driver until it implements a given interface
  3. get the package paths of every wrapper type, down the deepest driver which should be the actual one
    And probably other use-cases I haven't considered :-)
@gopherbot gopherbot added this to the Proposal milestone Nov 9, 2020
@gopherbot gopherbot added the Proposal label Nov 9, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 11, 2020
@ianlancetaylor ianlancetaylor changed the title proposal: define a SQL driver unwrapper interface and helper function proposal: database/sql: define a SQL driver unwrapper interface and helper function Nov 11, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 11, 2020

@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 15, 2020

I suspect this will be a "no". Have you explored a custom connector that then exposes this functionality?

@rsc
Copy link
Contributor

@rsc rsc commented Dec 2, 2020

It sounds like this is errors.Unwrap for sql.Driver. But why are sql.Drivers being wrapped so much?
What is the larger context here?

@rsc rsc moved this from Incoming to Active in Proposals Dec 2, 2020
@frioux
Copy link

@frioux frioux commented Dec 3, 2020

A reason we use sql driver wrapping at ZipRecruiter is to extract tracing information from a context.Context and inject it into the actual query as a comment to achieve basic tracing. This way if a query is taking a long time to run you can see who initiated the query. https://github.com/ngrok/sqlmw is a package that assists with this and could trivially implement the proposal here.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Dec 8, 2020

Related: #18080

@rsc
Copy link
Contributor

@rsc rsc commented Dec 9, 2020

OK, so I understand the point of having an "add tracing of SQL commands" wrapper driver.
But then the next question is: why would you need to unwrap an arbitrary wrapped driver?

In general unwrapping is not a safe operation. Supposed you had, as a dumb example, a rot13-wrapping driver that stored all the data rot13'd (and did the reverse on the way back out). If you unwrapped to the raw driver underneath, any use of the calls would break the rot13 invariant. This comes up all the time for various things the wrappers are trying to ensure. We struggled a lot with errors here and for that specific case it was important to get at the underlying ones to test for specific errors and so on. But in general - for example in the FS API that we just adopted - unwrapping is not something that is safe and should be done.

So why is unwrapping important for SQL drivers? When is it necessary, and why?

@frioux
Copy link

@frioux frioux commented Dec 9, 2020

A common pattern (across languages) is to use the driver to figure out what sql variant to use. For example pagination is different in almost all relational databases. I don't see a clear way (other than running queries against the database) to interrogate the DB object to figure out what kind of database you might be connected to. That's what's mentioned in the original post (dialect inference.)

Frankly, I would suggest that sql generators (ORMs) not infer based on connection but instead outer layers, but I bet that's the main thought here.

@nemith
Copy link
Contributor

@nemith nemith commented Dec 9, 2020

I wrote some MySQL wrappers at my last job and many ORMs allow to pass in a string or other identifier to tell it the "flavor" of the connection vs trying to type assert it. This is probably preferred since even a Unwrap method would still require modifications to the ORM/generator.

@egonelbre
Copy link
Contributor

@egonelbre egonelbre commented Dec 9, 2020

This sounds like that it should be specified when creating ORM itself rather than derived from the driver or queries. e.g. orm.FromPostgres(db).

For example, let's say there's a proxy driver, how would you query the instance to determine what dialect it's using? In principle the exact same driver/connector type could be used for different dialects.

@frioux
Copy link

@frioux frioux commented Dec 9, 2020

Well, in projects where I've come across this in the past, you start by discovering the driver, and if it's a multi-db driver (ODBC is an example of that) you then do queries against the DB to figure out what it is. Again: I'm not emphasizing that this should be done, but if you do it, this is how.

@egonelbre
Copy link
Contributor

@egonelbre egonelbre commented Dec 10, 2020

... discovering the driver, and if it's a multi-db driver ...

How do you know it's a multi-db driver in the first place?

@Julio-Guerra
Copy link
Author

@Julio-Guerra Julio-Guerra commented Dec 10, 2020

So why is unwrapping important for SQL drivers? When is it necessary, and why?

Adding our application security agent use-case too: our sql-injection detection needs to know what is the SQL dialect being used.
To do so, we read the driver package path and maintain a map of the package path's dialects (eg. github.com/mattn/go-sqlite3 is sqllite). That is the current outcome of the discussion at #12600

But when the driver is wrapped, we get the wrapper package path instead, and we can no longer infer the dialect.

Note that our use-case would be definitely better solved by a specific way of getting the dialect being used at run time.

@frioux
Copy link

@frioux frioux commented Dec 10, 2020

@rsc
Copy link
Contributor

@rsc rsc commented Dec 16, 2020

I'm not hearing a compelling use case for Unwrap. These seem like less-than-elegant uses that would not be 100% satisfied by Unwrap. Egon's comment above about orm.FromPostgres seems like a cleaner solution. (And if you want a 90% solution, instead of Unwrap you could use reflect to poke around.)

@rsc
Copy link
Contributor

@rsc rsc commented Jan 6, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals Jan 6, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals Jan 13, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Jan 13, 2021

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.