the x-headers middleware appears to be incompatible with seq header values

[yogthos]

Using a header such as {"content-type" ["text/plain" "text/html"]} in the response will result in the following exception:

[clojure.core$re_matcher invokeStatic core.clj 4667]
  [clojure.core$re_find invokeStatic core.clj 4716]
  [clojure.core$re_find invoke core.clj 4716]
  [ring.middleware.default_charset$text_based_content_type_QMARK_ invokeStatic default_charset.clj 7]
  [ring.middleware.default_charset$text_based_content_type_QMARK_ invoke default_charset.clj 6]
  [ring.middleware.default_charset$add_charset invokeStatic default_charset.clj 15]
  [ring.middleware.default_charset$add_charset invoke default_charset.clj 13]
  [ring.middleware.default_charset$wrap_default_charset$fn__12380 invoke default_charset.clj 27]
  [ring.middleware.not_modified$wrap_not_modified$fn__12337 invoke not_modified.clj 52]
  [ring.middleware.x_headers$wrap_xss_protection$fn__11091 invoke x_headers.clj 71]
  [ring.middleware.x_headers$wrap_frame_options$fn__11079 invoke x_headers.clj 38]
  [ring.middleware.x_headers$wrap_content_type_options$fn__11085 invoke x_headers.clj 53]

Not sure if this is the expected behavior as the spec states that header values can be seqs.

[weavejester]

It probably shouldn't throw an error, and I'd welcome a patch to fix it. But is it even legal for a HTTP response to have two content-types?

[yogthos]

Oh yeah the particular example doesn't make sense, I just meant it as an illustration of the issue. I'll take a look at making a patch if I get a chance.

[weavejester]

Isn't the issue specific to the content-type header?

[yogthos]

Ah you know, I haven't actually tried other headers. I was going by the errata opened for my book issue 80401, and that was the header used there.

[weavejester]

Since the stacktrace suggests it's trying to grab the charset from the content-type header, my guess is that this only affects the content-type.

[yogthos]

You're absolutely right, the other headers aren't affected. So, I think the behavior is fine as I don't think the content-type header is repeating. I could do a pr to use a try/catch and throw a more descriptive error, if you think that would be useful.

[yogthos]

I would propose adding the following code for content-type handling:

(defn- add-charset [resp charset]
  (if-let [content-type (response/get-header resp "Content-Type")]
    (try
      (if (and (text-based-content-type? content-type)
               (not (contains-charset? content-type)))
        (response/charset resp charset)
        resp)
      (catch Exception _
        (throw (IllegalArgumentException. (str "failed to parse the Content-Type header: " content-type)))))
    resp))

It would make it clear what the problem was and show what the value of the content-type header was.

[weavejester]

I don't think we should have a broad exception, and if we do catch it, we shouldn't throw it away, but add it as a cause. However, my preference, I think, would be to use the last charset defined instead of throwing an exception.

[yogthos]

I think you're right, using the last defined charset would be the desired behavior.