Support use in an http client #107

chrisbetz · 2020-03-19T13:33:16Z

Hi @ikitommi, hope you're fine after ClojureD.

Got a PR for you, as I'm trying to build production code on top of a hato/muuntaja combination for (new) http client libs.

As stated in #101 , content negotiation is not only a server-side issue, but also a http client side one. As muuntaja looks very promising (thanks for that), I made changes to use it in my hato fork (an http client based upon the 'new' jdk11 http client).

One problem I stumbled upon was Disabling Muuntaja with Content-Type header is case-sensitive #99 .
Another one was a failure when reading empty bodies. That should be ok if format permits (e.g. an empty JSON body should return a nil :body).

It would be very kind to consider the changes. However, I stumbled over some minor difficulties developing the stuff, so besides the necessary changes to the code I also altered stuff in CONTRIBUTING.md, which I believe just wasn't tested/updated properly before.

And, last but not least, I switched to 'lein v' instead of hardcoded versions to keep things consistent between the modules more easily. It made my life easier, but you might not like it.

I kept the commits clean, so you can basically cherry-pick the changes or take the whole PR. Up to you.

Regards,

Chris

This change is

In http-1.1, headers are to be compared case insensitive, and in http-2, headers are to be lower-cased before encoding. Thus, retrieving "Content-Type" in a case-sensitive fashion is likely to fail. Introduce two new abstractions muuntaja.core/header and .../header? to relieve direct dependencies on how headers are represented.

Helping others to contribute, as I stumbled over minor problems with CONTRIBUTING.md

At gorillalabs we prefer managing versions by git tags and `lein v` commands. Also, lein v middleware makes it unneccessary to manually update dependency versions. Thus, the there's no need for `scripts/set-version` anymore. Also, managed-dependencies is kind of broken in combination with lein v middleware. However, it did not make sense to have the versions at a central place anyhow, since each central dependency is used in one place only. So I pushed the version of dependencies (e.g., jsonista) to the project definition where it is acutally used.

Having the same project coordinates (metosin/muuntaja) for the parent project and a module clashed in Cursive. Also, some tests require jsonista namespaces without stating that dependency, obviously leaving things to implicit dependencies (from module metosin/muuntaja). This makes it hard to start a REPL in Cursive with the proper settings. With this commit, I was able to start a Cursive based REPL where I was able to run both the tests and (re-)load the `muuntaja.core` ns). This increases (my) development speed.

If there is nothing to decode, it is okay to return nil. That's not a failure to decode. This problem came up when testing HEAD requests, which are body-less by default. If we still run it through middleware to decode the body, that should not fail. It's not super-elegant, as we already know we do not care for the body anyhow (or at least we shouldn't care), but it should not fail. Also, it might arise in other situations, too.

To use muuntaja on the client side, it is necessary to encode the request body. The functionality was there already, however the code was "asymmetrical" wrt to decoding/encoding. This "duplicates" code with minor changes for encoding/decoding willingly.

ikitommi

thanks!

Comments attached.

related to

Another one was a failure when reading empty bodies. That should be ok if format permits (e.g. an empty JSON body should return a nil :body).

I think it already works?

((m/decoder m/instance "application/json") nil) ;=> nil
((m/decoder m/instance "application/edn") nil) ; => nil

ikitommi · 2020-03-25T07:18:12Z

modules/muuntaja/src/muuntaja/core.clj

+ v))
+ (:headers request-or-response)))
+
+(defn header?


could be header-exists?

could be, definitively. :)

ikitommi · 2020-03-25T07:27:32Z

modules/muuntaja/src/muuntaja/core.clj

 (defn extract-content-type-ring
 "Extracts content-type from ring-request."
 [request]
 (or
 (:content-type request)
- (get (:headers request) "content-type")))
+ (header "content-type" request)))


all these extractors are actually what the Ring1 spec defines: all request keys are always lower-cased.

In Ring2, both the request and response keys are lowercased.

The suggestef "all-are-good" is definetely easy for developers, but it is order of magnitude slower as one has to scan all headers.

I suggest creating an extra set of extractor functions, extract-content-type-ignore-case etc. which would use the suggested code. I have proposed a set of protocols to ring, which would have a (get-header [this name]) that could call the Java servers underlaying & efficient impl, which already is ignore-case. Would have another set of extractors for this.

Not sure what the default should be, maybe the easy-one you have done, but let's keep an option to swap it to faster impl when the users know what they are doing.

ikitommi · 2020-03-25T07:28:10Z

modules/muuntaja/src/muuntaja/core.clj

- :extract-accept extract-accept-ring
- :decode-request-body? (constantly true)
- :encode-response-body? encode-collections}
+ {:http {:extract-content-type extract-content-type-ring


please re-identent all files as they were :)

ikitommi · 2020-03-25T07:31:01Z

modules/muuntaja/src/muuntaja/core.clj

@@ -423,7 +491,7 @@
 (catch Exception e
 (fail-on-request-decode-exception m e req-fc res-fc request))))))
 -encode-response? (fn [request response]
- (and (or (not (contains? (:headers response) "Content-Type"))
+ (and (or (not (header? "Content-Type" response))


I think this should also be one of the callback fn's via options, for example :response-header-exists?.

ikitommi · 2020-03-25T07:31:46Z

modules/muuntaja/src/muuntaja/core.clj

@@ -441,7 +509,7 @@
 (as-> response $
 (dissoc $ :muuntaja/content-type)
 (update $ :body encoder charset)
- (if-not (get (:headers $) "Content-Type")


ikitommi · 2020-03-25T07:55:59Z

modules/muuntaja/src/muuntaja/core.clj

@@ -497,13 +565,29 @@
 $))))
 (-decode-response-body [this response]
 (or
- (if-let [res-fc (-> response :headers (get "Content-Type") -negotiate-content-type)]
+ (if-let [res-fc (-negotiate-content-type (or (header "Content-Type" response)
+ default-format))]


What is a use case for this? if the client doesn't send any content-type, it could be anything, most likely text/plain?

ikitommi · 2020-03-25T07:58:27Z

project.clj

- :codox {:src-uri "http://github.com/metosin/muuntaja/blob/master/{filepath}#L{line}"
+ :plugins [[lein-codox "0.10.7"]
+ [com.roomkey/lein-v "7.0.0"]]
+ :middleware [leiningen.v/version-from-scm


lein-v looks ok, but any guide how to use that?

hkupty · 2020-03-25T09:39:21Z

modules/muuntaja/src/muuntaja/core.clj

+ v))
+ (:headers request-or-response)))
+
+(defn header?


Could be defined as (some? (header header-name request-or-response)). Current implementation will return the supplied header-name or nil.

chrisbetz · 2020-03-25T17:12:02Z

Regarding the empty bodies (see 48d18da):

before my change, that was

(-decode-response-body [this response]
             (or
               (if-let [res-fc (-> response :headers (get "Content-Type") -negotiate-content-type)]
                 (if-let [decode (decoder this (:format res-fc))]
                   (try
                     (decode (:body response) (:charset res-fc))
                     (catch Exception e
                       (fail-on-response-decode-exception m e res-fc response)))))
               (fail-on-response-decode m response)...)

I created that change for a client application. When performing a HEAD request, there is no body content. So, decode resulted in a nil value (just as you wrote above). That nil result from decode results in the or falling through to the (fail-on-response-decode m response)...).

(I will not come through every comment today, as I had some other problems with another part of our own code today.)

@ikitommi

From the [PR comments by @ikitommi] (metosin#107 (comment)).

Deraen · 2020-03-25T20:24:28Z

modules/muuntaja/project.clj

@@ -1,13 +1,18 @@
-(defproject metosin/muuntaja "0.6.6"
+(defproject metosin/muuntaja "0.0.0" ;; use lein v


My opinion would be to just keep using set-version script.

after giving it more thought, I agree on using the set-version

Dr. Christian Betz added 7 commits March 16, 2020 17:53

Fix documented commands

44f2fc3

Helping others to contribute, as I stumbled over minor problems with CONTRIBUTING.md

Add next release to CHANGELOG

af36b60

Add request body encoding

058daec

To use muuntaja on the client side, it is necessary to encode the request body. The functionality was there already, however the code was "asymmetrical" wrt to decoding/encoding. This "duplicates" code with minor changes for encoding/decoding willingly.

chrisbetz changed the title ~~Support use in a http client~~ Support use in an http client Mar 19, 2020

Respect the default format

d2fa342

ikitommi requested changes Mar 25, 2020

View reviewed changes

hkupty reviewed Mar 25, 2020

View reviewed changes

Renamed header? to header-exists?

ed234d2

From the [PR comments by @ikitommi] (metosin#107 (comment)).

Deraen reviewed Mar 25, 2020

View reviewed changes

opqdonut force-pushed the master branch from 198c82b to c1170aa Compare March 15, 2024 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support use in an http client #107

Support use in an http client #107

chrisbetz commented Mar 19, 2020 •

edited

ikitommi left a comment

ikitommi Mar 25, 2020

chrisbetz Mar 25, 2020

ikitommi Mar 25, 2020

ikitommi Mar 25, 2020

ikitommi Mar 25, 2020

ikitommi Mar 25, 2020

ikitommi Mar 25, 2020

ikitommi Mar 25, 2020

hkupty Mar 25, 2020

chrisbetz commented Mar 25, 2020

Deraen Mar 25, 2020

ikitommi Mar 29, 2020

		@@ -1,13 +1,18 @@
		(defproject metosin/muuntaja "0.6.6"
		(defproject metosin/muuntaja "0.0.0" ;; use lein v

Support use in an http client #107

Are you sure you want to change the base?

Support use in an http client #107

Conversation

chrisbetz commented Mar 19, 2020 • edited

ikitommi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbetz commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbetz commented Mar 19, 2020 •

edited