I don't really understand how to work conveniently with connection pools in honeysql. The docs contain this example in https://cljdoc.org/d/com.github.seancorfield/next.jdbc/1.3.939/doc/getting-started?q=pool#connection-pooling
(defn -main [& args]
;; db-spec must include :username
(with-open [^HikariDataSource ds (connection/->pool HikariDataSource db-spec)]
;; this code initializes the pool and performs a validation check:
(.close (jdbc/get-connection ds))
;; otherwise that validation check is deferred until the first connection
;; is requested in a regular operation:
(jdbc/execute! ds ...)
(jdbc/execute! ds ...)
(do-other-stuff ds args)
(into [] (map :column) (jdbc/plan ds ...))))
But now I will have to pass around the data source in many function call. I would like to def a pool then easily be able to call it, something like this:
(def ds (connection/->pool db-spec))
(jdbc/execute! ds ...)
Can I do this or will I start leaking connections?How do you test things? Do you make all tests into a component?
No, you can create a test system in a fixture or in a test. Since the config is a plain map, you can easily create sub-systems and systems with mocks.
Your question is not about HoneySQL but rather about next.jdbc - different things. Usually questions about the latter go into #sql.
> now I will have to pass around the data source in many function call
It's the case even if you don't use a connection pool - you still have to pass around a data source.
It's possible to def the pool somewhere, but I would definitely advice against it.
Right, sorry. I'm setting up both honeysql and next.jdbc at the same time, therefore the mix-up. Why do I need to pass around a data source?
(def ds (connection/->pool HikariDataSource db-spec))
(defn exec [sql]
(jdbc/execute! ds (format sql))
;;;later in the program
(exec {:select [:*] :from [:foo]})
What are the disadvantages of def'ing the pool?Side-effects at namespace loading time make AOT compilation much harder to implement and less reliable. It makes things harder to test. You'd have to refactor it all if you ever want to connect to two DBs at the same time. It also makes the "REPL driven development" and the "reloaded" workflow both harder and less reliable.
Those are just the things I could recall right away.
One might think that using a dynamic variable instead of a plain def would solve most of those issues. But that's not really the case. It's a lesson learned by many in practice, and precisely the reason for why passing ds around is the recommended approach.
Thank you, I'm convinced. I'll do it that way 🙂
How do you work with this in the REPL? Since it's not accessible outside of main.
The example above is not very conducive for reusing the data source in a REPL, yes.
I myself am a staunch follower of the "reloaded" workflow. Currently I use Integrant, so in order to get a particular data source in a REPL all I need to do is call (get integrant.repl.state/system :the-data-source-key).
Without the "reloaded" workflow - not sure. Maybe you'd conditionally set some atom that exists only in a dev environment.
I guess it's time to start using Integrant 🙂
There are other choices as well. Just don't use Mount. :D It relies on singletons internally so running a dev app and a test system side by side becomes much harder, if not impossible.
Component is another popular choices.
juxt/clip also looks interesting. Perhaps this is what I'd use in a new project.
As mentioned above, you need to drive your app with some sort of system of components that change their state on demand. Good examples are Component, Integrant, or Mount (although the latter relies on the global state). Now that you have a component that depends on a pool object, do something like that:
(jdbc/on-connection [conn pool]
(jdbc/execute! conn ...)
(jdbc/execute! conn ...))
by exiting from the on-connection macro, the connection will be returned to the pool.Thank you both for the advice. I now have this
(defmethod ig/init-key :db/ds
[_ _]
(connection/->pool HikariDataSource db-spec))
(defmethod ig/halt-key! :db/ds
[_ ds]
(.close (jdbc/get-connection ds)))
(def system (ig/init {:db/ds db-spec}))
(jdbc/on-connection [conn (system :db/ds)]
(jdbc/execute! conn (hon/format {:select [:*] :from [:foo] :limit 10}))) ;;<- clojure is magic?
But doesn't this solve the initial question? Now I don't have to pass datasource around all the time, I can access it with (system :db/ds) .
Or am I misunderstanding something?You're trading one singleton for another, it doesn't solve anything.
You should never use the system value directly, apart from some dev things.
Instead, you should organize your code into separate components where each component that needs ds receives it via its config.
So somewhere you'd have something like
(defmethod ig/init-key :routes/index
[_ {:keys [db-src]}]
(jdbc/on-connection [conn db-src]
...))
and somewhere in the config you'd have
{...
:db/ds {...}
:routes/index {:db-src #ig/ref :db/ds}
...}> You're trading one singleton for another, it doesn't solve anything. That was my suspicion! I don't have any other components than the database connection, this program is just running on a computer and moving data around on a daily schedule. It's just a few functions that need database access, but to a large extent this is me trying to learn the proper way to do things. I realize you just said "never use the system value directly", but this is my current understanding which is clearly incorrect:
(defmethod ig/init-key :db/ds
[_ _]
(connection/->pool HikariDataSource db-spec))
(defmethod ig/halt-key! :db/ds
[_ ds]
(.close (jdbc/get-connection ds)))
(def system (ig/init {:db/ds db-spec})) ;; <-- How do I not do this?
(defn dostuff [ds]
(jdbc/on-connection [conn ds]
(jdbc/execute! conn (hon/format {:select [:*] :from [:foo] :limit 10}))))
(defn -main
"Start the application server and run the application"
[& args]
(with-open
[^HikariDataSource ds (system :db/ds)] ;; <--- how do I not do this?
(dostuff ds)
(.close (jdbc/get-connection ds))))
And thank you for the directions so far, this is very unfamiliar to me.
You can turn dostuff into a component.
Also, the way you work with ds looks very weird.
Here's a simplified version of what I have:
(defmethod ig/init-key ::src
[_ options]
(hikari/make-datasource options))
(defmethod ig/halt-key! ::src
[_ db-src]
(try
(hikari/close-datasource db-src)
(catch Exception _
;; Just swallow the exception - there's nothing reasonable that we can do here.
)))
AFAICT, there's no need to use with-open, to hint with HikariDataSoruce, or to call (.close (jdbc/get-connection ...)).
That ::src is then passed around by Integrant and I use it as db-src in (next.jdbc/with-transaction [c db-src] ...).Thank you!
(Simplified to remove the leftover deref that's not needed there.)
Note that next.jdbc has built-in support for HikariCP and Component.
on-connection is intended for when you don't know whether you'll be given a Connection or a "connectable" so it is generally not needed: if you're working from a connection pooled datasource, it is recommended to use with-open and get-connection (on the datasouce) so that your intent is clearer.
I had some more time today after a few busy days with other work. Does this code seem sound to you, have I got everything right? Again, this is a batch job program that runs indefinitely.
There is one batch job running - testquery. It is started on the ::schedule component which depends on the ::src component.
ig/init is in the function start-system which I run from the command line.
(def config
{::src {}
::schedule {:datasource (ig/ref ::src)}})
(defmethod ig/init-key ::src
[_ _]
(info "creating datasource")
(hikari/make-datasource db-spec))
(defmethod ig/halt-key! ::src
[_ db-src]
(info "closing datasource")
(try
(hikari/close-datasource db-src)
(catch Exception _)))
(defn testquery [ds]
(jdbc/with-transaction [conn ds]
(jdbc/execute! conn
(hon/format {:update :foo
:set {:timestamp [:raw "current_timestamp"]}
:where [:= :id 0]}))))
(defmethod ig/init-key ::schedule [_ {:keys [datasource]}]
(info "Setting schedules")
(let [s (chime/periodic-seq (-> (LocalTime/of 12 35 0)
(.adjustInto
(ZonedDateTime/now (ZoneId/of "Europe/Stockholm")))
.toInstant)
(Period/ofDays 1))]
(chime/chime-at s (fn [_time]
(testquery datasource)))))
(defn start-system [& args]
(ig/init config))
So at this point, I shouldn't be leaking connections, or be messing up with singletons, etc?
I haven't tried the reloaded workflow yet, but that will come once/if I get this right.Yes, this is exactly how I do it. And I also use chime. :) The only differences between your code and mine is formatting and using :datasource instead of :db-src.
And it should already work with the "reloaded" workflow, assuming you use the right API of Integrant for it.
Great to hear! Thank you for your help ❤️