Respect Content-Type instead of Accept header in consume!

[pgaertig]

This one fixes #64 . Today consume! incorrectly uses Accept request header to parse incoming content. The Accept header is about response data format not incoming format. This fix changes consume! to use Content-Type of request header to determine format of consumed content. If format is missing, unknown or not registered with Mime::Type the rack based framework will return HTTP status 415 Unsupported Media Type.

[pgaertig]

@apotonick Content-Type is standard compliant way of solving the issue but I suppose it may break some existing clients so I was considering fallback to Accept header when Content-Type is not present. Eventually some default format could be set in controller or by consume! option. What do you think?

[apotonick]

That looks great, @pgaertig I always had the feeling that my content type detection is wrong. So, thanks! ❤️

One question, though:

1. Why do we have to do the Mime lookup?
2. And, what's the deal with the exception? Is that the way other gems do it (happy to merge I just have never seen that).
3. Why would this break "old" apps? Why on earth did I check the Accept header at all?

[pgaertig]

1. Mime lookup is required to get symbol of format :json, :hal, :xml or whatever from raw Content-Type header which can be like application/hal+json. Accept header is already available via formats because Rails parses it before running an action, but Content-Type is not.
2. If you request Rails action for an unsupported format with Accept or by :format param then the framework returns 406 Not acceptable. Likewise, according to HTTP spec the unknown mime format in Content-Type may result in returning 415 Unsupported media type ( http://tools.ietf.org/html/rfc7231#section-6.5.13 ). consume! is with ! so naturally it should raise an exception in case such of issues, next the Rack middleware served by Rails converts an exception to proper HTTP 415 response.
3. I don't know really. Currently Roar-rails recognizes input data format by Accept header or format param, which is wrong. It happens for some time now so some developers could conform that rule. If they don't send proper Content-Type and will do an upgrade to newer roar-rails with above fix it will definitelly break their app.They will get status code 415 for sure. I feel I am a bit paranoid here :) Changelog and documentation should note that anyway.

I see that Travis CI is not passing against Rails 3.x . I suppose its problem with setting header in tests. I will check it out.

[apotonick]

I think I copied the Accept header bull from the old responders code. I personally think we can just release 1.0. This won't break too many people's code, and if it does, they will see it instantly.

If you could look into the builds 👍

[apotonick]

I've played with this and this will go into 1.0!

[apotonick]

So is that a "bug" in Rails AC::TestCase? Passing :format doesn't seem to set the format right (Content-type)?

[apotonick]

We can fix that ourselves in Roar::Rails::TestCase#process!

[pgaertig]

I've made tests compatible with 3.x. I also updated readme. Please do some proofreading.

[apotonick]

Great work, @pgaertig, really really! ❤️

I am wondering if we can do the Content-type assignment in Roar::Rails::TestCase#process so it properly sets that field?

This goes into 1.0.0, so no one can complain about changed semantics. I always hated that Accept header and knew it was wrong, deep inside of me....

[pgaertig]

Thanks, and finally green tests! I left some typos behind - it is quite late here ;)
Regarding Roar::Rails::TestCase#process I feel it should stay as it is to have it compatible with Rails semantics. Eventually I would introduce something like #consumes_content_type or #will_consume to free developers from awkward @request... stuff.

[apotonick]

Go to bed!!!! 😴

The idea was to make TestCase#process a bit smarter and set Content-type automatically. I absolutely do not want it to be compatible to the Rails' core method as the latter does not what I want!!! Also, if it turns out to be the right behaviour, we can merge it back into Rails core as I already did before with #process.

[pgaertig]

Hm, something like this?:

 post :create, content_type: :json, content

I hope user won't forget to include Roar::Rails::TestCase or he will get content_type in params.

Yet I have no idea how to automatically detect content type from content provided other that use some default - XML currently in above TestCase.

[apotonick]

Your branch is merged here: 144cdfa#diff-56a50d5da8ce4e982d68b9ef16833b2bR31

I removed a lot of checks as we don't need to be all too gentle 😁 I am wondering why (in the linked test) it still finds a representer even though there is no HAL representer available. I might have to change that in self.class.represents_options.for(format, model, controller_path).

[apotonick]

Thanks again @pgaertig, your stuff is great. I found out that the actual problem with the HAL test is broken in roar-rails' way of registering known media types. I will change that with #73.