Never mutate controller options #1572

bf4 · 2016-03-09T18:23:05Z

Purpose

Pagination links incorrectly passing options to serializable_hash.

Only the renderer should be passing options to serializable_hash.

It's only working now because we are mutating the controller options which it
later passes to to_json which calls serializable_hash on the adapter.

This PR ensures that we do not mutate controller options, which
exposes failures only in the pagination links.

Changes

Make a (deep) dup of rendering options to pass to SerializableResource
Freezes rendering options (might be undesirable)
Caveats

freezing rendering options might be undesirable in general, and probably is
not necessary for this fix.
Related GitHub issues

AMS 0.10.0.rc4 / master

https://github.com/rails-api/active_model_serializers/blob/6b4c8df6fb6bc142ee6a74da51bb26c42a025b3c/lib/active_model_serializers/adapter/json_api.rb

def pagination_links_for(serializer, options)
  PaginationLinks.new(serializer.object, options[:serialization_context]).serializable_hash(options)
end

Those options come from the call to adapter.serializable_hash(options)
which is wrong. They should be passed into the Adapter initializer.

NullVoxPopuli · 2016-03-09T18:24:30Z

lib/action_controller/serialization.rb

@@ -26,13 +26,13 @@ def serialization_scope
        respond_to?(_serialization_scope, true)
    end

-    def get_serializer(resource, options = {})
+    def get_serializer(resource, serializer_options = {})


thankyou for specifying which options these are. omg

:-)

NullVoxPopuli · 2016-03-09T18:26:28Z

👍

groyoh · 2016-03-19T05:20:35Z

Rebased and fixed the tests.

beauby · 2016-03-20T04:26:14Z

👍

groyoh · 2016-03-20T09:12:12Z

lib/action_controller/serialization.rb

-        options.fetch(:serialization_context) do
-          options[:serialization_context] = ActiveModelSerializers::SerializationContext.new(request, options)
+        options.freeze
+        serialization_options = options.deep_dup


@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?

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.

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.

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

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

bf4 · 2016-03-21T02:32:55Z

@groyoh Thanks for helping clean this up. Great work!

In the future, would you mind doing me a favor and if there's substantial changes, as there are here, to add a commit, rather than amend the existing one? It make it easier to see what's changed. Thanks!

I left a lot of comments here. I've been super swamped lately, and beggars can't be choosers, so I was wondering how you feel about the direction the comments go, code-wise, and if that's something you'd rather talk to me about, wait for me to come back to, or attend to yourself.

Thanks, again! 🌈 ✨

groyoh · 2016-03-21T09:23:30Z

test/adapter/json_api/key_case_test.rb

@@ -62,14 +52,14 @@ def setup
                           publish_at: @publish_at)
          @comment1.post = @post
          @comment2.post = @post
+          @error_resource = ModelWithErrors.new


Moved here because it was used by many tests.

groyoh · 2016-03-21T09:47:58Z

@bf4 I actually created multiple commit locally but then squashed them up. I admit that it would have been better to leave it as it was for understanding the changes. I'll try to split them up in multiple commits if possible. I also commented on most of the changes 😉

- The controller options are now frozen - The serialization_context is now passed to the adapter via the adapter options and no more via as_json

bf4 · 2016-03-27T16:53:25Z

@groyoh I restored my original commits, 8fe6354543d7ace65c3c7698fac2a9f38f344374 6bd777e29a4c140212e3efc77dd52ee680a80456, and cherry-picked 094af45 for easier review, and rebased on master

groyoh · 2016-03-27T17:05:26Z

Sorry about this 😞

bf4 · 2016-03-27T17:15:37Z

lib/action_controller/serialization.rb

-          serializer_options[:serialization_context] = ActiveModelSerializers::SerializationContext.new(request, serializer_options)
+        serialization_options = options.deep_dup
+        if options.key?(:serializer)
+          serialization_options[:serializer] = options[:serializer]


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

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.

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

bf4 · 2016-04-01T18:48:01Z

lib/active_model_serializers/adapter/json_api.rb

@@ -128,7 +128,7 @@ def success_document(options)

        if is_collection && serializer.paginated?
          hash[:links] ||= {}
-          hash[:links].update(pagination_links_for(serializer, options))
+          hash[:links].update(pagination_links_for(serializer))
        end


apparently in the rebase i lost that this was @groyoh 's commit

bf4 · 2016-04-11T18:14:02Z

Closed by #1647

NullVoxPopuli reviewed Mar 9, 2016
View reviewed changes

bf4 mentioned this pull request Mar 10, 2016

Provide key transformation #1574

Merged

groyoh force-pushed the freeze_controller_options branch from 0bd1eda to f436ab9 Compare March 19, 2016 05:19

groyoh force-pushed the freeze_controller_options branch 2 times, most recently from aad9142 to 094af45 Compare March 19, 2016 05:39

groyoh reviewed Mar 20, 2016
View reviewed changes

bf4 added C: Feature V: 0.10.x C: Cleanup labels Mar 21, 2016

groyoh reviewed Mar 21, 2016
View reviewed changes

groyoh mentioned this pull request Mar 22, 2016

enable key_transform option outside of controller #1613

Closed

bf4 force-pushed the freeze_controller_options branch from 094af45 to 5103fe5 Compare March 27, 2016 16:52

Never mutate controller options

80b09b1

bf4 added 2 commits March 27, 2016 11:52

trying to fix

4b1ef59

Prevent controller options from being mutated

5103fe5

- The controller options are now frozen - The serialization_context is now passed to the adapter via the adapter options and no more via as_json

bf4 reviewed Mar 27, 2016
View reviewed changes

This was referenced Mar 31, 2016

Document and implement options to serializable_hash #1643

Open

Restrict serializable_hash to accepted options #1647

Merged

bf4 reviewed Apr 1, 2016
View reviewed changes

bf4 closed this Apr 11, 2016

bf4 deleted the freeze_controller_options branch April 11, 2016 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Never mutate controller options #1572

Never mutate controller options #1572

bf4 commented Mar 9, 2016

NullVoxPopuli Mar 9, 2016

NullVoxPopuli commented Mar 9, 2016

groyoh commented Mar 19, 2016

beauby commented Mar 20, 2016

groyoh Mar 20, 2016

bf4 Mar 21, 2016

groyoh Mar 21, 2016

groyoh Mar 30, 2016

bf4 Mar 30, 2016

bf4 commented Mar 21, 2016

groyoh Mar 21, 2016

groyoh commented Mar 21, 2016

bf4 commented Mar 27, 2016

groyoh commented Mar 27, 2016

bf4 Mar 27, 2016

groyoh Mar 27, 2016

groyoh Mar 27, 2016

bf4 Apr 1, 2016

bf4 commented Apr 11, 2016

Never mutate controller options #1572

Never mutate controller options #1572

Conversation

bf4 commented Mar 9, 2016

Purpose

Changes

Caveats

Related GitHub issues

Choose a reason for hiding this comment

NullVoxPopuli commented Mar 9, 2016

groyoh commented Mar 19, 2016

beauby commented Mar 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Mar 21, 2016

Choose a reason for hiding this comment

groyoh commented Mar 21, 2016

bf4 commented Mar 27, 2016

groyoh commented Mar 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Apr 11, 2016