gnarroway / hato

An HTTP client for Clojure, wrapping JDK 11's HttpClient
MIT License
380 stars 27 forks source link

Adjust behavior when parsing an invalid content-type header? #42

Open vincentjames501 opened 2 years ago

vincentjames501 commented 2 years ago

I'm fine closing this one as I could go either way on if this is a real issue or not or what the debatable expected behavior should be.

One of our services was accidentally sending back application/json; charset: utf8 instead of application/json; charset=utf8 which causes hato to fail:

[[clojure.string$trim invokeStatic "string.clj" 238]
                       [clojure.string$trim invoke "string.clj" 234]
                       [hato.middleware$parse_content_type$fn__11368 invoke "middleware.clj" 254]
                       [clojure.core$map$fn__5884 invoke "core.clj" 2757]
                       [clojure.lang.LazySeq sval "LazySeq.java" 42]
                       [clojure.lang.LazySeq seq "LazySeq.java" 51]
                       [clojure.lang.RT seq "RT.java" 535]
                       [clojure.core$seq__5419 invokeStatic "core.clj" 139]
                       [clojure.core.protocols$seq_reduce invokeStatic "protocols.clj" 24]
                       [clojure.core.protocols$fn__8168 invokeStatic "protocols.clj" 75]
                       [clojure.core.protocols$fn__8168 invoke "protocols.clj" 75]
                       [clojure.core.protocols$fn__8110$G__8105__8123 invoke "protocols.clj" 13]
                       [clojure.core$reduce invokeStatic "core.clj" 6830]
                       [clojure.core$into invokeStatic "core.clj" 6897]
                       [clojure.core$into invoke "core.clj" 6889]
                       [hato.middleware$parse_content_type invokeStatic "middleware.clj" 255]
                       [hato.middleware$parse_content_type invoke "middleware.clj" 245]
                       [hato.middleware$output_coercion_response invokeStatic "middleware.clj" 261]
                       [hato.middleware$output_coercion_response invoke "middleware.clj" 257]
                       [hato.middleware$wrap_output_coercion$fn__11381$fn__11382 invoke "middleware.clj" 274]
                       [hato.middleware$wrap_decompression$fn__11529$fn__11530 invoke "middleware.clj" 641]
                       [hato.middleware$wrap_request_timing$fn__11501$fn__11502 invoke "middleware.clj" 571]
                       [hato.client$request_STAR_$reify__11636 apply "client.clj" 308]
                       [java.util.concurrent.CompletableFuture$UniApply tryFire "CompletableFuture.java" 642]
(defn- parse-content-type
  "Parse `s` as an RFC 2616 media type."
  [s]
  (when-let [m (re-matches #"\s*(([^/]+)/([^ ;]+))\s*(\s*;.*)?" (str s))]
    {:content-type (keyword (nth m 1))
     :content-type-params
     (->> (clojure.string/split (str (nth m 4)) #"\s*;\s*")
          (remove clojure.string/blank?)
          (map #(clojure.string/split % #"="))
          (map (fn [[k v]] [(keyword (clojure.string/lower-case k)) (clojure.string/trim v)]))
          (into {}))}))

Some other options:

1) Wrap w/ a try/catch and let coerce-response-body just hit the default defmethod 2) Wrap with a try/catch and throw a specific exception about invalid content-type header 3) Attempt to parse as much as possible such as changing this line to what's below. This will cause the body to be parsed as json still but will ignore the encoding since it couldn't be parsed. (map (fn [[k v]] (when (and k v) [(keyword (clojure.string/lower-case k)) (clojure.string/trim v)])))

gnarroway commented 2 years ago

Thanks for raising this. The ugly error is a bit unfortunate.

Happy to accept a PR on option 3.