RFC: Rails-API: Proposed better interface for registering serializers with the controller

509 views
Skip to first unread message

Benjamin Fleischer

unread,
Jun 2, 2015, 5:17:48 PM6/2/15
to rubyonra...@googlegroups.com
Adapted from original comment at https://github.com/rails/rails/pull/19832#discussion_r30468412

Since this post is long, I've divided it into sections: 
  1. Summary
  2. Background 
    1. How Serializers Work Now
    2. How Renderers Work Now
    3. Example code changing the renderer
    4. Example code changing the serializer
  3. Proposal


Summary:

I propose there be a first-class serializer interface in Rails much like there is for renderers.
Proposed interfaces are: serializer_for  and ActionController::Serializers.register

Background:

  How serializers work now:
Right now, that interface is a private-looking method :_render_with_renderer_json or:_render_option_json, (e.g. see ActionController:: Serialization)

Such that creating a serializer is complicated and non-obvious

# When Rails renders JSON, it calls the json renderer method
# "_render_with_renderer_#{key = json}"
# https://github.com/rails/rails/blob/8c2c1bccccb3ea1f22503a184429d94193a1d9aa/actionpack/lib/action_controller/metal/renderers.rb#L44-L45
# which is defined in AMS https://github.com/rails-api/active_model_serializers/blob/1577969cb76309d1f48c68facc73e44c49489744/lib/action_controller/serialization.rb#L36-L37
    [:_render_option_json, :_render_with_renderer_json].each do |renderer_method|
      define_method renderer_method do |resource, options|
          super(adapter, options)
      end
    end


which is really hard to find in the source code... since it calls "_render_with_renderer_#{key}" where the key is json...

# https://github.com/rails/rails/blob/8c2c1bccccb3ea1f22503a184429d94193a1d9aa/actionpack/lib/action_controller/metal/renderers.rb#L44-L45
   def _render_to_body_with_renderer(options)
      _renderers.each do |name|
        if options.key?(name)
          _process_options(options)
          method_name = Renderers._render_with_renderer_method_name(name)
          return send(method_name, options.delete(name), options)
        end
      end
      nil
    end

    # A Set containing renderer names that correspond to available renderer procs.
    # Default values are <tt>:json</tt>, <tt>:js</tt>, <tt>:xml</tt>.
    RENDERERS = Set.new

    def self._render_with_renderer_method_name(key)
      "_render_with_renderer_#{key}"
    end
that is, it calls "send(:render_with_renderer_json, @model, options)" which is not greppable in the code base
(`@model` is the argument to json: in the controller, for example `render json: @model`)


  How renderers work now:

How does the renderer fit into this?  Well, when a controller has

render json: @model

the  @model  is passed to the JSON Renderer (see ActionController::Renderers default)

which basically calls  json = @model.to_json(options)

  Example code changing the renderer:

If, for example, I wanted to change how the JSON was rendered, to pretty print it, I would just redefine the :json renderer as below

# https://github.com/rails/rails/blob/4-2-stable/actionpack/lib/action_controller/metal/renderers.rb#L66-L128
# https://github.com/rails/rails/blob/4-2-stable//actionview/lib/action_view/renderer/renderer.rb#L32
 ActionController::Renderers.remove :json
 ActionController::Renderers.add :json do |json, options|
    if !json.is_a?(String)
      # changed from
      # json = json.to_json(options)
      # changed to
      json = json.as_json(options) if json.respond_to?(:as_json)
      json = JSON.pretty_generate(json, options)
    end

    if options[:callback].present?
      if content_type.nil? || content_type == Mime::JSON
        self.content_type = Mime::JS
      end

      "/**/#{options[:callback]}(#{json})"
    else
      self.content_type ||= Mime::JSON
      json
    end
  end
  Example code changing the serializer:

But, to change how the @model is serialized, a library such as what ActiveModelSerializers overrides :_render_option_json, :_render_with_renderer_json]

to basically change @model = ModelSerializer.new(@model) so that the renderer is calling to_json/as_json on the serializer

I think this could be way better:

PROPOSAL:

1)  Renderer could have a method serializer_for that can by default returns its argument, but can be overridden in the controller

    add :json do |json, options|
-      json = json.to_json(options) unless json.kind_of?(String)
+      json = serializer_for(json).to_json(options) unless json.kind_of?(String)

    example controller code:

def serializer_for(model)
  ActiveModel::Serializer::Adapter.create(
    ActiveModel::Serializer.serializer_for(resource).new(resource, serializer_opts),
    adapter_opts
  ) 
end

or

2) have a serializer registry (like renderers and mime-types have), that may be called in a method just as in #1

 ActionController::Serializers.register :user, UserSerializer, only: [:json]
Benefits:

- Simple, clear, extendable interface for model serialization
- Less meta-programming
- I wouldn't have needed to spend hours in the debugger and rails source code trying to find out the path from `render json: @model` to `_render_with_renderer_json(@model, options)`, 


Thanks for your thoughts!

-Benjamin

Benjamin Fleischer

unread,
Jun 11, 2015, 1:34:55 AM6/11/15
to rubyonra...@googlegroups.com
I know y'all on core are busy.. I thought this was a reasonable proposal to improve the usability or Rails, especially as rails-api gets merged in https://github.com/rails/rails/pull/19832

Besides JBuilder and https://github.com/rails-api/active_model_serializers, I know of  https://github.com/cerebris/jsonapi-resources ( which basically forces you to use their render call that wraps the model in a 'resource serializer' ) and https://github.com/RestPack/restpack_serializer ( which requires you wrap your model in its serializer in the controller yourself ).  I think they'd all benefit from having a nice place in the request lifecycle where the how the rendered resource is serialized can be configured.

Anyhow, basically justing bumping to see if there's interest in a PR

Benjamin Fleischer

unread,
Jun 24, 2015, 12:07:14 PM6/24/15
to rubyonra...@googlegroups.com
I'm a little bummed no one responded to this.  Here's some new info: ActiveModelSerializers already mixes in `get_serializer` to controllers.



- def get_serializer(resource)
- @_serializer ||= @_serializer_opts.delete(:serializer)
- @_serializer ||= ActiveModel::Serializer.serializer_for(resource)
-
- if @_serializer_opts.key?(:each_serializer)
- @_serializer_opts[:serializer] = @_serializer_opts.delete(:each_serializer)
+ def get_serializer(resource, options = {})
+ serializable_resource = ActiveModel::Serializer::build(resource, options) do |builder|
+ if builder.serializer?
+ builder.serialization_scope ||= serialization_scope
+ builder.serialization_scope_name = _serialization_scope
+ builder.adapter
+ else
+ resource
+ end
end
-
- @_serializer
end

[:_render_option_json, :_render_with_renderer_json].each do |renderer_method|
define_method renderer_method do |resource, options|
- @_adapter_opts, @_serializer_opts =
- options.partition { |k, _| ADAPTER_OPTION_KEYS.include? k }.map { |h| Hash[h] }
-
- if use_adapter? && (serializer = get_serializer(resource))
-
- @_serializer_opts[:scope] ||= serialization_scope
- @_serializer_opts[:scope_name] = _serialization_scope
-
- # omg hax
- object = serializer.new(resource, @_serializer_opts)
- adapter = ActiveModel::Serializer::Adapter.create(object, @_adapter_opts)
- super(adapter, options)
- else
- super(resource, options)
- end
+ serializable_resource = get_serializer(resource, options)
+ super(serializable_resource, options)
end
end

Thanks for any eyeballs on this.  I want to help!

-Benjamin

Rafael Mendonça França

unread,
Jun 24, 2015, 12:11:37 PM6/24/15
to rubyonra...@googlegroups.com
Go for it, this will improve the way different serializers hook inside Rails so I don't see any reason to not do so.

Godfrey Chan

unread,
Jun 24, 2015, 1:43:14 PM6/24/15
to rubyonra...@googlegroups.com
I had a related user case at some point that led me down to path of the passive_model_serializers experiment.

Basically, the observation is that serializers:models is a N:1 relationship, not 1:1. So imo asking the model what its serializer should be (or as proposed, register one serializer per model type) is fundamentally problematic/flawed.

In theory, you might want a serializer (not necessarily a AM::S, but the concept in general) for exporting the "transactions" table in CSV; at the same time, you might have two different "transactions" serializers for the merchant dashboard and the admin dashboard, maybe another one for the legacy mobile app API.

Therefore, which serializer to use (in the case where you have more than one) depends on the context of the serialization, so you really should be asking the thing that is trying to serialize the model (in most case, the controller) rather than the model that is being serialized.

That's basically the spirit of the passive_model_serializers experiment. There are more nuances to that - inside your AdminTransactionsSerializer, if you want to serialize a user, chances are you probably want to use an AdminUserSerializer as well, but that serializer in turns want to serialize another object, you need to set that up properly too, which, in the then-current AM::S API required a lot of boilerplate.

I stopped working on the project that caused my pain, so I stopped working on the experiment too :P I'm not super happy with the state I left it at, so I never took the time to write up a proposal.. (so thank you for doing that :D)

So... In the context of this proposal, between the two things you proposed, my personal preference is to keep this decision inside the controller, instead of a 1:1 model-serializer map. (Also, the "User" class -> "UserSerializer" thing worked well for simple cases, and we should keep that working; but I guess that's an AM::S concern.)

Maybe something along the lines of...


class ActionController::Base
  
  # default implementation that ships with Rails
  class ToJsonSerializer < Struct.new(:object, :options)
    def serialize
      object.to_json(options)
    end
  end

  # default implementation that ships with Rails
  def serializer_for(object, options)
    ToJsonSerializer.new(object, options)
  end
 
end

...which can be overridden by AM::S or by yourself in your controller to do the right thing.

* * *

But at the end of the day, I'm not sure if this is really /that/ much different from what we have today.

At the high level, you are proposing that (correct me if I am wrong), when you call `render format: model, some_options`, we should look up an object that knows how to serialize model into "format" and get a string back. That sounds like... basically the same architecture as the current renderer set up, but with a different name than what you expected.

The renderer is basically a "serializer" that takes an object and an options hash and returns a string. The reason it is not greppable and uses meta-programming is that we allow for a huge list of formats other than json, and this is a generic set up that works well for most formats (including, imo, json). Do we only try to look up a serializer object for `render json: ...`? What about `render xml: ...`? `render text: ...`? So long as we don't special-case json or a handful of things (imo, that seems like a bad idea), I think we will still end up with the same problem with metaprogramming and greppability.

So, I wonder if we should just better document this plugin API and fix any flaws and annoyances you find along the way.

What do you think?

(Thanks again for taking the time to write a detailed proposal, really appreciate it!)

Godfrey


--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
To post to this group, send email to rubyonra...@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.

Benjamin Fleischer

unread,
Jun 24, 2015, 2:01:21 PM6/24/15
to rubyonra...@googlegroups.com

On Wed, Jun 24, 2015 at 12:42 PM, Godfrey Chan <godfr...@gmail.com> wrote:
The renderer is basically a "serializer" that takes an object and an options hash and returns a string. The reason it is not greppable and uses meta-programming is that we allow for a huge list of formats other than json, and this is a generic set up that works well for most formats (including, imo, json). Do we only try to look up a serializer object for `render json: ...`? What about `render xml: ...`? `render text: ...`? So long as we don't special-case json or a handful of things (imo, that seems like a bad idea), I think we will still end up with the same problem with metaprogramming and greppability.

Thanks Godfrey, I agree with you 100% here.  I was thinking of refining this to something that took the render used into account.  I'm not exactly proposing the bolded line there, but something like that would extend the current logic.  In any case, sounds like a PR to document this would at least be useful.  I'll take a look at your code and see what ideas are in there

def _render_to_body_with_renderer(options) 
  _renderers.each do |name| 
    if options.key?(name)
      _process_options(options)
      method_name = Renderers._render_with_renderer_method_name(name)
      object = Serializers._serialize_with_serializer_json(options.delete(name))
      return send(method_name, object, options)
    end
  end
  nil
end

or a controller method that takes in the renderer name somehow (again, just for illustration of that idea)

def get_serializer(resource, options)
  renderer_method_name = options.delete(:renderer_method_name)
  fail if renderer_method_name.blank?
  case renderer_method_name
  when :json then resource.as_json
  end
end


-Benjamin

Benjamin Fleischer

unread,
Nov 22, 2015, 1:54:36 PM11/22/15
to Ruby on Rails: Core, benj...@benjaminfleischer.com
Well, I've updated to PR to a state that I think improves on the status quo and is tested.  It should be a good point for discussion.

See https://github.com/rails/rails/pull/21496 and previous discussion in this thread.

-Benjamin
Reply all
Reply to author
Forward
0 new messages