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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mhodgson
Copy link

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
Copy link
Owner

Sutto commented Oct 26, 2013

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
Copy link
Author

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
Copy link
Owner

Sutto commented Nov 3, 2013

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
Copy link
Contributor

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

@Sutto
Copy link
Owner

Sutto commented Nov 27, 2013

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

@martinstannard
Copy link

@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
Copy link
Contributor

fsmanuel commented Dec 5, 2013

@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
Copy link

@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
Copy link
Contributor

fsmanuel commented Dec 5, 2013

@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
Copy link
Owner

Sutto commented Dec 5, 2013

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

@fsmanuel
Copy link
Contributor

fsmanuel commented Dec 5, 2013

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
Copy link
Contributor

fsmanuel commented Dec 5, 2013

@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?

@ghost ghost assigned Sutto Dec 13, 2013
@Sutto
Copy link
Owner

Sutto commented Dec 13, 2013

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
Copy link
Contributor

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
Copy link
Owner

Sutto commented Dec 15, 2013

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
Copy link

looking forward to having this in 2.0 👍

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

Successfully merging this pull request may close these issues.

5 participants