[BUG] --- Ring request headers are as keywords, instead of downcased strings

[viesti]

First off, thanks for all the work in this area! I'm really happy about it :)

It seems that when this adapter is used together with the holy-lambda project, because the Lambda input JSON event is parsed as keys being keywordized, in the resulting ring request, the :headers is a map with keywords, instead of downcased strings, as in the Ring spec:

:headers
  (Required, clojure.lang.IPersistentMap)
  A Clojure map of downcased header name Strings to corresponding header value 
  Strings. When there are multiple headers with the same name, the header
  values are concatenated together, separated by the "," character.

Here's a sample prn of a Ring request from an app that I'm testing the library with:

{...
 :headers {:Authorization "eyJraW...",
           :Content-Type "application/json"},

I think this goes unnoticed in the tests, since the tests use a Ring request that has the :headers containing valid Ring spec data. I guess there could be a test in this library, that mocks a lambda runtime and uses holy-lambda proper, to make sure a valid ring request is produced, but before that, we probably could have a simpler test that gives the hander a request with say

{:headers {:Content-Type "..."}}

and makes sure that the adapter converts this into

{:headers {"content-type" "..."}}

[viesti]

Whipped up a quick fix in #4.

Seems to work in my project, though I have some other problems there too :D

[viesti]

Actually, it could also holy-lambda proper would keep the headers map with strings as keys, but not sure what that would cause downstream.

[FieryCod]

I do already normalize the headers before an event is sent to AWS.

Normalizing input headers is the breaking change, but we can introduce it as an option in the core.

If you propose the PR upstream, I will accept it, but please put it under :normalize-headers? flag in h/entrypoint.

[viesti]

Right, for the core, a flag sounds reasonable for backward compatibility. And we can document the flag too.

I was wondering then the holy-lambdaring-adapter. I guess normalizing header keys is a backward breaking change for any current users, though the current users then have a version is does not entirely Ring spec compatible when used with holy-lambda core. Though someone could use this library with another "core" even, that already produces headers that conform to Ring spec.

I don't know if this library is used with another "core" though. So thinking that that would it be ok to do header normalization and document it in a changelog, but to warn that this is a breaking change? Or do we just do this in the core :)

[FieryCod]

Just do it in the core. It's safer that way. By the way, do you see any other issues with an adapter?

[viesti]

Just do it in the core. It's safer that way.

Yup, I'll see when I'll get to that :)

By the way, do you see any other issues with an adapter?

I haven't run into other issues, at least in the stuff that I used it for now :) I'll report if I run into any other issues.

I do though have this middleware that inspects the Ring response content type and if the content type is string-like and the body is a java.io.InputStream, then the body is decoded back to a string. This helps to debug Lambda responses that would otherwise be base64 encoded (and keeps the response size smaller). The alternative would be to originally keep the response body as a string, but that might sometimes be more work, at least initially, depending on what the ring handler or middleware is doing (like metosin/muuntaja gives back java.io.InputStream by default).

Not sure if a middleware like this could be part of the adapter by default, but I'd think that one would find that convenient though.

(defn string-content? [^String content-type]
  (or (.contains content-type "application/json")
      (.contains content-type "application/edn")
      ;; Matches also (.contains content-type "application/transit+json-verbose
      (.contains content-type "application/transit+json")
      (.startsWith content-type "text")))

(defn body->string
  "Makes body into a string
  if the content-type of the body is a string
  and the body itself is a java.io.InputStream."
  [content-type body]
  (if (and content-type
           (string-content? content-type)
           (instance? java.io.InputStream body))
    (slurp body)
    body))

(defn response-body->string
  "Turns input stream responses back to strings.

  Mainly meant to coerce muuntaja responses back to a String,
  so that Lambda response event content doesn't need to be encoded to Base64,
  since the Lambda response event can only be a string.

  The alternative would be to teach muuntaja to encode to string,
  instead into a ByteArrayInputStream."
  [handler]
  (fn
    ([req]
     (let [{:keys [headers] :as response} (handler req)
           content-type (get headers "Content-Type")]
       (update response :body #(body->string content-type %1))))
    ([req respond raise]
     (handler req
              (fn string-body-respond [{:keys [headers] :as response}]
                (let [content-type (get headers "Content-Type")]
                  (respond (update response :body #(body->string content-type %1)))))
              raise))))

[FieryCod]

Let's optimize it in the adapter. I think it would be better that way.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types
https://github.com/FieryCod/holy-lambda-ring-adapter/blob/master/src/fierycod/holy_lambda_ring_adapter/impl.cljc#L36

WDYT?

[viesti]

Yeah, doing this in the adapter would be good for users I think, I just didn't yet have change to propose :) I think RingResponseBody would have to take the headers too, not only body, to in order to return a "optimized" body.

[FieryCod]

Exactly. Regarding the content-type we can safely assume the everything that starts from text or contains json,edn,javascript might be represented as a string.

[viesti]

One thing to be aware of could be the encoding of the InputStream/ByteArray so that the String is created in with the encoding of the bytes. We might want to check the relevant HTTP headers from the response body for the encoding.