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

Add configurable serializers in each renderer #21496

Closed
wants to merge 26 commits into from

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Sep 4, 2015

Controller Examples

class TheController < ApplicationController
  serializing json: ->(json, options) do
    return json if json.is_a?(String)
    json = json.as_json(options) if json.respond_to?(:as_json)

    JSON.pretty_generate(json, options)
  end
end
class UserController < ApplicationController
  serializing json: ->(json, options) { UserSerializer.new(json, options).as_json(options) }
end

Most relevant core contributor comments

Comment 2017-09-05

Not sure if it can use JBuilder. Per rails/jbuilder#321

class UserController < ApplicationController
  serializing json: ->(json, options) { 
    template = JbuilderTemplate.new(lookup_context)
    result = JbuilderHandler.call(template)
    JSON.pretty_generate(result)
  }
end

Update 2017-08-30

Update 2016-09-25

Waiting for further feedback from Rails core. Test failures aren't related.

Update 2016-05-22

there's been a lot of churn in this PR over the time it's been worked on. Below is the original description. Current contents is best understood by reading the diff and comments on it.

A good summary can be obtained by reading #21496 (comment) and #21496 (comment)

Original description 2015-09-04

per https://groups.google.com/forum/#!topic/rubyonrails-core/K8t4-DZ_DkQ/discussion

Since 2011, work in EmberJS and on the JSON API has resulted in some changing naming conventions

  • Model: class that defines the properties and behavior of the data that you present to the user.
  • Record: A record is an instance of a model that contains data loaded from a server. Your application can also create new records and save them back to the server.
  • Adapter: knows how to talk to your server. knows how to translate requests from the client into requests on your server. object that translates requests from AMS/Rails/Grape etc (such as "find the user with an ID of 123") into a requests to a server. let you completely change how your API is implemented without impacting your application code.
  • Serializer: maps keys and values to desired format, see https://github.com/orbitjs/orbit.js/blob/master/lib/orbit-common/jsonapi-serializer.js
  • Store: the central repository of records in your application, use the store to retrieve records, as well to create new ones. The store will automatically cache records for you.

@rails-bot
Copy link

r? @carlosantoniodasilva

(@rails-bot has picked a reviewer for you, use r? to override)

@bf4
Copy link
Contributor Author

bf4 commented Sep 4, 2015

Existing tests should ensure no regression. If we move forward with this design or other feedback, I'll add tests and documentation for the new features as well.

@bf4 bf4 force-pushed the serializer_for_renderer branch from 6a856f5 to ee0c4e5 Compare September 4, 2015 10:17
@bf4
Copy link
Contributor Author

bf4 commented Sep 4, 2015

r? @guilleiguaran

self._serializers = serializers.freeze
end
alias use_serializer use_serializers
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might not need this, but I haven't looked into this vector, yet

@bf4
Copy link
Contributor Author

bf4 commented Sep 9, 2015

cc @joaomdmoura

#
# Create a csv renderer:
#
# ActionController::Serializers.add :csv do |obj, options|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does we have a method add in ActionController::Serializers?

@rafaelfranca
Copy link
Member

API looks weird but implementation is good. Maybe it would be better if we don't have the Serializers module and only the methods inside the Renderers module.

@bf4
Copy link
Contributor Author

bf4 commented Sep 9, 2015

Thanks @rafaelfranca

only the methods inside the Renderers module.

That works for me, and probably makes more sense and is simpler to understand.

I also apologize that I don't seem to have noted that this is a proposed solution that doesn't break anything (per tests), and that if we go with it, I'll write tests and some docs. So, I especially thank your for treatment of a PR like this without tests :)

add :json do |json, options|
json = json.to_json(options) unless json.kind_of?(String)
json = _serialize_with_serializer_json(json, options)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I'm not a huge fan of this interface, I think it has the following benefits

  1. It serves to implicitly document the use of methods like _serialize_with_serializer_json, which makes them easier to search for
  2. It's a very small change
  3. I think it could be a good thing to separate the ides of a 'json renderer' that uses as 'json serializer', yet keep them tightly coupled in usage and code location, even though there is a slightly higher cognitive burden. Perhaps we should ensure the renderer has a helpful failure message if the serializer isn't defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe would be nicer to have a sort of serializer middleware, so that we could extend it? e.g. ActiveModel::Serializers transforms it to as_json and another layer takes calls to_json(options) on that or JSON.pretty_generate(json, options)

@bf4 bf4 force-pushed the serializer_for_renderer branch 2 times, most recently from 7f0a606 to a83d52f Compare September 10, 2015 15:39
@bf4
Copy link
Contributor Author

bf4 commented Sep 10, 2015

@rafaelfranca Updated

cc

I figure tests might go in (?)

and doc changes would go in

# json = CustomSerializer.new(json, options)
# super
# end
#
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting that you can't really look up a renderer/serializer right now, that the dance Rails is doing is mapping e.g. :json to _render_with_renderer_json(json, options), e.g. that to render to json, it iterates, builds a method name, and sends to it

    def render_to_body(options)
      _render_to_body_with_renderer(options) || super
    end

    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

That there's no ActionController::Renderers.for(:json).render(json, options) which might use an object like JsonRenderer or even the proc/block defined as the body of _render_with_renderer_json.

And having ActionController::Rendering.renderer call ActionController::Renderer.for(controller) which creates and anonymous subclass of ActionController::Renderer it appears is only used for templates?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this API is something we can improve in a new PR

@bf4
Copy link
Contributor Author

bf4 commented Sep 21, 2015

Is there anything I can do to help move this along?

@@ -11,28 +11,61 @@ def self.remove_renderer(key)
Renderers.remove(key)
end

# RFC: Where is this used?
class MissingRenderer < LoadError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is not being used

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good spleunking. Now, the code over there is

    # This is the common behavior for formats associated with APIs, such as :xml and :json.
    def api_behavior
      raise MissingRenderer.new(format) unless has_renderer?
      # etc
    end

    # Check whether the necessary Renderer is available
    def has_renderer?
      Renderers::RENDERERS.include?(format)
    end

which makes me think either the exception should be defined in responders or has_renderer? should be defined here, as well.

@rafaelfranca
Copy link
Member

figure tests might go in (?)

https://github.com/rails/rails/blob/master/actionpack/test/controller/renderer_test.rb
https://github.com/rails/rails/blob/master/actionpack/test/controller/render_xml_test.rb
https://github.com/rails/rails/blob/master/actionpack/test/controller/mime/respond_to_test.rb#L139
https://github.com/rails/rails/blob/master/actionpack/test/controller/render_json_test.rb#L90
it looks like some tests may have been removed in ee77770#diff-28658021f35e96c7499fdf41908629f0L644 ?

def test_uses_renderer_if_an_api_behavior
ActionController::Renderers.add :csv do |obj, options|
send_data obj.to_csv, type: Mime::CSV
end
@controller = CsvRespondWithController.new
get :index, format: 'csv'
assert_equal Mime::CSV, @response.content_type
assert_equal "c,s,v", @response.body
ensure
ActionController::Renderers.remove :csv
end
def test_raises_missing_renderer_if_an_api_behavior_with_no_renderer
@controller = CsvRespondWithController.new
assert_raise ActionController::MissingRenderer do
get :index, format: 'csv'
end
end
def test_removing_renderers
ActionController::Renderers.add :csv do |obj, options|
send_data obj.to_csv, type: Mime::CSV
end
@controller = CsvRespondWithController.new
@request.accept = "text/csv"
get :index, format: 'csv'
assert_equal Mime::CSV, @response.content_type
ActionController::Renderers.remove :csv
assert_raise ActionController::MissingRenderer do
get :index, format: 'csv'
end
ensure
ActionController::Renderers.remove :csv
end

👍

bf4 added 15 commits September 7, 2017 14:16
Conflicts:
	actionpack/CHANGELOG.md
…erver': DRb::DRbServerNotFound (DRb::DRbConnError)

```
rails/actionpack

📝 $ bundle exec ruby -Ilib:test test/controller/renderers_test.rb --seed 41487
Run options: --seed 41487

Running:

.....
~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:1726:in `current_server': DRb::DRbServerNotFound (DRb::DRbConnError)
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:1795:in `to_id'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:1103:in `initialize'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:651:in `new'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:651:in `make_proxy'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:568:in `rescue in dump'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:565:in `dump'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:612:in `block in send_request'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:611:in `each'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:611:in `send_request'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:926:in `send_request'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:1253:in `send_message'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:1142:in `block (2 levels) in method_missing'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:1229:in `open'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:1141:in `block in method_missing'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:1160:in `with_friend'
        from ~/.rvm/rubies/ruby-2.4.1/lib/ruby/2.4.0/drb/drb.rb:1140:in `method_missing'
        from rails/actionpack/test/abstract_unit.rb:404:in `block (2 levels) in shutdown'
        from rails/actionpack/test/abstract_unit.rb:393:in `fork'
        from rails/actionpack/test/abstract_unit.rb:393:in `block in shutdown'
        from rails/actionpack/test/abstract_unit.rb:392:in `times'
        from rails/actionpack/test/abstract_unit.rb:392:in `each'
        from rails/actionpack/test/abstract_unit.rb:392:in `map'
        from rails/actionpack/test/abstract_unit.rb:392:in `shutdown'
        from rails/bundle/ruby/2.4.0/gems/minitest-5.10.3/lib/minitest.rb:140:in `run'
        from rails/bundle/ruby/2.4.0/gems/minitest-5.10.3/lib/minitest.rb:63:in `block in autorun'
.

Finished in 0.056913s, 105.4235 runs/s, 245.9882 assertions/s.
6 runs, 14 assertions, 0 failures, 0 errors, 0 skips
```
@bf4 bf4 force-pushed the serializer_for_renderer branch 2 times, most recently from b1ce853 to 6962db8 Compare September 13, 2017 20:55
@bf4 bf4 force-pushed the serializer_for_renderer branch from 6962db8 to 4093649 Compare September 13, 2017 21:11
@bf4
Copy link
Contributor Author

bf4 commented Sep 13, 2017

@rafaelfranca below are the relevant commits to review. I made a kind of clean slate within the PR. You can see some of the commits are refactors that aren't strictly necessary.

Message ref
Merge branch 'master' into serializer_for_renderer 4e7fdf0
Make even with master to start impl over c848569
Add actionpack/CHANGELOG to document use_renderers 1f06aa7
Make test description/render format more explicit 596179a
Test explicit undefined renderer raises MissingTemplate e6091fc
Remove unnecessary self.response_body = d305814
Implement minimal ActionController.add/remove_serializer a158880
Raise MissingSerializer when only Renderer defined c124ffb
Assert exception messages 1e5b552
Add Controller.serializing mime: ->(obj, options) { } … 70b2f85
Refactor _render_to_body_with_renderer cd00431
Improve naming in _render_to_body_with_renderer 44a68f0
Add changelog 4093649

Things that I think need attention

  • Any refactors or changes to remove?

  • That all the comments and documentation make sense, have no typos, are useful

  • Consider adding a rescue_response to actionpack/lib/action_dispatch/middleware/exception_wrapper.rb for MissngSerializer and MissingRenderer

  • Would be good to allow specifying the serializer as an argument to render, e.g.

-        serialized_value = _serializers[renderer_format].call(value_to_render, options)
+        serializer_name = (options.delete(:serializer_name) || renderer_format).to_sym
+        serialized_value = _serializers[serializer_name].call(value_to_render, options)

and

--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -16,11 +16,16 @@
     Renderers now only handle non-serialization concerns, such as setting the
     mime-type, how the data is returned, and handling callbacks.
 
-    The `serializer_name` is the name of the format being rendered, e.g. `json`.
+    The `serializer_name` is either from the params `serializer_name`, when given, or
+    the name of the format being rendered, e.g. `json`.
 
     The serializer is a callable object that accepts the same arguments as the renderer,
-    i.e. the object and the rendering options.  For example,
-    `render json: model` will serialize `model` as `_serializers[:json].call(model, options)`.
+    i.e. the object and the rendering options.
+        render json: model`
+        # => `_serializers[:json].call(model, options)
+
+        render json: model, serializer_name: :json_api
+        # =>  `_serializers[:json_api].call(model,options)
 
     The controller with raise an `ActionController::MissingSerializer` if no serializer is found
     for the format being rendered.
  • Confirm there are no thread-safety issues of runtime errors. Tests would probably go in actionpack/test/controller/metal/renderers_test.rb

  • Review interface and recommended usage, ActionController.add_renderer vs. ActionController::Renderers.add, etc.

  • Compare _renderers/use_renderer to _serializers/serializing that they make sense to exist as implemented.

  • Consider having the renderer redefine the method, rather than define it.

  • Should the default _renderers be RENDERERS.dup.freeze instead of Set.new.freeze? If not, should the default for _serializers still be SERIALIZERS.dup. Should it be frozen?

  • Consider raising MissingRenderer in Rails itself, not just in the responders gem.

--- a/actionpack/lib/action_controller/metal/renderers.rb
+++ b/actionpack/lib/action_controller/metal/renderers.rb
@@ -240,7 +240,9 @@ def serializing(options)
     # If no renderer is found, +super+ returns control to
     # <tt>ActionView::Rendering.render_to_body</tt>, if present.
     def render_to_body(options)
-      _render_to_body_with_renderer(options) || super
+      _render_to_body_with_renderer(options) \
+        || super \
+        || fail(ActionController::MissingRenderer.new(request.format))
     end

@rails-bot
Copy link

rails-bot bot commented Dec 18, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 18, 2019
@rails-bot rails-bot bot closed this Dec 25, 2019
@bf4
Copy link
Contributor Author

bf4 commented Jan 28, 2020

oh well

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

Successfully merging this pull request may close these issues.

9 participants