grape icon indicating copy to clipboard operation
grape copied to clipboard

env['api.format'] does not override formatter set by content-type

Open waynerobinson opened this issue 4 years ago • 5 comments

In reference to #1989 (sorry, I have commented in there before I realised the issue was closed).

I'm having exactly the same issue.

If this is used:

env['api.format'] = :binary # or :txt
content_type 'application/json' # or header 'Content-Type', 'application/json'
{key: 123}.to_json

Then it still calls the JSON formatter. If the content_type method is left off, it doesn't.

Here is an example using grape-on-rack:

https://github.com/waynerobinson/grape-on-rack

Please take a look at the extra methods added to Acme::Ping.

The only one that doesn't call the JSON formatter is the one where the content type is set to text/plain.

waynerobinson avatar Mar 30 '21 07:03 waynerobinson

It looks like the value of the content type overrides the format. See https://github.com/ruby-grape/grape/blob/02d7113d09eb9fcb4264c841d1fdd305e3e8adb5/lib/grape/middleware/formatter.rb#L55-L58

I'm not sure what the safest way is to reverse this? Could it literally just be reversed?

waynerobinson avatar Mar 30 '21 07:03 waynerobinson

If this test shows the intended behaviour, I don't think we'll be able to override the output by just changing the order the api_format is looked up in.

https://github.com/ruby-grape/grape/blob/7741f022b5928f0ddcee27de08555d95ea4556f8/spec/grape/api_spec.rb#L3732-L3739

If we did, it would break the behaviour where there is an explicit format set, but the implementation expects to be able to override it by changing the content type.

For example:

format :json

def action
  content_type "text/plain"
  "plain text"
end

would still output as JSON if we reversed the lookup to api_format = env[Grape::Env::API_FORMAT] || mime_types[headers[Grape::Http::Headers::CONTENT_TYPE]] because it would always pull out :json first.

I don't think there's any non-breaking way to override the behaviour of one of the content-type associated formatters at the moment on a per-request basis.

waynerobinson avatar Mar 30 '21 07:03 waynerobinson

So it looks like a bug because we explicitly document You can override this process (content negotiation) explicitly by specifying env['api.format'] in the API itself. Since you already have a bunch of examples, turn them into specs in Grape? We can discuss how to fix it, flipping the order on that line should be a start. And it does look like it would be breaking either way, so maybe we should remove support for env['api.format'] that doesn't work, and find another way to do it.

Related, what's a real use-case where you want to do this? Is it that you want to produce actual JSON and leave the formatter alone? Why not rely on the formatter to do this work?

dblock avatar Mar 30 '21 12:03 dblock

here's my use case: I build JSON in the database and want to return it as is, no additional formatting is needed

iabdulin avatar Mar 30 '21 15:03 iabdulin

Yeah. Our use case is similar. We have cached data that's already JSON that we'd like to just return vs having to parse and re-encode.

I'll take a look at starting a PR tomorrow with some options. 👍

waynerobinson avatar Mar 30 '21 21:03 waynerobinson