Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make jdbc-url and connection-uri optional #10

Closed
lucassousaf opened this issue Nov 25, 2022 · 4 comments
Closed

Make jdbc-url and connection-uri optional #10

lucassousaf opened this issue Nov 25, 2022 · 4 comments

Comments

@lucassousaf
Copy link

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:

  • adapter
  • server-name
  • port-number
  • username
  • password
  • database-name
  • ...

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
Copy link
Contributor

That sounds fine to me.

bgalartza added a commit to gethop-dev/database.sql.hikaricp that referenced this issue Nov 28, 2022
Details in Issue 10 of the original repository [1]

[1] duct-framework#10
bgalartza added a commit to gethop-dev/database.sql.hikaricp that referenced this issue Nov 28, 2022
Details in Issue 10 of the original repository [1]

[1] duct-framework#10
@lucassousaf
Copy link
Author

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:
gethop-dev@e7f3015

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

What do you think?

@weavejester
Copy link
Contributor

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

@iarenaza
Copy link

We just created the associatd PR: #15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants