duct-framework / database.sql.hikaricp

Integrant methods for a SQL database connection pool
5 stars 6 forks source link

Make jdbc-url and connection-uri optional #10

Closed lucassousaf closed 1 year ago

lucassousaf commented 1 year ago

As the README states:

The key takes the same config options as the Clojure hikari-cp wrapper library.

Those HikariCP configuration options allow us to specify the connection details by supplying the individual options:

instead of a single jdbc-url or connection-uri. But database.sql.hikaricp assumes that there will always be a jdbc-url or connection-uri option provided. And unconditionally sets the jdbc-url option in the map passed to hikari-cp/make-datasource.

Because hikari-cp/make-datasource checks the validity of the jdbc-url option (and throws an exception if it doesn't like it), that means that even if we provide individual options that are valid and sufficient to create the datasource, we will still need to provide a fake (but still valid-looking) jdbc-url or connection-uri. Even if that fake jdbc-url/connection-uri is completely ignored by hikari-cp/make-datasource in this case.

So we propose to make jdbc-url/connection-uri completely optional. And we would be glad to provide a pull request for that change.

weavejester commented 1 year ago

That sounds fine to me.

lucassousaf commented 1 year ago

A status update on this issue. We already have the implementation however we had a problem while writing the tests for our changes. hikari-cp library had a bug when trying to use the adapter option for SQLite. We recently did a pull request, it got merged and we are waiting for a new release with our changes applied. Once that is done we'll make PR to this repository.

Just let you know in advance here is the part that changed in order to accomplish what we described in this issue: https://github.com/gethop-dev/database.sql.hikaricp/commit/e7f3015859fe36e5b4a9334441c041838adec6d0

Once we do the PR we'll give more details about the change in the commit message.

What do you think?

weavejester commented 1 year ago

That seems fine, too. Thanks for your work on this.

iarenaza commented 1 year ago

We just created the associatd PR: #15