Bypass formatting for statuses with no entity-body

[tylerdooling]

tl;dr;

Allow the formatter middleware to bypass serialization of response bodies when the status indicates there is no content.

Background

#1162 describes a scenario where OPTIONS requests and redirects can raise exceptions when content types are limited to those that don't consider '' as valid or are not prepared to handle it. The specific example given involved content types being limited (or set by default) to XML. '' is not consider to be valid XML and as such results in an error when serialization is attempted.

A resolution was proposed to handle this in the formatters by checking the request method. It seems - and was suggested - that this opens up the scope of concern for the formatters too much and I agree. Additionally, it places a burden on all custom formatters to account for responses intended to have no content - like the OPTIONS request in question.

Process

I initially referenced the RFC to determine the best course of action, and found this:

A 200 response SHOULD include any header fields that indicate optional features implemented by the server and applicable to that resource (e.g., Allow), possibly including extensions not defined by this specification. The response body, if any, SHOULD also include information about the communication options. The format for such a body is not defined by this specification, but might be defined by future extensions to HTTP. Content negotiation MAY be used to select the appropriate response format. If no response body is included, the response MUST include a Content-Length field with a field-value of "0".

This indicates that while the specification for a response body is not defined in the RFC, it is most certainly allowed given that the status code is 200 and should ideally be accounted for. This led me to begin ensuring that OPTIONS requests could be serialized appropriately by the formatter middleware, which led to ideas involving a special representation of no content that could be checked conditioned there.

When thinking through this and beginning to write some tests, I found that I was unable to get the content type to persist in the response. This led me to Rack where I found this:

 if [204, 205, 304].include?(status.to_i)
   header.delete CONTENT_TYPE
   header.delete CONTENT_LENGTH
   close
   [status.to_i, header, []]
 else
   [status.to_i, header, BodyProxy.new(self){}]
 end

After some additional digging I found that Rack::Lint takes it a step further and does not consider a response to be valid if it has content information and it's status code is included in Rack::Utils::STATUS_WITH_NO_ENTITY_BODY.

• https://github.com/rack/rack/blob/master/lib/rack/lint.rb#L659
• https://github.com/rack/rack/blob/master/lib/rack/lint.rb#L673

Proposal

Using these observations, my proposal here is to condition the formatter middleware to look for response statuses that indicate a content body in not appropriate and make the decision to bypass serialization.

This allows for both OPTIONS routes that return a 200 with a serialized body based on normal content negotiation, and for the default OPTIONS route that is auto-generated by the framework which returns a 204 and no content and does add additional concerns to each default and custom formatter.

Additionally, I believe this addresses the second issue in #1162 involving a similar issue with redirects.

[tylerdooling]

I left my refactoring commits separate as to not add noise when reviewing the feature change. Please let me know if you'd prefer them squashed.

[dblock]

This makes a lot of sense. It definitely needs an entry in UPGRADING.

I would be down with just merging this "as is", fix the build though please ;)

[tylerdooling]

Got caught by the cop!

Thanks for the reminder regarding UPGRADING. I added a section there. Please let me know if it is adequate.

[dm1try]

As for me, it makes more sense if we could skip the formatter explicity outside the formatter scope. I mean

# endpoint
def status(code)
   skip_formatter_middleware if no_body_status_code(code)
   ...
end

def skip_formatter_middleware
   env['grape.formatter.skip'] = true
end

# formatter
def call(env)
   return response if  env['grape.formatter.skip']
end

EDIT: removed redirect(it calls status internally)

[dblock]

@dm1try I think it makes sense to do this by default wrt STATUS_WITH_NO_ENTITY_BODY, no? An ENV variation that could go one way or another could be an addon I think.

Tell us how strongly you feel about this or something else before I merge?

[dm1try]

@dblock PR looks good, you can merge it "as is")

[dm1try]

My point was about having possibility to skip formatter middleware for some reason(the STATUS_WITH_NO_ENTITY_BODY is one of them).

[dblock]

Merged

[dblock]

Can we close #1162?

[tylerdooling]

#1159 (OPTIONS) is definitely addressed here, but I recant my suggestion regarding #1157 (Redirects).

The RFC suggests for entity bodies for 301 and 302 responses:

the entity of the response SHOULD contain a short hypertext note with a hyperlink to the new URI(s).

Do you think it would make sense for #redirect to set the body to a Location-like object that supports standard serialization? - i.e. responds to #to_json, #to_xml, and #to_s. It seems like that would be consistent with the theme of the spec. In addition, an override could be provided in options or someplace else if desired by the user.

[tylerdooling]

Another approach could be to set the content type explicitly inside #redirect and allow users to override it via an optional argument:

def redirect(url, options = {})
  # ...
  header 'Location', url
  header Grape::Http::Headers::CONTENT_TYPE, options.fetch(:content_type) { env[Grape::Env::API_FORMAT] }
  body ''
end

The downside is that of course that the burden is on the user if they support multiple serialization formats. In that case though, it could be as simple as:

redirect(some_url, content_type: env[Grape::Env::API_FORMAT]

In that case it might make sense for them to be able to override the body as well to ensure it is serializable.

@dblock - do either of those suggestions strike you as appropriate?

[dblock]

I am not sure. Redirect with content type seems strange to me.

I think this discussion should be re-raised in a new issue either way.