JSONP content-type does not serialize entities properly...

[deckchairhq]

See issue: #345

I believe this is to do with the lack of support for :jsonp ( content-type: application/javascript ).

Support should be as per :json ( content-type: application/json ) with regard to serializing entities.

Support for application/javascript should be scoped to within the :jsonp symbol, specifically for JSONP. As this is a special case wherein you deliver Javascript but still serialize everything within the callback wrapper as JSON.

I'll get digging on this but presumably you'll know where to look immediately.

Cheers,

G

[dblock]

You also need a formatter for jsonp. The following will fix your problem.

format :jsonp
formatter :jsonp, lambda { |object, env| object.to_json }

I fixed the spec with this and merged it into Grape.

Since we don't do JSONP support "out of the box" and you have to add Rack::JSONP explicitly, I think we shouldn't declare anything for JSONP in Grape proper.

I'd like to see a paragraph on enabling JSONP somewhere, maybe the README or a blog post?

[deckchairhq]

Looks like there's more to this issue than first thought...

It's looks like a simple fix but i'm not acquainted with the Rack or Grape/GrapeEntity codebase enough to decide where the fix should occur...

Here's the issue:

• Rack::JSONP is given a GrapeEntity object as the response body.
• Rack::JSONP calls to_s on the response body (presuming it was going to be handed a json formatted string) - You can see that here: https://github.com/rack/rack-contrib/blob/1.1.0/lib/rack/contrib/jsonp.rb#L59 .
• The request for a GrapeEntity when using Rack::JSONP ends up looking like this: myCallbackName(#<MyAPI::Entities::V1::Camera:0x007f9d5e82f610>)

I'm presuming the best fix will be one of the following:

a) Convert object to a JSON string earlier in the lifecycle
b) Change the to_s method on GrapeEntity to alias to_json
c) Call Rack::JSONP later in the process (if possible)

I'm happy to do the work/a pull request for this one, but would like some guidance about where/how you feel it would be best fixed.

Kind regards,

Gareth

[dblock]

Rack::JSONP should not be getting a GrpeEntity object as the response body, the Grape formatters are doing the job of converting things. The test you wrote is fixed by adding a formatter :jsonp, lambda { |object, env| object.to_json }, what are you seeing that's broken after adding that?

[dblock]

... and really try to put this into a test, however clumsy it may be - maybe try mounting Rack::JSONP into an RSpec subject?

[deckchairhq]

@dblock Cheers, JSONP is called before the formatter.

I noted this comment from another (old) issue: #28 (comment)

His issue was not with entities but instead that the formatter was not setting the content-type before JSONP was called, in my case I've been setting use Rack::ContentType, "application/json" so I was getting further before encountering this bug.

It does seem that for both of us, formatter kicks in after other middleware.

I'll try and get a test done today.

[deckchairhq]

Here we go...

#352

[dblock]

I'll look into this, but do investigate moving the Middleware later in the stack.

[deckchairhq]

Hey,

How does this look?

https://github.com/Deckchair/grape/commit/da95278d1e4ba66b928a4b426d4e11b58e28e348

I've essentially moved the aggregate loop to the top of the build_middleware method. This passes all tests and works for me. As we don't have tests for external middleware that others may be using we should be careful about pulling this straight in.

What would you like as the next step for this? If you can't see any immediate issues with the change I can do a clean pull with an updated test?

Cheers,

G

[dblock]

I don't see anything wrong with this. If all specs pass and you have a new test for the JSONP issue we should trust our system to say we're good until proven otherwise.