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

Proof of concept for bringing your own JSON library #53

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rome-user
Copy link

Addresses #52.

This is not ready to be merged; unit test are not written yet. Here is an example of how to use:

  1. Require the library adapter you want. In my case, I want (require 'hato.optional.data-json).
  2. Use JSON in your requests like so.
(hato.client/post "https://httpbin.org/post"
                  {:form-params {:a {:b 5}}
                   :content-type :json})

The current design introduces a dynamic variable hato.middleware/*json-lib*, which gets set when you load the optional JSON adapter, or you can bind it yourself!

(binding [hato.middleware/*json-lib* 'cheshire]
  ;; This will use cheshire for decoding the JSON
  (hato.client/get "https://dogapi.dog/api/v2/breeds/68f47c5a-5115-47cd-9849-e45d3c378f12"
                   {:as :json}))

@p-himik
Copy link

p-himik commented May 28, 2023

The PR uses multimethods and a dynamic var to select the desired JSON implementation.

I haven't looked into it too much but it feels that a better alternative would be to add :encode-json and :decode-json keys to the client configuration map passed to build-http-client.

@rome-user
Copy link
Author

I've changed my mind on my idea but never noted it here. I think in general "choosing your own JSON library" is anti-modular. If there are two libraries that depend on hato, they may not agree on the JSON library to choose. Your idea may get around this, but it still enables the exact opposite of what I intended: multiple JSON codecs in a Clojure project

@rome-user rome-user marked this pull request as draft May 30, 2023 01:09
@p-himik
Copy link

p-himik commented May 30, 2023

I'd say libraries that use Hato should provide their users an ability to specify a custom Hato client. It makes sense regardless of JSON processing.

Multiple JSON codecs in a Clojure project can be easily achieved with my approach - you can simply construct multiple Hato clients. A Hato client is not inherently a singleton, you specify it per-request. In fact, by default Hato builds a new client for each request.

@jimpil
Copy link

jimpil commented Aug 31, 2023

There is a rather neat little trick that hato can use to essentially drop the cheshire dependency, and use whatever json-lib is available on the classpath (e.g. data.json, jsonista, or in fact cheshire). Consider a json.clj with the following public Vars:

(def from-string
  (or
   (find-impl "cheshire"  :from/string) ;; cheshire.core/parse-string
   (find-impl "data.json" :from/string) ;; clojure.data.json/read-str
   (find-impl "jsonista"  :from/string) ;; jsonista.core/read-value
   (no-lib-found!)))

(def from-stream
  (or
   (find-impl "cheshire"  :from/stream) ;; cheshire.core/parse-stream
   (find-impl "data.json" :from/stream) ;; clojure.data.json/read
   (find-impl "jsonista"  :from/stream) ;; jsonista.core/read-value
   (no-lib-found!)))

(def to-string
  (or
   (find-impl "cheshire"  :to/string)   ;; cheshire.core/generate-string
   (find-impl "data.json" :to/string)   ;; clojure.data.json/write-str
   (find-impl "jsonista"  :to/string)   ;; jsonista.core/write-value-as-string
   (no-lib-found!)))

(def to-stream
  (or
   (find-impl "cheshire"  :to/stream)   ;; cheshire.core/generate-stream
   (find-impl "data.json" :to/stream)   ;; clojure.data.json/write
   (find-impl "jsonista"  :to/stream)   ;; jsonista.core/write-value
   (no-lib-found!)))

With data.json on the classpath, evaluating from-string gives #function[clojure.data.json/read-str]. The downside is that if more than one of these 3 libs are on the classpath (which can/does happen), the choice will be the first match (in the hard-coded order). This might not be a problem for hato itself (as it will have no json dependencies anymore), but might cause confusion to consumers. Whether that is acceptable or not, is kind of debatable and ultimately up to the maintainer. For what it's worth completing the rest of the namespace shown above is fairly trivial:

(defn- try-resolve [sym]
  (try @(requiring-resolve sym)
       (catch Exception _ nil)))

(defn- find-impl
  [lib direction]
  (case lib
    "cheshire"
    (case direction
      :from/string  (try-resolve 'cheshire.core/parse-string)
      :from/stream  (try-resolve 'cheshire.core/parse-stream)
      :to/string    (try-resolve 'cheshire.core/generate-string)
      :to/stream    (try-resolve 'cheshire.core/generate-stream))
    "data.json"
    (case direction
      :from/string (try-resolve 'clojure.data.json/read-str)
      :from/stream (try-resolve 'clojure.data.json/read)
      :to/string   (try-resolve 'clojure.data.json/write-str)
      :to/stream   (try-resolve 'clojure.data.json/write))
    "jsonista"
    (case direction
      :from/string (try-resolve 'jsonista.core/read-value)
      :from/stream (try-resolve 'jsonista.core/read-value)
      :to/string   (try-resolve 'jsonista.core/write-value-as-string)
      :to/stream   (try-resolve 'jsonista.core/write-value))
    ))

(defn- no-lib-found! []
  (throw
   (IllegalStateException.
    "None of`cheshire`, `data.json`, or `jsonista` could be found on the classpath!")))

Hope this helps...

@jimpil
Copy link

jimpil commented Sep 1, 2023

@p-himik I agree that being able to provide your own encode/decode functions when building the client, is in many ways the cleanest & most flexible approach. However, it doesn't really help with dropping the cheshire dependency, unless these two keys were made mandatory both when building a client object, but also when issuing a plain request (which as you said, builds a new client every time), and doing so would be a breaking change ...

[EDIT] - apologies, it seems that hato does NOT depend on cheshire directly, but rather, it sort of assumes/checks if it's there (via json-enabled?). Even though this approach is not too different from the code in my earlier message, I find it kind of odd, from an API design point-of-view. If you think about it, whether json/transit etc is enabled (or not) is irrelevant...The real question is is json/transit required?, and that can be inferred from the args provided when building a client (or a request). For example, if you saw :as :json in the params, then you would have to provide a json encoder/decoder. From an API perspective this would have been so much cleaner/explicit, and this ticket/PR would not exist. However, since the decision to find/resolve cheshire dynamically has already been made, it wouldn't be too hard to extend it to three choices, and in fact, the hard-coded order of (or chesire data.json jsonista) kind of fits here, because cheshire has always been the 'default'. The important thing is that, providing your own encode/decode fns (e.g. at the client/request level), none of that indirection should matter! In other words, there are two dimensions to this:

  1. What should hato do in the absence of any custom encode/decode fns? Currently it tries to resolve cheshire, and could fail. It could additionally try to resolve data.json (which many people use), and perhaps jsonista. Nothing will change for classpaths that include cheshire (backwards compatible), and classpaths that don't include it, might work (instead of straight failing). The only confusion that might arise is when both data.json & jsonista are on the classpath, and user actually wants jsonista (but will get data.json). Not providing a jsonista fallback sidesteps this issue altogether.
  2. what should hato do in the presence of custom encode/decode fns? it should honor them!

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

Successfully merging this pull request may close these issues.

3 participants