-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Encapsulate serialization in ActiveModel::SerializableResource #954
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,8 @@ module Serialization | |
|
||
include ActionController::Renderers | ||
|
||
ADAPTER_OPTION_KEYS = [:include, :fields, :adapter] | ||
# Deprecated | ||
ADAPTER_OPTION_KEYS = ActiveModel::SerializableResource::ADAPTER_OPTION_KEYS | ||
|
||
included do | ||
class_attribute :_serialization_scope | ||
|
@@ -18,48 +19,37 @@ def serialization_scope | |
respond_to?(_serialization_scope, true) | ||
end | ||
|
||
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 = {}) | ||
if ! use_adapter? | ||
warn "ActionController::Serialization#use_adapter? has been removed. "\ | ||
"Please pass 'adapter: false' or see ActiveSupport::SerializableResource#serialize" | ||
options[:adapter] = false | ||
end | ||
|
||
@_serializer | ||
end | ||
|
||
def use_adapter? | ||
!(@_adapter_opts.key?(:adapter) && !@_adapter_opts[:adapter]) | ||
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 | ||
|
||
ActiveModel::SerializableResource.serialize(resource, options) do |serializable_resource| | ||
if serializable_resource.serializer? | ||
serializable_resource.serialization_scope ||= serialization_scope | ||
serializable_resource.serialization_scope_name = _serialization_scope | ||
begin | ||
serialized = serializer.new(resource, @_serializer_opts) | ||
serializable_resource.adapter | ||
rescue ActiveModel::Serializer::ArraySerializer::NoSerializerError | ||
else | ||
resource = ActiveModel::Serializer::Adapter.create(serialized, @_adapter_opts) | ||
resource | ||
end | ||
else | ||
resource | ||
end | ||
|
||
super(resource, options) | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thought this was better than a straight-up deprecation just in case anyone is using these methods. But this won't handle people that override the methods and expect them to have some effect. I'm not sure the best way to handle that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is good. We're currently in an RC, so people will have time to see these deprecations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Glad you support that. I'm really unhappy with straight-up removing them, but didn't come up with any code solutions that I thought were an improvement over just failing. Although, I think my implementation is wrong, in that if someone overrides these methods, they won't get a failure. So, that leaves me with doing a method_defined? check at boot/inheritance and/or taking a look at method missing.. ugh.. I'm going to take another look to see if I can keep the code alive a little longer. |
||
|
||
def rescue_with_handler(exception) | ||
@_serializer = nil | ||
@_serializer_opts = nil | ||
@_adapter_opts = nil | ||
# Deprecated | ||
def use_adapter? | ||
true | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could remove the method and the warning. Any specific reasons to keep it @bf4 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only because someone might have used it to turn off the adapter and hence skip the serializer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm, okay, lets just not forget of removing it after a while, maybe on official 0.10.x release, I'm not sure. tks 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy to add a self-destructing 'raise if AMS.version > 0.10.0' type of thing. That sorta what you're thinking about? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is a blocker for you, I'd rather remove it, merge, and discuss in subsequent issues or PRs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, not a blocker! 😄 Let's keep it for now. Just wanted to hear @kurko opinion before I merge it because he was following it too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the meantime, would you like me to rebase the renamed builder -> serializable_resource? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Baby steps. No need to cause so many changes in one PR. If you're refactoring, try to avoid change of behaviors in the same go. |
||
|
||
super(exception) | ||
[:_render_option_json, :_render_with_renderer_json].each do |renderer_method| | ||
define_method renderer_method do |resource, options| | ||
serializable_resource = get_serializer(resource, options) | ||
super(serializable_resource, options) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the Controller using the builder block form to conditionally return the adapter or the resource |
||
end | ||
|
||
module ClassMethods | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
require "set" | ||
module ActiveModel | ||
class SerializableResource | ||
|
||
ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter]) | ||
|
||
def initialize(resource, options = {}) | ||
@resource = resource | ||
@adapter_opts, @serializer_opts = | ||
options.partition { |k, _| ADAPTER_OPTION_KEYS.include? k }.map { |h| Hash[h] } | ||
end | ||
|
||
delegate :serializable_hash, :as_json, :to_json, to: :adapter | ||
|
||
# Primary interface to building a serializer (with adapter) | ||
# If no block is given, | ||
# returns the serializable_resource, ready for #as_json/#to_json/#serializable_hash. | ||
# Otherwise, yields the serializable_resource and | ||
# returns the contents of the block | ||
def self.serialize(resource, options = {}) | ||
serializable_resource = SerializableResource.new(resource, options) | ||
if block_given? | ||
yield serializable_resource | ||
else | ||
serializable_resource | ||
end | ||
end | ||
|
||
def serialization_scope=(scope) | ||
serializer_opts[:scope] = scope | ||
end | ||
|
||
def serialization_scope | ||
serializer_opts[:scope] | ||
end | ||
|
||
def serialization_scope_name=(scope_name) | ||
serializer_opts[:scope_name] = scope_name | ||
end | ||
|
||
def adapter | ||
@adapter ||= ActiveModel::Serializer::Adapter.create(serializer_instance, adapter_opts) | ||
end | ||
alias_method :adapter_instance, :adapter | ||
|
||
def serializer_instance | ||
@serializer_instance ||= serializer.new(resource, serializer_opts) | ||
end | ||
|
||
# Get serializer either explicitly :serializer or implicitly from resource | ||
# Remove :serializer key from serializer_opts | ||
# Replace :serializer key with :each_serializer if present | ||
def serializer | ||
@serializer ||= | ||
begin | ||
@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) | ||
end | ||
@serializer | ||
end | ||
end | ||
alias_method :serializer_class, :serializer | ||
|
||
# True when no explicit adapter given, or explicit appear is truthy (non-nil) | ||
# False when explicit adapter is falsy (nil or false) | ||
def use_adapter? | ||
!(adapter_opts.key?(:adapter) && !adapter_opts[:adapter]) | ||
end | ||
|
||
def serializer? | ||
use_adapter? && !!(serializer) | ||
end | ||
|
||
private | ||
|
||
attr_reader :resource, :adapter_opts, :serializer_opts | ||
|
||
end | ||
end |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
module ActionController | ||
module Serialization | ||
class ImplicitSerializerTest < ActionController::TestCase | ||
include ActiveSupport::Testing::Stream | ||
class ImplicitSerializationTestController < ActionController::Base | ||
def render_using_implicit_serializer | ||
@profile = Profile.new({ name: 'Name 1', description: 'Description 1', comments: 'Comments 1' }) | ||
|
@@ -140,10 +141,7 @@ def render_fragment_changed_object_with_relationship | |
|
||
private | ||
def generate_cached_serializer(obj) | ||
serializer_class = ActiveModel::Serializer.serializer_for(obj) | ||
serializer = serializer_class.new(obj) | ||
adapter = ActiveModel::Serializer.adapter.new(serializer) | ||
adapter.to_json | ||
ActiveModel::SerializableResource.new(obj).to_json | ||
end | ||
|
||
def with_adapter(adapter) | ||
|
@@ -400,6 +398,28 @@ def test_cache_expiration_on_update | |
assert_equal 'application/json', @response.content_type | ||
assert_equal expected.to_json, @response.body | ||
end | ||
|
||
def test_warn_overridding_use_adapter_as_falsy_on_controller_instance | ||
controller = Class.new(ImplicitSerializationTestController) { | ||
def use_adapter? | ||
false | ||
end | ||
}.new | ||
assert_match /adapter: false/, (capture(:stderr) { | ||
controller.get_serializer(@profile) | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here's the test for when someone has defined |
||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗ The two failing controller tests are because the 'deprecated' controller methods that raise the error are being overridden, and hence not called. I'm not sure how to handle the scenario where someone has written a |
||
|
||
def test_dont_warn_overridding_use_adapter_as_truthy_on_controller_instance | ||
controller = Class.new(ImplicitSerializationTestController) { | ||
def use_adapter? | ||
true | ||
end | ||
}.new | ||
assert_equal "", (capture(:stderr) { | ||
controller.get_serializer(@profile) | ||
}) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
require 'test_helper' | ||
|
||
module ActiveModel | ||
class SerializableResourceTest < Minitest::Test | ||
def setup | ||
@resource = Profile.new({ name: 'Name 1', description: 'Description 1', comments: 'Comments 1' }) | ||
@serializer = ProfileSerializer.new(@resource) | ||
@adapter = ActiveModel::Serializer::Adapter.create(@serializer) | ||
@serializable_resource = ActiveModel::SerializableResource.new(@resource) | ||
end | ||
|
||
def test_serializable_resource_delegates_serializable_hash_to_the_adapter | ||
options = nil | ||
assert_equal @adapter.serializable_hash(options), @serializable_resource.serializable_hash(options) | ||
end | ||
|
||
def test_serializable_resource_delegates_to_json_to_the_adapter | ||
options = nil | ||
assert_equal @adapter.to_json(options), @serializable_resource.to_json(options) | ||
end | ||
|
||
def test_serializable_resource_delegates_as_json_to_the_adapter | ||
options = nil | ||
assert_equal @adapter.as_json(options), @serializable_resource.as_json(options) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be the right place or way to catch the failure when instantiating the serializer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Baby steps 😁