trailblazer / roar-rails

Use Roar's representers in Rails.
http://roar.apotomo.de
MIT License
235 stars 65 forks source link

Error-Responses (422) are broken for custom formats #115

Closed besief closed 9 years ago

besief commented 9 years ago

Say you define a custom format in initializers/mime_type.rb like this: Mime::Type.register "application/vnd.example-com.custom_format+json", :custom_format and make your controller do represents :custom_format, entity: MyEntityRepresenter, collection: MyCollectionRepresenter (where the output format is simply JSON, but under a different name, i.e. the representers just include Roar::JSON). If you then want to return a 422 response for a request in that format, there will be odd behaviour – in our setup, the response contained only the name of the object's erronous field, but not even quoted, i.e. not looking like JSON at all.

Simple and clean way to fix this seems to be adding a _render_option_custom_format method in Roar::Rails::ControllerAdditions, in the same vain as _render_option_json etc., which are already defined there. Now I think it would be nice to have this automated or at least documented, because it's kinda hard to dig this one out by yourself. :)

Regards

apotonick commented 9 years ago

What is #_render_option_custom_format? Feel free to PR - I am not using roar-rails at the moment and don't know what can be made better.

besief commented 9 years ago

Ok, after looking further into this, I'm sorry I reported it. The proper fix is, of course, to implement #custom_format_resource_errors in my responder (subclassing Roar::Rails::Responder). Still, as #json_resource_errors is implemented in ActionController::Responder, it doesn't really have anything to do with this gem. :)

apotonick commented 9 years ago

I'd be interested to see that overridden method/responder - any chance you can paste it here for a better understanding (and discuss merge)? :heart_eyes:

besief commented 9 years ago

I'm afraid there's nothing to merge, as my format is entirely custom. The thing is that we have an API which can return different representations, that are all JSON of some kind, but not identical. Some fields may be left out etc. So these formats are named, say, :foo, :bar, :baz. Now for normal JSON and a 422 response, the execution path goes through ActionController::Responder#json_resource_errors, which turns the response content into a Hash that's just {errors: model.errors}. But for :foo, the lookup of #foo_resource_errors on the responder fails, so that Hash never gets generated. After that, the execution path doesn't really matter, I guess, because the damage has already been done, i.e. we never get to that Hash after that. What did happen after the change I described in the OP, was that I basically got model.errors.to_json back instead of model.errors.keys.first.to_s, which is, of course, really weird, but ultimately, I guess, just a case of "ex falso quodlibet". ;) I mean, #_render_option_foo should be defined anyways, because I called

ActionController::Renderers.add :foo do |js, options|
  self.content_type ||= Mime[:foo]
  js
end

like the documentation already advises. :)