ring-clojure / ring-json

Ring middleware for handling JSON
313 stars 47 forks source link

Replace :content-type key with header lookup #35

Closed vignessh closed 9 years ago

vignessh commented 9 years ago

The private function json-request? was checking for the presence of the :content-type keyword inside the request object. When I was running Compojure with Aleph, this was no longer true. request now contains a map :headers under which :content-type is available. Because of this, the ring-json middleware wasn't able to parse the incoming content as a json and instead left the body as a input stream.

weavejester commented 9 years ago

Aleph should still contain a :content-type key, as it's only deprecated in the spec, not removed entirely. So in this case, Aleph is at fault.

However, it is a good idea to switch from the deprecated :content-type key to the header key directly. To do this, you just need to change:

(:content-type request)

To:

(get-in request [:headers "content-type"])

You don't need to check multiple sources, because we have a spec telling us what the correct source is. You also don't need to add a new test; just change the existing tests to use the header. That should work with older and newer versions of Ring.

Finally, make sure that your commit summary line is brief, under 70 characters at maximum, otherwise you'll find it wrapping. So something more like:

Replace :content-type key with header lookup

Since Ring 1.3.0, the :content-type key has been deprecated.

This provides a summary of the change, and a more detailed explanation of why in the commit body.

vignessh commented 9 years ago

Thanks for the comments ! Will make the necessary amends and create the request again.

weavejester commented 9 years ago

Thanks. Could you squash your commits?

vignessh commented 9 years ago

Sure, I've squashed both the commits and pushed again.

vignessh commented 9 years ago

Let me know if there is anything more to be done from my side. Many thanks !

weavejester commented 9 years ago

Nope, that's fine. The earlier update just got lost somewhere in my inbox.