Sutto / rocket_pants

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

Respect ActiveModel::Serializer's setting for including root in the JSON #79

Open mhodgson opened 11 years ago

mhodgson commented 11 years ago

Right now it is impossible to use serializers and have the root included in the JSON. Allowing root in the JSON is important for some features of serializers (embed) and also for integrating with front-end frameworks like Ember.js. This is a pretty big change that may catch some people by surprise, so there should probably be some discussion before it is merged.

Sutto commented 11 years ago

My ideal situation with this, at least by default, would be to enable it via a rocket pants setting and have it off by default - this means we wouldn't automatically break backwards compatibility, but people could enable it if they need it.

Any thoughts on adjusting the pull request to work in this manner?

mhodgson commented 11 years ago

I think that probably makes sense. I'm not sure what the setting should be called since the difference is slightly nuanced. The most explicit name would probably be something like 'serializer_serialization_method' (as_json or serializable_hash) but it feels a bit low level to me. Something like 'allow_root_in_json' might be more appropriate but doesn't really tell the whole story. Thoughts?

Sutto commented 10 years ago

I'm not sure honestly - I'd tend to say the latter makes more sense, maybe with some docs on integrating with ember for context?

fsmanuel commented 10 years ago

why not go with the default_serializer_options in the controller AMS is using?

Sutto commented 10 years ago

@fsmanuel would you be able to clarify a little on what you mean by this?

martinstannard commented 10 years ago

@Sutto I assume @fsmanuel is referring to https://github.com/rails-api/active_model_serializers#4-define-default_serializer_options-in-your-controller. I'd be keen see something like this rolled into rocket_pants as I'd like to use it with ember.js too, and this is a blocker at the moment.

fsmanuel commented 10 years ago

@martinstannard you are right. that's what i'm referring to. but i have to dig into rocket_pants and active model serializer to suggest a solution to the problem. will look into it later. what version of ember data do you use? I'm on 1.0.0-beta.3 and have a custom serializer which works without the root. i can share it with you if u want.

martinstannard commented 10 years ago

@fsmanuel I'm on master as of a few days ago - just trying to convert my app from 0.13 to 1.0.0. Thanks, it'd be great to checkout your serializer.

fsmanuel commented 10 years ago

@martinstannard here we go:

App.ApplicationSerializer = DS.ActiveModelSerializer.extend
  normalizePayloadBeforeExtract: (type, payload) ->
    root = @keyForAttribute(type.typeKey)
    payload[Ember.String.pluralize(root)] = payload.response

    # TODO what to do with the meta data?
    delete payload.count
    delete payload.pagination
    delete payload.response

    payload

  extractSingle: (store, type, payload, recordId, requestType) ->
    payload = @normalizePayloadBeforeExtract(type, payload)
    @_super(store, type, payload, recordId, requestType)

  extractArray: (store, type, payload) ->
    payload = @normalizePayloadBeforeExtract(type, payload)
    @_super(store, type, payload)

I'm not jet sure how i use the meta information provided by rocket pants (see comment above) but will keep you posted.

Sutto commented 10 years ago

Ah, good idea - I'm working on adding default_serializer_options right now and will update here once it's supported.

fsmanuel commented 10 years ago

The custom serializer is always needed as long as we stick with the repsonse root. i'll make it a RocketPantsActiveModelSerializer and put it up on github for other folks.

fsmanuel commented 10 years ago

@Sutto can you help me to understand how the use of the serializer works?

def serializable_hash(options = {})
  instance = serializer.new(object, options)
  if instance.respond_to?(:serializable_hash)
    instance.serializable_hash
  else
    instance.as_json options
  end
end

Why is it that we want the serializable_hash in here (if instance.respond_to?(:serializable_hash))? Don't we want the as_json all the time when using AMS? The options are always empty? We should check for default_serializer_options in here and pass it to as_json. As long as there is no self.root = false in the serializer or root: false in the default_serializer_options AMS will include the root. To be backward compatible we could introduce a config.rocket_pants.include_ams_root = true | false with a default to false. The config can be used to set:

if (config.rocket_pants.include_ams_root == false)
  # Disable for all serializers (except ArraySerializer)
  ActiveModel::Serializer.root = false
  # Disable for ArraySerializer
  ActiveModel::ArraySerializer.root = false
end

What do you think?

Sutto commented 10 years ago

I complete missed this before - sorry for the delay.

  1. Since we had AMS support, we've also had default_serializer_options - which should exist in your controller (and sets root to explicitely).
  2. Re. as_json vs serializable_hash, serializable_hash is the preferred method in Rails / in general,

The reason we use the serializer wrapper is to do with the fact the versions of AMS we had to be compatible with originally didn't support arguments to serializable hash. AMS has changed since then it appears, so I'll need to revisit that - but I'm inclined to push the root handling / generation into RocketPants (as I'm doing in 2.0), since that remains serializer-agnostic, instead of doing hacks around AMS (if that makes sense?)

fsmanuel commented 10 years ago

i was looking into how AMS does it and found some hints: render_option_json, build_json_serializer build_json_serializer could replace serializer = object.active_model_serializer in def self.normalise_to_serializer That way we don't have to care about default_serializer_options and passing or merging them into options.

Sutto commented 10 years ago

The big issue with the current implementation in ActiveModelSerializers is that doing it that way means tieing it pretty heavily to how AMS does stuff internally, which I'm not a big fan of if I can avoid it. I'm trying to make AMS supported with a minimum of ease, and so will likely bring across most of the features - but I wan't to avoid adding it as a direct dependency.

The current goals of the new structure I'm doing for this are:

  1. Support for using the root from AMS - This is a big one, since it means we then have drop in support (in theory) for Ember and co.
  2. Easier to add support for other things - I want to make it possible, even trivial, to have automatic support for things like Roar and such as needed.

Unfortunately, the version of AMS on master is not yet release - so I can't use the methods directly build in. The logic has stayed pretty sane, so I like the idea of sticking with that - and just cleaning up how we integrate.

If you're interested, I've pushed a very-much WIP of the support I've been working on here - The thing to note with this is it adds a layer of indirection (I'm still not convinced on this) and does a lot of lookups - which, for larger collections of things / complex serialization trees, I want to avoid.

The added bonus of this refactoring is it makes things like will_paginate and kaminari easier to support out of the box with very little code - see the other converters for examples of this.

lightswitch05 commented 10 years ago

looking forward to having this in 2.0 :+1: