Refactor connection factory

[bcardiff]

NOTE: This PR is a breaking-change for drivers implementors, but should not be for driver consumers.

The goal of this PR is to decouple the URI and ConnectionContext structures from the connection initialization.

Driver implementors might be little bit more convoluted since they now will need to return a DB::ConnectionBuilder Proc(DB::Connection) which usually requires an explicit upcast.

A Database can now be created more easily with a public API method which enables more complex scenario as #162 and also allows frameworks to create pooled database from other structures than URI.

Pool and Connection configuration have now their own structures which allows simpler overrides for defaults.

crystal-db will still support URI as first class connection strings. That is not going away.

Pending: update docs done!

[bcardiff]

I have already an update for crystal-pg at will/crystal-pg@master...bcardiff:crystal-pg:refactor-connection-factory

With those changes the example of #162 to setup tunneled connection becomes:

require "pg"
require "ssh2"

# ssh into the postgres server
session = SSH2::Session.connect("p.someid.db.postgresbridge.com", 22)
session.login_with_pubkey "username", "./key", "./key.pub"

# create a proc that returns an IO talking to the postgres socket
connect_proc = -> do
  channel = session.open_session
  channel.command("nc -U /var/run/postgresql/.s.PGSQL.5432")
  channel.as IO
end

driver = PG::Driver.new

# the host part is ignored, but user and database name all work normally
uri = URI.parse("postgres://my_pg_user@whatever/db_name")
conn_info = PQ::ConnInfo.new(uri)

# connection_options and pool_options return regular records. They don't need to come from http params.
params = HTTP::Params.parse(uri.query || "")
connection_options = driver.connection_options(params)
driver_options = driver.pool_options(params)

# instead of DB.open, start the connection manually since that's the only way
# to pass in the new proc
db = DB::Database.new(connection_options, driver_options) do
  channel = connect_proc.call
  connection = PQ::Connection.new(channel, conn_info)
  PG::Connection.new(connection_options, connection)
end

p db.query_one("select current_user || now()", &.read)
db.close

cc: @will

[bcardiff]

sqlite3 branch: https://github.com/crystal-lang/crystal-sqlite3/compare/refactor-connection-factory
mysql branch: https://github.com/crystal-lang/crystal-mysql/compare/refactor-connection-factory

[will]

This way seems good! The changes to src/pg/connection.cr in crystal-pg make it more clear to me than reading this PR in isolation.

I found the current context way a bit confusing to figure out how to modify, whereas splitting it into options + conninfo or a connection is pretty straight forward in comparison.

Sorry I forgot to leave a comment on the previous way.

[bcardiff]

Yeah, the context is a bit convoluted. But is needed mostly to auto-release the connection into the pool. Yet that is internal and should not bother if you want to use the connection directly. I think that with this PR we hide that a bit better and the default should play nice.

The sqlite and mysql PRs shows how we can use custom configuration for each driver, in different structures. But we also allows eagerly parsing those and once for the whole pool. Which seems neat.

Thanks for the feedback @will 👍

[beta-ziliani]

Looks good! I just have a little concern/question; definitively not a blocker.

We should track drivers out there and make the corresponding PRs, and announce in the forum the change in advance before merging.

[bcardiff]

The only pending driver I'm aware of id cassandra. They are listed in the readme. For the others I already have the branches changed.

[straight-shoota]

There's also an ODBC driver: https://github.com/naqvis/crystal-odbc

[straight-shoota]

This looks good overall 👍

I'm wondering if Proc(DB::Connection) is an adequate abstraction for a connection builder. Maybe it might be too limiting in case we encounter a need to refine the interface in the future. A dedicated factory type would be more flexible.
I don't have a clear use case in mind. I just want us to think about whether there could be a reasonable demand for having different ways of building a connection, or passing in some kind of configuration from the driver when building it.
I'm totally fine with keeping Proc(DB::Connection) if we're sure it's good enough.

[beta-ziliani]

Perhaps a NamedTuple(connection : DB::Connection)? then we can extend it and existing code just extracting the connection from it will still work. This said, if Brian hasn't found the need in the different drivers he updated, I doubt there's a strong need for it.

[bcardiff]

The only benefit I see in introducing something like:

  abstract class ConnectionBuilder
    abstract def build : Connection
  end

is that we don't need to explicitly upcast the return type in each driver.

A sample implementation that mimics the behavior of this PR is to parse the uri when creating the builder and initializing the connection in the build method.

  class ConnectionBuilder < DB::ConnectionBuilder
    @options : DB::Connection::Options

    def initialize(driver : DummyDriver, uri : URI)
      params = HTTP::Params.parse(uri.query || "")
      @options = driver.connection_options(params)
    end

    def build : DB::Connection
      DummyConnection.new(@options)
    end
  end

Any change on the builder is likely to be a breaking change so I was not worry to change from Proc to Builder class in the future if needed.

It's a bit more verbose to use a builder class, but is slightly more elegant as we avoid the explicit upcast in drivers.

Thoughts?

[beta-ziliani]

The lack of upcasting is definitively an interesting point of that proposal, so a 👍 from me.

[bcardiff]

Refactor to use a ConnectionBuilder has been pushed and mysql, pg, sqlite3 implementations are ready to be PRed once this is released.

[beta-ziliani]

Just a little fix of a comment. After that, it's good to go.