seancorfield / next-jdbc

A modern low-level Clojure wrapper for JDBC-based access to databases.
https://cljdoc.org/d/com.github.seancorfield/next.jdbc/
Eclipse Public License 1.0
755 stars 90 forks source link

Get datasource from #next.jdbc.default_options.DefaultOptions #193

Closed taraktikos closed 2 years ago

taraktikos commented 2 years ago

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior (preferably code -- or link to a GitHub repo containing a small, self-contained repro example):

  1. Enable specs (specs/instrument)
  2. Wrap a Datasource with custom options (def db (jdbc/with-options (connection/->pool HikariDataSource {:jdbcUrl "URL") jdbc/unqualified-snake-kebab-opts))
  3. get datasouce from instance (jdbc/get-datasource db)

Expected behavior HikariDataSourceProxy should returns

Stacktraces Execution error - invalid arguments to next.jdbc/get-datasource at (core.clj:667).

next.jdbc.default_options.DefaultOptions{:connectable #next.jdbc.sql_logging.SQLLogging{:connectable #object[com.zaxxer.hikari.HikariDataSource 0x4809f391 "HikariDataSource (null)"], :sql-logger #object[app.db.postgres$start$fn13144 0x52fa3e5d "app.db.postgres$start$fn13144@52fa3e5d"], :result-logger #object[app.db.postgres$start$fn13147 0x7c479c59 "app.db.postgres$start$fn13147@7c479c59"]}, :options {:column-fn #object[camel_snake_kebab.core$GT_snake_case 0x4e1b882f "camel_snake_kebab.core$GT_snake_case@4e1b882f"], :table-fn #object[camel_snake_kebab.core$GT_snake_case 0x4e1b882f "camel_snake_kebab.core$GT_snake_case@4e1b882f"], :label-fn #object[camel_snake_kebab.core$GT_kebab_case 0x49897f06 "camel_snake_kebab.core$GT_kebab_case@49897f06"], :qualifier-fn #object[camel_snake_kebab.core$GT_kebab_case 0x49897f06 "camel_snake_kebab.core$GT_kebab_case@49897f06"], :builder-fn #object[next.jdbc.result_set$as_unqualified_kebab_maps 0x4a82b8bc "next.jdbc.result_set$as_unqualified_kebab_maps@4a82b8bc"]}} - failed: (contains? % :dbtype) at: [:spec :db-spec] spec: :next.jdbc.specs/db-spec-map

Maybe extending ::db-spec can help

(s/def ::sourceable #(satisfies? p/Sourceable %))

(s/def ::db-spec (s/or :db-spec    ::db-spec-map
                       :jdbc-url   ::jdbc-url-map
                       :string     ::jdbcUrl
                       :ds         ::datasource
                       :sourceable ::sourceable))
seancorfield commented 2 years ago

It turns out that I actually foresaw this being a problem when I wrote the Specs: the next.jdbc.specs docstring says:

The connectable argument is currently just any? but both get-datasource and get-connection have stricter specs. If you extend Sourceable or Connectable, those specs will likely be too strict.

And this is true for the default options wrapper and the logging wrapper. The issue with extending the Spec is that Associative (hash map), String, and DataSource already satisfy Sourceable and, right now, both get-datasource and get-connection are specified to accept that ::db-spec "type". The Specs have a notion of ::connectable but treat it as any? rather than satisfying a protocol, and the same goes for ::transactable. The actual Connectable protocol is already extended to Object so that the any? -> Sourceable -> Connectable path works. The Specs were only ever intended to be a development aid for basic code: writing Specs that are "accurate" in broader cases means you'll lose some of the useful hints that Specs can provide (i.e., what fields are expected in a db-spec hash map).

I'll have to give this some thought. The Specs are (deliberately) too strict for the use case you have and this does reinforce my feeling that I should never have given in to pressure from users about adding support for default options in the first place!