`after` filter processing of response.body is obstructed #512

Closed
mislav opened this Issue May 8, 2012 · 10 comments

Projects

None yet

3 participants

mislav commented May 8, 2012

My app returns a lot of JSON. I figured I would automatize JSON serialization for responses instead of calling JSON.generate every time:

get '/' do
  body :hello => 'from json'
end

after do
  if Hash === response.body
    content_type :json
    body JSON.generate(response.body)
  end
end

I expected this app to return this response body: {"hello": "from json"}. However, a Ruby string representation of my hash converted to an Array was returned. It was just as if my after block didn't exist.

The offending code in Sinatra that's sabotaging my editing of response.body is the combination of invoke and dispatch!:

  1. The after block happens in dispatch!
  2. however, the whole thing is wrapped in an invoke, which takes the result of running dispatch! and stores it in response.body if it responds to each
  3. dispatch! returns the last expression of running a route; in my case it's the raw Hash
  4. So even if I stored a string to response.body in after, the invoke block overwrites that by storing the raw Hash

I've worked around this problem for now by augmenting body helper to not return anything after writing to response.body:

helpers do
  def body(value = nil) super; nil end
end

However I fear that changing the behavior of this method might trigger some nasty edge cases in Sinatra because body is used a lot internally.

What say you? A bug or feature? (I say bug!) Any ideas how to fix this?

canton7 commented May 8, 2012

Slightly off-topic, but you're aware of http://www.sinatrarb.com/contrib/json.html ?

mislav commented May 8, 2012

@canton7 No, thanks for sharing. But I was thinking of writing my own json method (it would be a 2-liner) and using it instead of body. It would work around this issue. I'm not saying my initial approach is the best to use here, but I do want to be able to perform response.body post-processing in after and am sad that I can't.

Owner
rkh commented May 13, 2012

What say you? A bug or feature? (I say bug!) Any ideas how to fix this?

It's not a bug, IMO. I think this issue is a feature request, i.e. add the ability to store arbitrary objects in body.

body without argument is assumed to return the current body (you should use this instead of response.body). If we assign anything to response.body, we might mess with Rack's internals (it's basically a Rack::Response, which accepts a String or anything adhering to the Rack SPEC).

I have to think on this.

mislav commented May 13, 2012

The official documentation mentions the use-case of using body in after filter:

Note: Unless you use the body method rather than just returning a String from the routes, the body will not yet be available in the after filter, since it is generated later on.

Seems silly that you are able to read the body in the filter, but not change it.

Owner
rkh commented May 13, 2012

Uh, oh, you can change it. You just can't set a Hash as body in the route.

mislav commented May 14, 2012

Uh, oh, you can change it.

…Really?

require 'net/http'
require 'sinatra'
set :environment, :test
disable :run

get('/') { body 'route'  }
after    { body 'filter' }

Thread.new { Sinatra::Application.run! }.join 1

res = Net::HTTP.get URI('http://localhost:4567')
puts res
# => "route"
Owner
rkh commented May 14, 2012

OK, that is a bug, I guess.

@mislav mislav added a commit to github/hub that referenced this issue Oct 10, 2012
@mislav mislav abandon the nasty JSON hack for Sinatra responses
I tried to hack Sinatra to automatically serialize non-string response
bodies as JSON, but this is not straightforward due to sinatra/sinatra#512
and doesn't really work in a whole lot of edge-cases.

Introduced a simple `json` helper and now using that instead of `body`.
7ab0d27
Owner
rkh commented Jan 26, 2013

btw, we achieved the same in travis-api by overriding body

mislav commented Jan 26, 2013

My failing test case in the previous comment is still failing, even with Sinatra 1.3.3

Owner
rkh commented Jan 26, 2013

Ah, yes, that was just a note. As body is called internally, you can just return the hash in that case. I'm still meaning to fix this in 1.4.

@rkh rkh closed this in ed2b318 Feb 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment