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

JSON body not parsing into params automatically with format :serializable_hash. #326

Closed
Neil-Aframe opened this issue Jan 28, 2013 · 6 comments

Comments

@Neil-Aframe
Copy link

This was previously opened as issue #64, and seems to of regressed with recent changes.

I would like my API to process JSON message body when it receives a request with header "Content-Type: application/json", and to make that available to my application. It is a documented feature that Grape should handle this automatically, and I think a good design that it should do so.

I am not sure if there are better patterns already built into Grape for me to interact with incoming JSON in HTTP bodies? If so, it may be inexperience on my part, and I would be interested in seeing any better way.

For now, the following monkey-patch (taken from #64 and adjusted) solves things for me, but is probably now applied at the wrong place in the code structure, and may not cover all possible scenarios:

# Overload grape to parse application/json objects until this is fixed
module Grape
  class API
    class << self
      def call!(env)
        case env['CONTENT_TYPE']
        when /^application\/json/
          hash = env['rack.input'].read
          parsedForm = JSON.parse(hash) unless hash.blank?
          env.update('rack.request.form_hash' => parsedForm, 'rack.request.form_input' => env['rack.input']) if parsedForm
        end
        instance.call(env)
      end
    end
  end
end
@Neil-Aframe
Copy link
Author

Looks like you already have passing tests related to this issue.

However, I suspect rack/test is doing something extra or out-of-sequence (edit: my suspicions were incorrect, see next comment), and is hiding the fault. So I have reproduced the fault using a minimal grape configuration, and curl example.

When I rackup -p 8090 the class from here: https://gist.github.com/4657311

And run this

curl -d '{"foo":"bar"}' -H 'Content-Type: application/json' http://127.0.0.1:8090/api/echo

. . . then I see

{"params":{"route_info":"version=, method=POST, path=/api/echo(.:format)"}}

Where I would expect to see

{"params":{"foo":"bar","route_info":"version=, method=POST, path=/api/echo(.:format)"}}

@Neil-Aframe
Copy link
Author

I changed my example where it has

format :serializable_hash

to

format :json

and the issue no longer appears. There have been some changes to grape in this area too, so it looks like I have been caught out by them.

This is no longer an issue for me, I will use json format.

It is not clear to me whether this still constitutes a general issue - i.e. whether declaring format as "serializable_hash" should maintain json parsing of body on input or disable it.

@dblock
Copy link
Member

dblock commented Jan 28, 2013

Another workaround for this is to add

content_type :json, "application/json"

When you do format :anything, it removes all default formatters, parsers and error handlers, then re-adds any matching parsers, formatters or error handlers available from those defaults. We have a full set for :json, but nothing for serializable_hash.

We could fix this and make serializable_hash behave the same way as JSON in every other way than formatting, but right now I am more 80/20 should/shouldn't. This serializable_hash is a special formatter that, TBH, I think we should get rid of. I left it for backward compatibility, but most people have no good reason to use serializable_hash as far as I know and should be using plain JSON.

Do you know why you use serializable_hash? :)

@Neil-Aframe
Copy link
Author

I believed I used serializable_hash based on my reading of older 1.2.5 documentation, where it appeared not to be a format as such, but simply an automatic check on return value, which to me looked like a clean way of separating out content types from return values. I simplified my prototype code considerably by implementing a serializable_hash method on all my resource representations - and when that didn't work as expected, I searched for the reason why and discovered the serializable_hash format. So probably just my misunderstanding.

It now looks like the original intent behind serializable_hash has since been realised in grape in other, and better, ways?

I am quite happy to change my app to use format :json and take advantage of grape's defaults.

@dblock
Copy link
Member

dblock commented Jan 29, 2013

Yes, the real intent of serializable_hash was to render objects that respond to it in a somewhat different way than those that support as_json and to_json. The recommended way for a JSON API is to use format :json and implement as_json normally. You should, in theory, never have to call to_json or as_json explicitly either.

@dblock dblock closed this as completed Jan 29, 2013
@Neil-Aframe
Copy link
Author

Thanks for your fast and helpful responses!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants