all: add support for cockroachdb

[lopezator]

add support for cockroachdb

Related MR

This would need to be merged before: ory/hydra#1348

Proposed changes

Add support for cockroachdb in x.

[lopezator]

There is some tricky game here, because the used DSN: cockroach:// and the real DSN to use in lib/pq: postgres:// have to be converted in many places, but I implemented in the same way as mysql:// prefix adding and removal is done inside the code, to keep the consistency with the current code and without changing it too much.

CC \ @aeneasr

[lopezator]

PTAL @aeneasr

[aeneasr]

Thank you for this PR and your hard work!

I'm wondering if introducing a wrapper to sqlx.DB is really the best solution here? First we introduce a couple of breaking changes which will require updates in all dependent code and second I'm not entirely sure why we're wrapping e.g. NamedExecContext but not other methods?

I'm not sure if I'm missing something very critical here, but wouldn't it be enough to just update this method so that it, for input cockroach://foo:[email protected]:1234/db?k=v returns postgres://foo:[email protected]:1234/db?k=v? That should solve all downstream issue, right?

The only issue I see is in migrations where we're using the db.DriverName() to get the proper migration (example from hydra):

 migrate.Exec(m.DB.DB, m.DB.DriverName(), Migrations[dbal.Canonicalize(m.DB.DriverName())], migrate.Up)

However, for that we could probably come up with an idea to solve that another way (e.g. using the configuration provider to get the full DSN, or by updating the CreateSchema() method to CreateSchema(driver string, db *sqlx.DB)).

What do you think?

[lopezator]

Wrapper removed, PTAL @aeneasr

[aeneasr]

Nice! This looks much cleaner I think - what do you think? I have to run this locally to assure that everything works as expected but I don't think that there will be any changes necessary!

[lopezator]

Yep, agree, easier to understand. I don't have more changes in mind, so if you are ok with this, me too!

[aeneasr]

Great work! Just a few nitpics :)

[aeneasr]

Shouldn't that be cockroach?

[lopezator]

It can't be cockroach, because we need the "real" dsn (from GO driver perspective) inside the bootstrap function, for a successful sql.Open to happen.

I just applied the same logic it's been applied with mysql (send the DSN without mysql://, then add it after the bootstrap function).

Unless I'm missing something here the only thing we can do it's just change the place of the replace to inside the boostrap function with a switch/case (for mysql and cockroach) instead of doing it individually inside bootstrapMySQL() and bootstrapCockroach().

See here:

https://github.com/ory/x/blob/master/sqlcon/connector_test.go#L299-L302

And here:

https://github.com/ory/x/blob/master/sqlcon/connector_test.go#L337

[lopezator]

Changes applied, PTAL and tell some if you find something more @aeneasr !

[aeneasr]

THank you so much!