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

Julio-Guerra · 2020-11-09T15:12:59Z

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:

detect a driver has been wrapped
unwrap the driver until it implements a given interface
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 :-)

The text was updated successfully, but these errors were encountered:

ianlancetaylor · 2020-11-11T18:12:42Z

CC @kardianos

kardianos · 2020-11-15T13:30:50Z

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

rsc · 2020-12-02T18:47:37Z

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?

frioux · 2020-12-03T00:04:11Z

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 · 2020-12-08T12:18:31Z

Related: #18080

rsc · 2020-12-09T18:15:02Z

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 · 2020-12-09T18:33:04Z

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 · 2020-12-09T19:41:22Z

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 · 2020-12-09T20:04:24Z

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 · 2020-12-09T21:53:16Z

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 · 2020-12-10T07:41:01Z

... 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 · 2020-12-10T14:06:11Z

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 · 2020-12-10T15:15:05Z

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

In the same way that you have code mapping drivers to dialects, you end up with code that can do more and more accurate detection. The same thing needs to be done even within a single driver if you care about different support as a single engine evolves. I'm not sure describing this in more depth is helpful for this discussion any more. If someone is interested let me know and I can point at some resources.

…

On Wed, Dec 9, 2020, 11:41 PM Egon Elbre ***@***.***> wrote: ... 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? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#42460 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAB7Y6RJQW5BTRSDR7WT3DSUB3RZANCNFSM4TPO5SIQ> .

rsc · 2020-12-16T18:55:19Z

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 · 2021-01-06T22:03:03Z

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

rsc · 2021-01-13T19:22:50Z

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

gopherbot added this to the Proposal milestone Nov 9, 2020

gopherbot added the Proposal label Nov 9, 2020

Julio-Guerra mentioned this issue Nov 9, 2020

Define a SQL driver unwrapping method elastic/apm-agent-go#848

Closed

ianlancetaylor added this to Incoming in Proposals (old) Nov 11, 2020

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

Julio-Guerra mentioned this issue Nov 12, 2020

Proposal: Define a SQL driver unwrapping method opencensus-integrations/ocsql#46

Open

rsc moved this from Incoming to Active in Proposals (old) Dec 2, 2020

rsc mentioned this issue Dec 2, 2020

proposal: review meeting minutes #33502

Open

rsc moved this from Active to Likely Decline in Proposals (old) Jan 6, 2021

rsc added the Proposal-FinalCommentPeriod label Jan 6, 2021

rsc moved this from Likely Decline to Declined in Proposals (old) Jan 13, 2021

rsc removed the Proposal-FinalCommentPeriod label Jan 13, 2021

rsc closed this as completed Jan 13, 2021

golang locked and limited conversation to collaborators Jan 13, 2022

gopherbot added the FrozenDueToAge label Jan 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Julio-Guerra commented Nov 9, 2020

ianlancetaylor commented Nov 11, 2020

kardianos commented Nov 15, 2020

rsc commented Dec 2, 2020

frioux commented Dec 3, 2020

kardianos commented Dec 8, 2020

rsc commented Dec 9, 2020

frioux commented Dec 9, 2020

nemith commented Dec 9, 2020

egonelbre commented Dec 9, 2020

frioux commented Dec 9, 2020

egonelbre commented Dec 10, 2020

Julio-Guerra commented Dec 10, 2020 •

edited

frioux commented Dec 10, 2020 via email

rsc commented Dec 16, 2020

rsc commented Jan 6, 2021

rsc commented Jan 13, 2021

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

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

Comments

Julio-Guerra commented Nov 9, 2020

Problem

Proposed solution

ianlancetaylor commented Nov 11, 2020

kardianos commented Nov 15, 2020

rsc commented Dec 2, 2020

frioux commented Dec 3, 2020

kardianos commented Dec 8, 2020

rsc commented Dec 9, 2020

frioux commented Dec 9, 2020

nemith commented Dec 9, 2020

egonelbre commented Dec 9, 2020

frioux commented Dec 9, 2020

egonelbre commented Dec 10, 2020

Julio-Guerra commented Dec 10, 2020 • edited

frioux commented Dec 10, 2020 via email

rsc commented Dec 16, 2020

rsc commented Jan 6, 2021

rsc commented Jan 13, 2021

Julio-Guerra commented Dec 10, 2020 •

edited