Ability to format logged queries

[danielytics]

Adds an optional key :log-query-formatter to the component, which takes a function (fn [query] ...) that gets passed each query before it gets logged, for allowing custom formatting of the logged query. This does not impact the execution of the query, only how it gets logged.

As briefly discussed in #3

I'm unsure of the key name and whether it should accept a vector of all queries, or each individual query like now. Feedback welcome.

[atomist]

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

• The commit message subject is over 50 characters.

[weavejester]

Sorry it too so long to get round to reviewing this. There are a few changes necessary that I've commented on.

[weavejester]

Looks like a space was accidentally deleted here, misaligning the indentation.

[weavejester]

Nonstandard indentation, and also short enough to put on one line. Could you format as:

  (into [(str (re-find #"^\w+" query) "...")] params)

[weavejester]

Line longer than 80 characters.

[weavejester]

Why use an if here, instead of tacking the extra assoc onto the end of the cond->?

[danielytics]

I think the reason was so that it only runs:

(assoc boundary :unlogged-spec {:datasource datasource})

when a logger is set. Perhaps just always set unlogged-spec, regardless of if a logger is specified or not?

Something like this:

(let [datasource (-> (dissoc options :logger)
                     (assoc :jdbc-url (or jdbc-url connection-uri))
                     (hikari-cp/make-datasource))]
    (-> (sql/->Boundary {:datasource (cond-> datasource logger (wrap-logger logger log-query-formatter))})
        (assoc :unlogged-spec {:datasource datasource})))

PS: I'm not sure how you want it formatted, I'm having a hard time the wrap-logger line under 80 characters without introducing another binding in the let. Perhaps this:

(defmethod ig/init-key :duct.database.sql/hikaricp
  [_ {:keys [logger connection-uri jdbc-url log-query-formatter]
      :or {log-query-formatter identity} :as options}]
  (let [datasource (-> (dissoc options :logger)
                       (assoc :jdbc-url (or jdbc-url connection-uri))
                       (hikari-cp/make-datasource))
        logged-datasource (cond-> datasource
                            logger (wrap-logger logger log-query-formatter))]
    (assoc (sql/->Boundary {:datasource logged-datasource})
           :unlogged-spec {:datasource datasource})))

[weavejester]

I see now. Then perhaps let's factor out some of this into a separate function for clarity:

(defn- logged-boundary [datasource {:keys [logger log-query-formatter]}]
  (let [logged-source (wrap-logger datasource logger log-query-formatter)]
    (-> (sql/->Boundary {:datasource logged-source})
        (assoc :unlogged-spec {:datasource datasource}))))

(defmethod ig/init-key :duct.database.sql/hikaricp
  [_ {:keys [logger connection-uri jdbc-url logger] :as options}]
  (let [datasource (-> (dissoc options :logger)
                       (assoc :jdbc-url (or jdbc-url connection-uri))
                       (hikari-cp/make-datasource))]
    (if logger
      (logged-boundary data-source options)
      (sql/->Boundary {:datasource datasource}))))

What do you think?

[danielytics]

That looks good to me. I'll push this and the other changes soon.

[weavejester]

Adding a default value for log-query-formatter as identity here would prevent a need for the (or query-formatter identity) later on.