Support for arbitrary config keys?

[dparis]

Hey @tomekw, nice job with the library. I wrote a very similar internal library for a project my company is currently working on. We were looking into open-sourcing it if no one else got around to it, and I'm glad to see someone beat us to it! I'd love to standardize on your library (more eyes = less bugs), so I'm investigating what changes we'll need to make.

One thing I noticed is that your configuration code doesn't allow for arbitrary configuration parameters. This is a problem since the underlying drivers might use all kinds of custom config values. For instance, the PGSimpleDataSource requires the non-standard ssl and sslfactory properties to be set in order to make SSL-encrypted connections.

I'm not sure what the best way would be to handle this generally. In our postgres-specific internal code, we look for the presence of the :ssl and :sslfactory keys in the config, and when both are defined, we call:

(.addDataSourceProperty "ssl" true)
(.addDataSourceProperty "sslfactory" ssl-factory-name)

Any thoughts?

• Dylan

[tomekw]

Thanks @dparis! I will try to come up with something tomorrow!

[tomekw]

@dparis what about enforcing only pool-specific settings (timeouts, size, etc.) and allowing to set arbitrary options with (.addDataSourceProperty config "key" "value") and... let it crash if these adapter-specific option will be invalid?

[tomekw]

#4 :)

[dparis]

Hey @tomekw, sorry I fell off for a bit there. The solution you came up with seems sensible. Only thing I might suggest is for the validate-options function:

(defn validate-options
  ""
  [options]
  (try
    (let [all-options (merge default-datasource-options options)
          required-options (select-keys all-options (keys ConfigurationOptions))]
      (s/validate ConfigurationOptions required-options)
      all-options)
    (catch clojure.lang.ExceptionInfo e
      (throw
        (IllegalArgumentException. (exception-message e))))))

Instead of selecting out of the ConfigurationOptions keys, you can add a generic key/val pair to the schema to catch the arbitrary options while still enforcing that they have keyword keys:

(def ConfigurationOptions
  {:auto-commit        s/Bool
   :read-only          s/Bool
   :connection-timeout IntGte100
   :idle-timeout       IntGte0
   :max-lifetime       IntGte0
   :minimum-idle       IntGte0
   :maximum-pool-size  IntGte0
   :adapter            AdaptersList
   s/Keyword           s/Any})

Otherwise, this looks like a good change to me. If I run into anything else that blocks our adoption of your library, I'll open a new issue. Thanks!

[tomekw]

Good idea! Will do, thanks!