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.

Sign up for GitHub

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

Never mutate controller options #1572

Closed
wants to merge 3 commits into from
Closed

Never mutate controller options #1572

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80b09b1
Never mutate controller options
bf4 Mar 9, 2016
4b1ef59
trying to fix
bf4 Mar 9, 2016
5103fe5
Prevent controller options from being mutated
bf4 Mar 9, 2016
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
17 changes: 10 additions & 7 deletions lib/action_controller/serialization.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ def serialization_scope
respond_to?(_serialization_scope, true)
end

def get_serializer(resource, serializer_options = {})
def get_serializer(resource, serialization_options = {})
if !use_adapter?
Copy link
Member Author

Choose a reason for hiding this comment

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

I like the name change

warn 'ActionController::Serialization#use_adapter? has been removed. '\
"Please pass 'adapter: false' or see ActiveSupport::SerializableResource.new"
serializer_options[:adapter] = false
serialization_options[:adapter] = false
end
serializable_resource = ActiveModel::SerializableResource.new(resource, serializer_options)
serializable_resource = ActiveModel::SerializableResource.new(resource, serialization_options)
if serializable_resource.serializer?
serializable_resource.serialization_scope ||= serialization_scope
serializable_resource.serialization_scope_name = _serialization_scope
Expand All @@ -57,11 +57,14 @@ def use_adapter?
[:_render_option_json, :_render_with_renderer_json].each do |renderer_method|
define_method renderer_method do |resource, options|
options.freeze
serializer_options = options.deep_dup
serializer_options.fetch(:serialization_context) do
serializer_options[:serialization_context] = ActiveModelSerializers::SerializationContext.new(request, serializer_options)
serialization_options = options.deep_dup
Copy link
Member

Choose a reason for hiding this comment

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

@bf4 I'm not sure if it is good to deep_dup here as it also dup the serializer when present and that made the tests fail. That's why I had to do the workaround below. Is this really needed is your opinion?

Copy link
Member Author

Choose a reason for hiding this comment

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

wouldn't the serializer be a class at this point? How would that be a problem.

I suppose an alternative would be something like strong params where we basically specific which params we want, but that would kill the instance_options behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Somehow there was some issues with the type not being duped properly. Therefore the test failed cause the type method return the wrong value. I didn't dig to much into it though. I can try and spend some more time to figure out why it was duped properly.

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need to dup and freeze the options at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking not

On Wed, Mar 30, 2016 at 7:14 AM Yohan Robert [email protected]
wrote:

In lib/action_controller/serialization.rb
#1572 (comment)
:

@@ -56,10 +56,15 @@ def use_adapter?

 [:_render_option_json, :_render_with_renderer_json].each do |renderer_method|
   define_method renderer_method do |resource, options|

  •    options.fetch(:serialization_context) do

  •      options[:serialization_context] = ActiveModelSerializers::SerializationContext.new(request, options)

  •    options.freeze

  •    serialization_options = options.deep_dup

Do we actually need to dup and freeze the options at all?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/rails-api/active_model_serializers/pull/1572/files/094af453fe0eb9e8978f6aeec65ec87e22b59a28#r57877815

if options.key?(:serializer)
serialization_options[:serializer] = options[:serializer]
Copy link
Member Author

Choose a reason for hiding this comment

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

you said there was some weird bug caused by duping the serializer that this fixes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I still haven't figured it out but there were three bugs failling. The resulting JSON API types looked like "some_module_some_class" instead of the values specified via type "post". So I assume when the class was duped the _type wasn't duped properly.

Copy link
Member

Choose a reason for hiding this comment

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

You could probably remove these lines and run the tests to trigger it.

end
serializable_resource = get_serializer(resource, serializer_options)
unless serialization_options.key?(:serialization_context)
serialization_options[:serialization_context] = ActiveModelSerializers::SerializationContext.new(request, serialization_options)
end
Copy link
Member Author

Choose a reason for hiding this comment

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

using fetch would be the more idiomatic way to do this, and is also easier to read. @groyoh is there a reason you changed this?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't fetch main use case supposed to be "retrieving an object"? I changed it because for me using fetch when we don't use the return value feels weird. But I can revert this change if needed.

serializable_resource = get_serializer(resource, serialization_options)
super(serializable_resource, options)
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/active_model/serializable_resource.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
require 'active_model_serializers/adapter'
module ActiveModel
class SerializableResource
ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter, :meta, :meta_key, :links])
ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter, :meta, :meta_key, :links, :serialization_context])
Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose the problem here will be if a serializer can use url helper to make links without the serialization context.. but that's not anything anyone is working on now

include ActiveModelSerializers::Logging

delegate :serializable_hash, :as_json, :to_json, to: :adapter
Expand Down
2 changes: 1 addition & 1 deletion lib/active_model_serializers/adapter/json.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class Json < Base
def serializable_hash(options = nil)
options ||= {}
serialized_hash = { root => Attributes.new(serializer, instance_options).serializable_hash(options) }
transform_key_casing!(serialized_hash, options[:serialization_context])
transform_key_casing!(serialized_hash, instance_options[:serialization_context])
end
end
end
Expand Down
10 changes: 5 additions & 5 deletions lib/active_model_serializers/adapter/json_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# coding: utf-8
Copy link
Member Author

Choose a reason for hiding this comment

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

Was there a problem not having this?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like emacs automatically added it. Will fix it.

# {http://jsonapi.org/format/ JSON API specification}
# rubocop:disable Style/AsciiComments
# TODO: implement!
Expand Down Expand Up @@ -43,14 +44,13 @@ def default_key_transform

# {http://jsonapi.org/format/#crud Requests are transactional, i.e. success or failure}
# {http://jsonapi.org/format/#document-top-level data and errors MUST NOT coexist in the same document.}
def serializable_hash(options = nil)
options ||= {}
def serializable_hash(_options = nil)
Copy link
Member Author

Choose a reason for hiding this comment

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

So, we do need to take options into serializable_hash from the renderer. I'm willing to be convinced, but when someone passing in options from theActiveModel::Serializers::JSONAPI toas_json`, I would expect them to be honored, rather than silently ignored.

Copy link
Member

Choose a reason for hiding this comment

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

Which options would they pass to as_json that could be used by the JsonApi adapter?

Do you mean we should add instance_options.merge!(options) here?

document = if serializer.success?
success_document(options)
success_document
else
failure_document
end
transform_key_casing!(document, options[:serialization_context])
transform_key_casing!(document, instance_options[:serialization_context])
Copy link
Member Author

Choose a reason for hiding this comment

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

@groyoh So, there's no reason the adapter can't use instance_options for serialization context (or, really, the options to initialize would be the proper place to handle this), but putting the serialization_context in the adapter options isn't incompatible with https://github.com/rails-api/active_model_serializers/pull/1572/files#r56778382

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand your comment.

end

# {http://jsonapi.org/format/#document-top-level Primary data}
Expand All @@ -68,7 +68,7 @@ def serializable_hash(options = nil)
# links: toplevel_links,
# jsonapi: toplevel_jsonapi
# }.reject! {|_,v| v.nil? }
def success_document(options)
def success_document
is_collection = serializer.respond_to?(:each)
serializers = is_collection ? serializer : [serializer]
primary_data, included = resource_objects_for(serializers)
Expand Down
23 changes: 13 additions & 10 deletions test/adapter/json/key_case_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,19 @@ class PostSerializer < ActiveModel::Serializer
def setup
ActionController::Base.cache_store.clear
@blog = Blog.new(id: 1, name: 'My Blog!!', special_attribute: 'neat')
serializer = CustomBlogSerializer.new(@blog)
@adapter = ActiveModelSerializers::Adapter::Json.new(serializer)
end

def test_key_transform_default
mock_request
assert_equal({
blog: { id: 1, special_attribute: 'neat', articles: nil }
}, @adapter.serializable_hash(@options))
}, adapter.serializable_hash)
Copy link
Member

Choose a reason for hiding this comment

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

The serialization_context is no more passed via as_json options but via the adapter options.

end

def test_key_transform_global_config
mock_request
result = with_config(key_transform: :camel_lower) do
@adapter.serializable_hash(@options)
adapter.serializable_hash
end
assert_equal({
blog: { id: 1, specialAttribute: 'neat', articles: nil }
Expand All @@ -45,7 +43,7 @@ def test_key_transform_global_config
def test_key_transform_serialization_ctx_overrides_global_config
mock_request(:camel)
result = with_config(key_transform: :camel_lower) do
@adapter.serializable_hash(@options)
adapter.serializable_hash
end
assert_equal({
Blog: { Id: 1, SpecialAttribute: 'neat', Articles: nil }
Expand All @@ -56,36 +54,41 @@ def test_key_transform_undefined
mock_request(:blam)
result = nil
assert_raises NoMethodError do
result = @adapter.serializable_hash(@options)
result = adapter.serializable_hash
end
end

def test_key_transform_dashed
mock_request(:dashed)
assert_equal({
blog: { id: 1, :"special-attribute" => 'neat', articles: nil }
}, @adapter.serializable_hash(@options))
}, adapter.serializable_hash)
end

def test_key_transform_unaltered
mock_request(:unaltered)
assert_equal({
blog: { id: 1, special_attribute: 'neat', articles: nil }
}, @adapter.serializable_hash(@options))
}, adapter.serializable_hash)
end

def test_key_transform_camel
mock_request(:camel)
assert_equal({
Blog: { Id: 1, SpecialAttribute: 'neat', Articles: nil }
}, @adapter.serializable_hash(@options))
}, adapter.serializable_hash)
end

def test_key_transform_camel_lower
mock_request(:camel_lower)
assert_equal({
blog: { id: 1, specialAttribute: 'neat', articles: nil }
}, @adapter.serializable_hash(@options))
}, adapter.serializable_hash)
end

def adapter
Copy link
Member Author

Choose a reason for hiding this comment

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

this should just be serializable(@blog, serializer: CustomBlogSerializer, adapter: :json)

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, I just move the call without thinking too much about it. Here I move the this part of the code in a method, to make sure that @options was set before using it.

serializer = CustomBlogSerializer.new(@blog)
ActiveModelSerializers::Adapter::Json.new(serializer, @options || {})
end
end
end
Expand Down
Loading