XML formatter can raise errors in scenarios where it shouldn't possibly even be invoked

[dblock]

The scenario in #1159 is that OPTIONS returns an empty string that the XML formatter tries to serialize and fails.

The scenario in #1157 is that a redirect causes an empty body and fails.

[dblock]

@urkle would love a spec that fails with an accept header which included xml but not JSON

@franzliedke would love a spec where the redirect causes the same error

I'll take suggestions on what the "right" fix would be here.

[urkle]

What about adding a method that can be called by the "redirect / options" handlers to disable the use of the formatters?

[urkle]

Here is a spec that fails with the current code. This is using request headers that firefox sends in the OPTIOPNS request that it sends before a POST request. It sends this Accept header regardless of whether the POST has it's Accept header set to JSON or XML!

    it 'options fails with a 500 error with an Accepts header that includes xml' do
      subject.content_type :xml, 'application/xml'
      subject.default_format :xml
      subject.post 'example' do
        {message: 'example'}
      end
      options '/example', {}, {
        Origin: 'http://example.com',
        Accept: 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
        'Acccess-Control-Request-Method' => 'POST'
      }
      expect(last_response.status).to eql 204
      expect(last_response.body).to eql ''
    end

Also I noticed that the OPTIONS request by default is sending back an Allow header.. the Allow header is only supposed to be sent back with a 406 response. OPTIONS uses the Access-Control-Allow-Methods to return that information as to what methods are allowed on this path..

[urkle]

So, a cleaner proposed change. the redirect and option essentially have NO content.. (thus should not even have the Content-Type header). So we need a way for the handler to denote this to the grape core. either via a method e.g. no_content or by returning some fixed value (e.g. a class) that can be tested against and the code would bypass the formatters in that scenario and omit the content-type header.

[dblock]

Why can't we make the XML formatter behave like the JSON formatter?

[urkle]

because an empty string is not valid XML.. but an empty string "" IS valid json.. JSON can be simply

""

and HTTP RFC spec-wise the Options and Redirect should NOT be returning a content-type at all and should simply have no body. That is the correct way to handle those.

[dblock]

So, lets make sure that options and redirect don't have a body and that no formatter is invoked unless there's a body explicitly set?

[urkle]

the issue with that is that for JSON, (and plain text) an empty string is a valid response. so the safest approach is to have the redirect / options set or return a marker that can be tested to mean "REALLY no body here.. move along". that way the behaviour is 100% opt-in and will never interfere with someone's ill-conceived API that legitimately returns an empty string.

[urkle]

Also it's more than just not running a formatter.. It's also that the content-type header should NOT be set in the response headers.

[dblock]

Empty string yes, empty body not so much. If the body is nil with OPTIONS verb or 301 or 302 redirects, skip the formatter altogether - all I am saying is that it shouldn't be something specific to the XML formatter. Try it? See what specs break, if any?

[urkle]

I never said something specific to the XML formatter. I was stating generically. And is "nil" a safe enough marker to make to mean "no body". Is there any legitimate usage for returning nil? It is technically valid for JSON, but not for other formats natively supported by grape.

My thinking was simply to create an empty class e.g.

class Grape::NoContent; end

# later in your method
  self.class.options(path, {}) do
    header 'Allow', allow_header
    status 204
    Grape::NoContent
  end

# later in code that kicks the formatter
  if body.equal/(Grape::NoContent)
    # do whatever to skip formatting and skip setting the Content-Type header
  end

And yes this could work as well with nil, so long as we don't ever expect someone to use nil as a valid response in their API.

[franzliedke]

I agree that we should just return some marker to indicate formatters should be skipped. 👍

What that should look like exactly is not up to me to decide - you know the code far better than I do. :)

[dblock]

I think nil could mean Grape::NoContent, even though null is valid JSON, unless there's already a spec for this. I can't imagine anyone depending on this behavior. I don't have any strong feelings though, if Grape::NoContent was a thing I would merge it.

[tylerdooling]

@dblock - Am I right that we should be able to close this now that #1190 and #1194 have been merged?

[dblock]

Alright.