Proof-of-concept of passing an interface rather than *sql.DB to database drivers

[jszwedko]

Hey all,

In our application, we encapsulate our sql.DB handles behind an interface so that we can wrap an actual *sql.DB with a type that handles things such as logging, collecting metrics, and reporting errors. We'd like to use this same wrapped handle with golang-migrate but the fact that the postgres driver take a literal *sql.DB type makes this not possible.

As a proof-of-concept, I've updated the database/postgres driver to have a WithInterface() method which takes a postgres.DB interface type instead of an *sql.DB. I'm curious to get any feedback on this approach. If you like it, I can polish it a little more and update the documentation.

I did note that the drivers packages seem independent -- they just return something satisfying the driver.Driver interface -- so it seems like we could make this change just in the Postgres driver for now (which is the one we depend on).

Thanks!

[coveralls]

Pull Request Test Coverage Report for Build 736

• 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.06%) to 53.238%

---

Totals
Change from base Build 734:	0.06%
Covered Lines:	2573
Relevant Lines:	4833

---

💛 - Coveralls

[dhui]

I'm all for using interfaces, but there isn't an interface in database/sql yet, so I'm hesitant to create and use our own which may differ from any future interfaces in database/sql.

FYI, there are talks about adding interfaces to database/sql here

Once there are interfaces in database/sql, I'm all for updating all of our drivers to use interfaces.
In the meanwhile, you could:

1. Try to create your own postgres DB driver that embeds the migrate postgres one, but I'm not sure if that'll work with interfaces.
2. Fork migrate with this change, but it is a pain to maintain a fork...

[jszwedko]

Thanks for the thoughts!

I can definitely understand wanting to use interfaces defined by database/sql that cover all functionality supported by sql.DB, but I also think it is typical for packages to define narrow interfaces that describe exactly the set of functionality they need. For example, see sqlx: https://godoc.org/github.com/jmoiron/sqlx#Preparer that is used with sqlx.Preparex. This allows me to use any type implementing that method which is useful for testing or for wrapping a sql.DB with something that does logging or tracing.

The trade-off of narrow interfaces is, of course, that making the interface wider (if migrate ends up needing more functionality from the passed database handle) this would be a backwards incompatible change.

golang/go#7898 seems to be about supporting nested transactions and save points rather than defining a generic interface on top of database/sql.DB. It discusses defining another database/sql/driver interface that database drivers could implement to convey that they support nested transactions.

golang/go#14468 does seem to discuss a general interface, but doesn't seem to have much traction with the core team even though golang/go#14468 (comment) astutely points out that many libraries using database/sql expect a *sql.DB as migrate does due to the lack of an interface in the stdlib; although one could argue that each library should define its own interface for the dependency and an adapter for an sql.DB.

Given that it doesn't seem likely that this will be introduced into the stdlib anytime soon, if ever, would you consider defining the interface in migrate as I'm doing here? I could expand the interface to be as wide as possible to include other methods that migrate would not need now, but may in the future.

I don't see any other workaround aside from the ones you suggested:

• Maintaining a fork of the postgres driver. I don't think I could just embed the migrate one as it ultimately requires a concrete sql.DB
• Maintaining a fork of migrate

Neither of these seem particularly desirable.

[dhui]

The trade-off of narrow interfaces is, of course, that making the interface wider (if migrate ends up needing more functionality from the passed database handle) this would be a backwards incompatible change.

I'm fine with starting with narrow interfaces and adding additional methods later. Any such changes would be documented and any breakage due to incompatibility would be very loud and obvious. e.g. won't build

For now, I still think it's best to hold off on using our own interfaces until there are ones in the standard library or we have more use cases (e.g. reasons to use our own interfaces).
I'm not sure how we'd handle the DBs that don't use sql.DB. e.g. neo4j, mongodb, and cassandra. Maintaining different interfaces for each DB would be a pain. We probably would only support interfaces for *sql.DB.

Also, I've provided feedback to your PR should we decide to define our own interfaces and accept your PR. Thanks for all of your research and the discussion!

[dhui]

Don't think we need this. e.g. *sql.DB should work
It's probably worth documenting that *sql.DB satisfies this interface the interface should be moved up a package to migrate/database

[dhui]

We should be able to keep the same method WithInstance() and just change the param to use the interface.

[dhui]

Using an interface would also make transactions easier to implement: #196
e.g. you could chain a bunch of "middleware" or options for DB drivers
This goes against the current simple approach though...