Restrict serializable_hash to accepted options #1647

bf4 · 2016-04-01T09:03:36Z

See #1643 for rationale behind these changes.

Some of these failures are addressed in other PRs such as
#1572

bf4 · 2016-04-01T10:51:39Z

@remear @groyoh I could use some help finishing this one, or fixing the failures outside of it. Basically, this takes any time we're calling serializable_hash and whitelists the valid keys. so, anywhere we passed invalid keys now fail

bf4 · 2016-04-01T10:52:26Z

lib/active_model_serializers/adapter/attributes.rb

@@ -8,7 +8,7 @@ def initialize(serializer, options = {})
      end

      def serializable_hash(options = nil)
-        options ||= {}
+        options = super


I figure this pattern with the base class is a little nicer and less error prone.

I actually find this pattern quite weird 😄 I mean, when I read super I would expect it to return a Hash.

yeah, I think it's weird and intuitive.

Super does return a hash. We need to make sure options is always set. This is how it is in Rails. See links in #1643. If someone passes in 'nil', it sets options to '{}'. Rather than make sure the signature is always 'options = nil' and the first line in the method 'options ||= {}', we just delegate that guard to the base class. Alternatively, we use a different abstract method that serializable_hash calls.

groyoh · 2016-04-01T18:37:15Z

@bf4 I'll have a look at it later today.

bf4 · 2016-04-01T19:32:08Z

lib/active_model_serializers/serializable_resource.rb

@@ -2,7 +2,7 @@

 module ActiveModelSerializers
  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])


@remear this is where I added serialization context to the adapter options.

Now pagination links call from jsonapi adapter is PaginationLinks.new(serializer.object, instance_options).as_json and PaginationLinks calls @context = adapter_options.fetch(:serialization_context)

Add :key_transform here and your failing test will pass 😉

bf4 · 2016-04-01T19:56:24Z

Looks good to me but could probably use a bit more docs

groyoh · 2016-04-02T15:57:15Z

test/adapter/json_api/pagination_links_test.rb

-          ActiveModelSerializers::SerializableResource.new(paginated_collection, options)
+        def load_adapter(paginated_collection, mock_request = nil)
+          render_options = { adapter: :json_api }
+          render_options[:serialization_context] = mock_request if mock_request


I like this! 👍

groyoh · 2016-04-02T16:14:34Z

What is your main objective when restricting the options of serializable_hash?

bf4 · 2016-04-03T00:26:23Z

@groyoh I think I can give you a better answer if you could clarify how you're reading #1643

It's just the args passed to to_json (which calls serializable_hash) in Rails

bf4 · 2016-04-03T00:33:26Z

lib/active_model_serializers/adapter/base.rb

-      def serializable_hash(_options = nil)
-        fail NotImplementedError, 'This is an abstract method. Should be implemented at the concrete adapter.'
+      def serializable_hash(options = nil)
+        (options ||= {}).slice(*ActiveModel::Serializer::SERIALIZABLE_HASH_VALID_KEYS) # rubocop:disable Lint/UselessAssignment


This is the centerpiece of the pr

groyoh · 2016-04-03T00:55:14Z

@bf4 yeah sorry, my question was not really clear. Since you wan to restrict the options passed to to_json, why not restrict it in the to_json/as_json method or directly at https://github.com/rails-api/active_model_serializers/blob/master/lib/action_controller/serialization.rb#L52?

bf4 · 2016-04-03T01:27:55Z

Good question, I agree it should probably be as_json but we have so many
examples and tests that call serializable_hash.... Which is the interface
in xml serializers and active modrl serializers as well.,, We could search
and replace :)
On Sat, Apr 2, 2016 at 7:55 PM Yohan Robert notifications@github.com
wrote:

@bf4 https://github.com/bf4 yeah sorry, my question was not really
clear. Since you wan to restrict the options passed to to_json, why not
restrict it in the to_json/as_json method or directly at
https://github.com/rails-api/active_model_serializers/blob/master/lib/action_controller/serialization.rb#L52
?

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#1647 (comment)

bf4 · 2016-04-03T19:49:03Z

@groyoh This is how AMS fits into rendering and why I'm thinking this way

render json: record, options
# |-> https://github.com/rails/rails/blob/v5.0.0.beta3/actionpack/lib/action_controller/metal/renderers.rb#L151-L152
  _render_with_renderer_json(record, options)
+  # |-> https://github.com/rails-api/active_model_serializers/blob/d30aa4c44fe004821be49d3427b56973f95c4984/lib/action_controller/serialization.rb#L46-L52
+    record = ActiveModel::Serializable_resource.new(record, options)
    # |-> https://github.com/rails/rails/blob/v5.0.0.beta3/actionpack/lib/action_controller/metal/renderers.rb#L159
      record.to_json(options) unless record.is_a?(String)
      # |-> https://github.com/rails/rails/blob/v5.0.0.beta3/activemodel/lib/active_model/serializers/json.rb#L88-L92
        record.as_json(options)
        # |-> https://github.com/rails/rails/blob/v5.0.0.beta3/activemodel/lib/active_model/serialization.rb#L124-L137
          record.serializable_hash(options)

where serializalbe_hash is where the options only, except, methods, include are defined, and as_json defines the option root and uses include_root_in_json.

NullVoxPopuli · 2016-04-03T22:33:52Z

lib/active_model_serializers/adapter/base.rb

@@ -41,6 +43,10 @@ def cache_check(serializer)

      private

+      def serialization_options(options)
+        (options ||= {}).slice(*ActiveModel::Serializer::SERIALIZABLE_HASH_VALID_KEYS) # rubocop:disable Lint/UselessAssignment


why do you have this assignment here?

why not just:

(options || {}).slice(*ActiveModel::Serializer::SERIALIZABLE_HASH_VALID_KEYS)

I guess in the current implementation it doesn't matter. Good eyes. previously, when it was inline, we, of course wanted to set the value of options when nil.

NullVoxPopuli · 2016-04-03T22:50:31Z

@bf4, I'm good with this. After squashing, I'd be happy to merge.

(Or we can try out the new github feature where it forces squashing) :-)

bf4 · 2016-04-11T02:33:28Z

test/adapter/json_api/transform_test.rb

@@ -102,8 +102,8 @@ def test_success_document_transform_global_config
          mock_request
          result = with_config(key_transform: :camel_lower) do
            serializer = PostSerializer.new(@post)
-            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
-            adapter.serializable_hash(@options)
+            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)


This test is failing when run as part of the suite, but not alone.. minitest-bisect anyone?

So, I added gem 'minitest-bisect' to my Gemfile.local and ran

RUBYOPT=-Ilib:test MTB_VERBOSE=2 be minitest_bisect --seed 48816 $(find test -type f -name \*_test.rb)

and minitest-bisect tells me

Final reproduction: Run options: --seed 48816 -n "/^(?:ActiveModel::Serializer::OptionsTest#(?:test_no_option_is_passed_in)|ActionController::Serialization::JsonApi::KeyTransformTest#(?:test_render_resource_with_transform))$/" # Running: .F Finished in 0.039316s, 50.8697 runs/s, 50.8697 assertions/s. 1) Failure: ActionController::Serialization::JsonApi::KeyTransformTest#test_render_resource_with_transform [/Users/benjamin/files/dev/repos/active_model_serializers/test/action_controller/json_api/transform_test.rb:105]: --- expected +++ actual @@ -1 +1 @@ -{"Data"=>{"Id"=>"1337", "Type"=>"Posts", "Attributes"=>{"Title"=>"Title 1", "Body"=>"Body 1", "PublishAt"=>"2020-03-16T03:55:25.291Z"}, "Relationships"=>{"Author"=>{"Data"=>{"Id"=>"1", "Type"=>"Authors"}}, "TopComments"=>{"Data"=>[{"Id"=>"7", "Type"=>"TopComments"}, {"Id"=>"12", "Type"=>"TopComments"}]}}, "Links"=>{"PostAuthors"=>"https://example.com/post_authors"}, "Meta"=>{"Rating"=>5, "FavoriteCount"=>10}}} +{"data"=>{"id"=>"1337", "type"=>"posts", "attributes"=>{"title"=>"Title 1", "body"=>"Body 1", "publish-at"=>"2020-03-16T03:55:25.291Z"}, "relationships"=>{"author"=>{"data"=>{"id"=>"1", "type"=>"authors"}}, "top-comments"=>{"data"=>[{"id"=>"7", "type"=>"top-comments"}, {"id"=>"12", "type"=>"top-comments"}]}}, "links"=>{"post-authors"=>"https://example.com/post_authors"}, "meta"=>{"rating"=>5, "favorite-count"=>10}}}

but that's wrong

I commented that out and it still failed. It appears to go away when I rm-rf test/action_controller

Apparently it's in test/action_controller/json_api/transform_test.rb

and, omg, I can't read, that's where it's been all along. no test pollution, just test/adapter/json_api/transform_test.rb looks a lot like test/action_controller/json_api/transform_test.rb

:-( I believe in you!! :-)

groyoh · 2016-04-11T07:46:06Z

@bf4 I pushed two commits that should fix the failing test plus two rubocop complaints 😉

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

lib/active_model_serializers/serializable_resource.rb

@@ -2,7 +2,7 @@

 module ActiveModelSerializers
  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, :key_transform])


@groyoh thanks for catching :key_transform, d'oh

bf4 reviewed Apr 1, 2016
View reviewed changes

bf4 added C: Feature V: 0.10.x S: Changelog Missing labels Apr 1, 2016

bf4 force-pushed the serializable_hash_interface branch from 691a596 to ec824c9 Compare April 1, 2016 19:16

bf4 reviewed Apr 1, 2016
View reviewed changes

bf4 force-pushed the serializable_hash_interface branch 2 times, most recently from aa602d5 to c3dfee7 Compare April 1, 2016 19:54

bf4 changed the title ~~[WIP] Restrict serializable_hash to accepted options~~ Restrict serializable_hash to accepted options Apr 1, 2016

bf4 force-pushed the serializable_hash_interface branch from c3dfee7 to 001ecb9 Compare April 1, 2016 22:28

groyoh reviewed Apr 2, 2016
View reviewed changes

bf4 reviewed Apr 3, 2016
View reviewed changes

bf4 force-pushed the serializable_hash_interface branch from 001ecb9 to 826c927 Compare April 3, 2016 20:05

bf4 removed the S: Changelog Missing label Apr 3, 2016

NullVoxPopuli reviewed Apr 3, 2016
View reviewed changes

Restrict tests/impl from passing AMS options into serializable_hash

4d89ec8

bf4 force-pushed the serializable_hash_interface branch from 2e66676 to 4d89ec8 Compare April 11, 2016 02:29

Test failing due to test pollution somewhere

25b0fd5

bf4 reviewed Apr 11, 2016
View reviewed changes

bf4 and others added 5 commits April 10, 2016 21:37

wip removing extra options

ecc2105

Better exception assertion

1e9dce8

Less warnings

717b0bb

Fix failing key_transform test

072190f

Fix rubocop complaints

0d6dfb7

bf4 reviewed Apr 11, 2016
View reviewed changes

bf4 merged commit aad7779 into rails-api:master Apr 11, 2016

bf4 deleted the serializable_hash_interface branch April 11, 2016 18:10

This was referenced Apr 11, 2016

Never mutate controller options #1572

Closed

Restrict serializable_hash to accepted options #1678

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict serializable_hash to accepted options #1647

Restrict serializable_hash to accepted options #1647

bf4 commented Apr 1, 2016

bf4 commented Apr 1, 2016

bf4 Apr 1, 2016

groyoh Apr 2, 2016

NullVoxPopuli Apr 2, 2016

bf4 Apr 3, 2016

groyoh commented Apr 1, 2016

bf4 Apr 1, 2016

groyoh Apr 11, 2016

bf4 commented Apr 1, 2016

groyoh Apr 2, 2016

groyoh commented Apr 2, 2016

bf4 commented Apr 3, 2016

bf4 Apr 3, 2016

groyoh commented Apr 3, 2016

bf4 commented Apr 3, 2016

bf4 commented Apr 3, 2016

NullVoxPopuli Apr 3, 2016

bf4 Apr 3, 2016

NullVoxPopuli commented Apr 3, 2016

bf4 Apr 11, 2016

bf4 Apr 11, 2016

bf4 Apr 11, 2016

bf4 Apr 11, 2016

bf4 Apr 11, 2016

NullVoxPopuli Apr 11, 2016

groyoh commented Apr 11, 2016

bf4 Apr 11, 2016

Restrict serializable_hash to accepted options #1647

Restrict serializable_hash to accepted options #1647

Conversation

bf4 commented Apr 1, 2016

bf4 commented Apr 1, 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

groyoh commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Apr 1, 2016

Choose a reason for hiding this comment

groyoh commented Apr 2, 2016

bf4 commented Apr 3, 2016

Choose a reason for hiding this comment

groyoh commented Apr 3, 2016

bf4 commented Apr 3, 2016

bf4 commented Apr 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Apr 3, 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

Choose a reason for hiding this comment

groyoh commented Apr 11, 2016

Choose a reason for hiding this comment