Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass content_types option to Grape::Middleware::Error #557

Merged
merged 1 commit into from
Jan 23, 2014

Conversation

bernd
Copy link
Contributor

@bernd bernd commented Jan 23, 2014

This fixes the Content-Type header on error responses with a custom
format. It wasn't able to find the custom format and used the default
of "text/html".

This fixes the Content-Type header on error responses with a custom
format. It wasn't able to find the custom format and used the default
of "text/html".
@dblock
Copy link
Member

dblock commented Jan 23, 2014

Just to confirm that I understand this correctly.

Errors now return the same content type as the body. Should this really be the case? Aren't errors their own things and can be handled as such? Or am I misunderstanding and the error middleware already does this and looks up the content type, except that it's broken for custom content-types?

@dm1try
Copy link
Member

dm1try commented Jan 23, 2014

I think, error middleware already does this using Base middleware class implementation:

# lib/grape/middleware/base.rb
def content_types
  ContentTypes.content_types_for(options[:content_types])
end

def content_type
  content_type_for(env['api.format'] || options[:format]) || 'text/html'
end

without options[:content_types], it looks up only predefined content types:

# lib/grape/util/content_types.rb

    CONTENT_TYPES = ActiveSupport::OrderedHash[
      :xml,  'application/xml',
      :serializable_hash, 'application/json',
      :json, 'application/json',
      :jsonapi, 'application/vnd.api+json',
      :atom, 'application/atom+xml',
      :rss,  'application/rss+xml',
      :txt,  'text/plain',
   ]

@bernd
Copy link
Contributor Author

bernd commented Jan 23, 2014

The error middleware already does this but fails for custom content-types.

An example:

class Api < Grape::API
  content_type :json, 'application/json'
  content_type :xml, 'application/xml'
  content_type :custom, 'application/vnd.custom'

  formatter :custom, ->(obj, _) { "CUSTOM | #{obj[:message]}\n" }
  error_formatter :custom, ->(*a) { "CUSTOM  ERROR | #{a.first[:message]}\n" }

  get '/ok' do
    {message: "hello world"}
  end

  get '/error' do
    error!({message: 'ERROR'})
  end
end
+ curl -i localhost:9292/ok.json
HTTP/1.1 200 OK
Content-Type: application/json
Content-Length: 25

{"message":"hello world"}

+ curl -i localhost:9292/ok.xml
HTTP/1.1 200 OK
Content-Type: application/xml
Content-Length: 87

<?xml version="1.0" encoding="UTF-8"?>
<hash>
  <message>hello world</message>
</hash>

+ curl -i localhost:9292/ok.custom
HTTP/1.1 200 OK
Content-Type: application/vnd.custom
Content-Length: 21

CUSTOM | hello world

+ curl -i localhost:9292/error.json
HTTP/1.1 500 Internal Server Error
Content-Type: application/json
Content-Length: 19

{"message":"ERROR"}

+ curl -i localhost:9292/error.xml
HTTP/1.1 500 Internal Server Error
Content-Type: application/xml
Content-Length: 83

<?xml version="1.0" encoding="UTF-8"?>
<error>
  <message>ERROR</message>
</error>

+ curl -i localhost:9292/error.custom
HTTP/1.1 500 Internal Server Error
Content-Type: text/html
Content-Length: 22

CUSTOM  ERROR | ERROR

With my patch applied, the content-type for the last response changes to application/vnd.custom.

@dblock
Copy link
Member

dblock commented Jan 23, 2014

Thanks, merging.

dblock added a commit that referenced this pull request Jan 23, 2014
Pass content_types option to Grape::Middleware::Error
@dblock dblock merged commit 59ca798 into ruby-grape:master Jan 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants