Sutto / rocket_pants

API building tools on top of ActionController. Also, an awesome name.
MIT License
981 stars 130 forks source link

:response key breaks JSON API spec compatibility #125

Open jasonbosco opened 9 years ago

jasonbosco commented 9 years ago

The :response key that is implicitly included in the JSON that is generated by expose here, makes the JSON generated by rocket_pants incompatible with the JSON API specs.

According to the specs:

Primary data MUST appear under a top-level key named "data"

but in the JSON generated by rocket_pants, the implicit top-level key is response and is not configurable.

It would be great if there was an option to remove this top-level root key.

What do you guys think?

Sutto commented 9 years ago

This is a pretty big required thing for Rocket Pants 1.12 - I'd considered changing it before, but the issue was that making it dynamic was a rather painful change.

It's been planned for v2.0, but as looking at the bug tracker may show, that keeps getting pushed back due to limited time on my behalf.

jasonbosco commented 9 years ago

I came up with this monkey patch that makes the root element dynamic, based on an option called :top_level_root. It seems to be working. Can this have any other side effects though?

module RocketPants
  module Respondable
    private
    def respond_with_object_and_type(object, options, type, singular)
      pre_process_exposed_object object, type, singular
      options = options.reverse_merge(:compact => true) unless singular
      meta    = expose_metadata metadata_for(object, options, type, singular)
      render_json(json_response_object_for(object, options).merge(meta), options)
      post_process_exposed_object object, type, singular
    end

    def json_response_object_for(object, options)
      default_top_level_root = :response
      normalised_object      = normalise_object(object, options)

      if options[:top_level_root] == false
        normalised_object
      else
        top_level_root = options[:top_level_root].to_sym || default_top_level_root

        { top_level_root => normalised_object }
      end
    end
  end
end
Sutto commented 9 years ago

Ah, I'd misinterpreted that - I thought it needed dynamic keys. I'm not against a patch - In fact, if you write one (and add tests) I'd be more than happy to merge it in.

My only big concern: I don't want to support the case of no top level root, as that's actively a bad idea - It's mostly fixed, but I think not dealing with that case would greatly simplify the code.

The way I'd imagined it is as a rocket pants config setting - All we change is the response key to something else.

If you add a patch for that + a test, I'm happy to merge it - otherwise I can write it myself?

ahegyi commented 9 years ago

I've been able to change the root key at runtime using responds, would this be a viable solution?

responds(
  fascinating_object_collection,
  root: "data",
  each_serializer: FascinatingObjectSerializer,
  meta: # add other options, etc.
)

This breaks the rspec have_exposed matcher, though, since it always checks for the response key: https://github.com/Sutto/rocket_pants/blob/master/lib/rocket_pants/rspec_matchers.rb#L102

Sutto commented 9 years ago

I'd prefer it be a installation-level setting, but I can see where the above would be useful. If that's the case, maybe a way to pass the data root key to specs etc would be useful - configurable in the rspec config maybe?

ahegyi commented 9 years ago

I think my suggestion is more of a monkey patch when needing a dynamic root level key and when using ActiveModel Serializers. (In this case, it was about returning valid GeoJSON, and I needed "features" to be the root key.)

I agree it's probably best to limit Rocket Pants to have an installation level setting for the root key, and leave this sort of "solution" to one-off fixes :)