Skip to content

Restrict serializable_hash to accepted options #1678

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

Merged
merged 1 commit into from
Apr 13, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/active_model/serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
# reified when subclassed to decorate a resource.
module ActiveModel
class Serializer
# @see #serializable_hash for more details on these valid keys.
SERIALIZABLE_HASH_VALID_KEYS = [:only, :except, :methods, :include, :root].freeze
extend ActiveSupport::Autoload
autoload :Adapter
autoload :Null
Expand Down
2 changes: 1 addition & 1 deletion lib/active_model_serializers/adapter/attributes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def initialize(serializer, options = {})
end

def serializable_hash(options = nil)
options ||= {}
options = serialization_options(options)
Copy link
Member Author

Choose a reason for hiding this comment

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

also see #965


if serializer.respond_to?(:each)
serializable_hash_for_collection(options)
Expand Down
8 changes: 8 additions & 0 deletions lib/active_model_serializers/adapter/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ def cached_name
@cached_name ||= self.class.name.demodulize.underscore
end

# Subclasses that implement this method must first call
# options = serialization_options(options)
def serializable_hash(_options = nil)
fail NotImplementedError, 'This is an abstract method. Should be implemented at the concrete adapter.'
end
Expand All @@ -41,6 +43,12 @@ def cache_check(serializer)

private

# see https://github.com/rails-api/active_model_serializers/pull/965
# When <tt>options</tt> is +nil+, sets it to +{}+
def serialization_options(options)
options ||= {} # rubocop:disable Lint/UselessAssignment
end

def meta
instance_options.fetch(:meta, nil)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/active_model_serializers/adapter/json.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ module ActiveModelSerializers
module Adapter
class Json < Base
def serializable_hash(options = nil)
options ||= {}
options = serialization_options(options)
serialized_hash = { root => Attributes.new(serializer, instance_options).serializable_hash(options) }
self.class.transform_key_casing!(serialized_hash, instance_options)
end
Expand Down
3 changes: 2 additions & 1 deletion test/action_controller/json_api/transform_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,11 @@ def render_resource_with_transform_nil
end

def render_resource_with_transform_with_global_config
setup_post
old_transform = ActiveModelSerializers.config.key_transform
setup_post
ActiveModelSerializers.config.key_transform = :camel_lower
render json: @post, serializer: PostSerializer, adapter: :json_api
ensure
ActiveModelSerializers.config.key_transform = old_transform
end
end
Expand Down
27 changes: 27 additions & 0 deletions test/adapter_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,33 @@ def test_serializable_hash_is_abstract_method
end
end

def test_serialization_options_ensures_option_is_a_hash
adapter = Class.new(ActiveModelSerializers::Adapter::Base) do
def serializable_hash(options = nil)
serialization_options(options)
end
end.new(@serializer)
assert_equal({}, adapter.serializable_hash(nil))
assert_equal({}, adapter.serializable_hash({}))
ensure
ActiveModelSerializers::Adapter.adapter_map.delete_if { |k, _| k =~ /class/ }
end

def test_serialization_options_ensures_option_is_one_of_valid_options
adapter = Class.new(ActiveModelSerializers::Adapter::Base) do
def serializable_hash(options = nil)
serialization_options(options)
end
end.new(@serializer)
filtered_options = { now: :see_me, then: :not }
valid_options = ActiveModel::Serializer::SERIALIZABLE_HASH_VALID_KEYS.each_with_object({}) do |option, result|
result[option] = option
end
assert_equal(valid_options, adapter.serializable_hash(filtered_options.merge(valid_options)))
ensure
ActiveModelSerializers::Adapter.adapter_map.delete_if { |k, _| k =~ /class/ }
end

def test_serializer
assert_equal @serializer, @adapter.serializer
end
Expand Down
16 changes: 16 additions & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,22 @@
require 'action_controller/test_case'
require 'action_controller/railtie'
require 'active_model_serializers'
# For now, we only restrict the options to serializable_hash/as_json/to_json
# in tests, to ensure developers don't add any unsupported options.
# There's no known benefit, at this time, to having the filtering run in
# production when the excluded options would simply not be used.
#
# However, for documentation purposes, the constant
# ActiveModel::Serializer::SERIALIZABLE_HASH_VALID_KEYS is defined
# in the Serializer.
ActiveModelSerializers::Adapter::Base.class_eval do
alias_method :original_serialization_options, :serialization_options

def serialization_options(options)
original_serialization_options(options)
.slice(*ActiveModel::Serializer::SERIALIZABLE_HASH_VALID_KEYS)
end
end
require 'fileutils'
FileUtils.mkdir_p(File.expand_path('../../tmp/cache', __FILE__))

Expand Down