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

Support use in an http client #107

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

chrisbetz
Copy link

@chrisbetz chrisbetz commented Mar 19, 2020

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).

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 Reviewable

Dr. Christian Betz added 7 commits March 16, 2020 17:53
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.
@chrisbetz chrisbetz changed the title Support use in a http client Support use in an http client Mar 19, 2020
Copy link
Member

@ikitommi ikitommi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

v))
(:headers request-or-response)))

(defn header?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be header-exists?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be, definitively. :)

(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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please re-identent all files as they were :)

@@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this too.

@@ -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))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

: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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

v))
(:headers request-or-response)))

(defn header?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@chrisbetz
Copy link
Author

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.)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

4 participants