Restrict serializable_hash to accepted options (#1647)

Restrict tests/impl from passing AMS options into serializable_hash

CHANGELOG.md

@@ -8,6 +8,8 @@ Breaking changes:
 - [#1574](https://github.com/rails-api/active_model_serializers/pull/1574) Default key case for the JsonApi adapter changed to dashed. (@remear)
 
 Features:
+- [#1647](https://github.com/rails-api/active_model_serializers/pull/1647) Restrict usage of `serializable_hash` options
+  to the ActiveModel::Serialization and ActiveModel::Serializers::JSON interface. (@bf4)
 - [#1645](https://github.com/rails-api/active_model_serializers/pull/1645) Transform keys referenced in values. (@remear)
 - [#1650](https://github.com/rails-api/active_model_serializers/pull/1650) Fix serialization scope options `scope`, `scope_name`
   take precedence over `serialization_scope` in the controller.

lib/active_model_serializers/adapter/json.rb

@@ -4,7 +4,7 @@ module ActiveModelSerializers
       def serializable_hash(options = nil)
         options ||= {}
         serialized_hash = { root => Attributes.new(serializer, instance_options).serializable_hash(options) }
-        self.class.transform_key_casing!(serialized_hash, options)
+        self.class.transform_key_casing!(serialized_hash, instance_options)
       end
     end
   end

lib/active_model_serializers/adapter/json_api.rb

@@ -43,14 +43,13 @@ module ActiveModelSerializers
 
       # {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)
+      def serializable_hash(*)
-        options ||= {}
         document = if serializer.success?
-                     success_document(options)
+                     success_document
                    else
-                     failure_document(options)
+                     failure_document
                    end
-        self.class.transform_key_casing!(document, options)
+        self.class.transform_key_casing!(document, instance_options)
       end
 
       # {http://jsonapi.org/format/#document-top-level Primary data}
@@ -68,10 +67,10 @@ module ActiveModelSerializers
       #    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, options)
+        primary_data, included = resource_objects_for(serializers)
 
         hash = {}
         # toplevel_data
@@ -128,7 +127,7 @@ module ActiveModelSerializers
 
         if is_collection && serializer.paginated?
           hash[:links] ||= {}
-          hash[:links].update(pagination_links_for(serializer, options))
+          hash[:links].update(pagination_links_for(serializer))
         end
 
         hash
@@ -148,7 +147,7 @@ module ActiveModelSerializers
       #   }.reject! {|_,v| v.nil? }
       # prs:
       #  https://github.com/rails-api/active_model_serializers/pull/1004
-      def failure_document(options)
+      def failure_document
         hash = {}
         # PR Please :)
         # Jsonapi.add!(hash)
@@ -163,10 +162,10 @@ module ActiveModelSerializers
         #   ]
         if serializer.respond_to?(:each)
           hash[:errors] = serializer.flat_map do |error_serializer|
-            Error.resource_errors(error_serializer, options)
+            Error.resource_errors(error_serializer, instance_options)
           end
         else
-          hash[:errors] = Error.resource_errors(serializer, options)
+          hash[:errors] = Error.resource_errors(serializer, instance_options)
         end
         hash
       end
@@ -224,21 +223,21 @@ module ActiveModelSerializers
       #     [x] url helpers https://github.com/rails-api/active_model_serializers/issues/1269
       #   meta
       #     [x] https://github.com/rails-api/active_model_serializers/pull/1340
-      def resource_objects_for(serializers, options)
+      def resource_objects_for(serializers)
         @primary = []
         @included = []
         @resource_identifiers = Set.new
-        serializers.each { |serializer| process_resource(serializer, true, options) }
+        serializers.each { |serializer| process_resource(serializer, true) }
-        serializers.each { |serializer| process_relationships(serializer, @include_tree, options) }
+        serializers.each { |serializer| process_relationships(serializer, @include_tree) }
 
         [@primary, @included]
       end
 
-      def process_resource(serializer, primary, options)
+      def process_resource(serializer, primary)
-        resource_identifier = ResourceIdentifier.new(serializer, options).as_json
+        resource_identifier = ResourceIdentifier.new(serializer, instance_options).as_json
         return false unless @resource_identifiers.add?(resource_identifier)
 
-        resource_object = resource_object_for(serializer, options)
+        resource_object = resource_object_for(serializer)
         if primary
           @primary << resource_object
         else
@@ -248,21 +247,21 @@ module ActiveModelSerializers
         true
       end
 
-      def process_relationships(serializer, include_tree, options)
+      def process_relationships(serializer, include_tree)
         serializer.associations(include_tree).each do |association|
-          process_relationship(association.serializer, include_tree[association.key], options)
+          process_relationship(association.serializer, include_tree[association.key])
         end
       end
 
-      def process_relationship(serializer, include_tree, options)
+      def process_relationship(serializer, include_tree)
         if serializer.respond_to?(:each)
-          serializer.each { |s| process_relationship(s, include_tree, options) }
+          serializer.each { |s| process_relationship(s, include_tree) }
           return
         end
         return unless serializer && serializer.object
-        return unless process_resource(serializer, false, options)
+        return unless process_resource(serializer, false)
 
-        process_relationships(serializer, include_tree, options)
+        process_relationships(serializer, include_tree)
       end
 
       # {http://jsonapi.org/format/#document-resource-object-attributes Document Resource Object Attributes}
@@ -286,9 +285,9 @@ module ActiveModelSerializers
       end
 
       # {http://jsonapi.org/format/#document-resource-objects Document Resource Objects}
-      def resource_object_for(serializer, options)
+      def resource_object_for(serializer)
         resource_object = cache_check(serializer) do
-          resource_object = ResourceIdentifier.new(serializer, options).as_json
+          resource_object = ResourceIdentifier.new(serializer, instance_options).as_json
 
           requested_fields = fieldset && fieldset.fields_for(resource_object[:type])
           attributes = attributes_for(serializer, requested_fields)
@@ -297,7 +296,7 @@ module ActiveModelSerializers
         end
 
         requested_associations = fieldset.fields_for(resource_object[:type]) || '*'
-        relationships = relationships_for(serializer, requested_associations, options)
+        relationships = relationships_for(serializer, requested_associations)
         resource_object[:relationships] = relationships if relationships.any?
 
         links = links_for(serializer)
@@ -425,13 +424,13 @@ module ActiveModelSerializers
       #     id: 'required-id',
       #     meta: meta
       #   }.reject! {|_,v| v.nil? }
-      def relationships_for(serializer, requested_associations, options)
+      def relationships_for(serializer, requested_associations)
         include_tree = ActiveModel::Serializer::IncludeTree.from_include_args(requested_associations)
         serializer.associations(include_tree).each_with_object({}) do |association, hash|
           hash[association.key] = Relationship.new(
             serializer,
             association.serializer,
-            options,
+            instance_options,
             options: association.options,
             links: association.links,
             meta: association.meta
@@ -499,8 +498,8 @@ module ActiveModelSerializers
       #   end
       # prs:
       #   https://github.com/rails-api/active_model_serializers/pull/1041
-      def pagination_links_for(serializer, options)
+      def pagination_links_for(serializer)
-        PaginationLinks.new(serializer.object, options[:serialization_context]).serializable_hash(options)
+        PaginationLinks.new(serializer.object, instance_options).as_json
       end
 
       # {http://jsonapi.org/format/#document-meta Docment Meta}

lib/active_model_serializers/adapter/json_api/pagination_links.rb

@@ -6,20 +6,25 @@ module ActiveModelSerializers
 
         attr_reader :collection, :context
 
-        def initialize(collection, context)
+        def initialize(collection, adapter_options)
           @collection = collection
-          @context = context
+          @adapter_options = adapter_options
+          @context = adapter_options.fetch(:serialization_context)
         end
 
-        def serializable_hash(options = {})
+        def as_json
           per_page = collection.try(:per_page) || collection.try(:limit_value) || collection.size
           pages_from.each_with_object({}) do |(key, value), hash|
             params = query_parameters.merge(page: { number: value, size: per_page }).to_query
 
-            hash[key] = "#{url(options)}?#{params}"
+            hash[key] = "#{url(adapter_options)}?#{params}"
           end
         end
 
+        protected
+
+        attr_reader :adapter_options
+
         private
 
         def pages_from

lib/active_model_serializers/adapter/null.rb

@@ -1,8 +1,7 @@
 module ActiveModelSerializers
   module Adapter
     class Null < Base
-      # Since options param is not being used, underscored naming of the param
+      def serializable_hash(*)
-      def serializable_hash(_options = nil)
         {}
       end
     end

lib/active_model_serializers/serializable_resource.rb

@@ -2,7 +2,7 @@ require 'set'
 
 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])
     include ActiveModelSerializers::Logging
 
     delegate :serializable_hash, :as_json, :to_json, to: :adapter

test/adapter/json/transform_test.rb

@@ -8,9 +8,11 @@ module ActiveModelSerializers
           context = Minitest::Mock.new
           context.expect(:request_url, URI)
           context.expect(:query_parameters, {})
-          @options = {}
+          options = {}
-          @options[:key_transform] = key_transform if key_transform
+          options[:key_transform] = key_transform if key_transform
-          @options[:serialization_context] = context
+          options[:serialization_context] = context
+          serializer = CustomBlogSerializer.new(@blog)
+          @adapter = ActiveModelSerializers::Adapter::Json.new(serializer, options)
         end
 
         Post = Class.new(::Model)
@@ -18,24 +20,22 @@ module ActiveModelSerializers
           attributes :id, :title, :body, :publish_at
         end
 
-        def setup
+        setup do
           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_transform_default
           mock_request
           assert_equal({
             blog: { id: 1, special_attribute: 'neat', articles: nil }
-          }, @adapter.serializable_hash(@options))
+          }, @adapter.serializable_hash)
         end
 
         def test_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 }
@@ -45,7 +45,7 @@ module ActiveModelSerializers
         def test_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 }
@@ -56,7 +56,7 @@ module ActiveModelSerializers
           mock_request(:blam)
           result = nil
           assert_raises NoMethodError do
-            result = @adapter.serializable_hash(@options)
+            result = @adapter.serializable_hash
           end
         end
 
@@ -64,28 +64,28 @@ module ActiveModelSerializers
           mock_request(:dash)
           assert_equal({
             blog: { id: 1, :"special-attribute" => 'neat', articles: nil }
-          }, @adapter.serializable_hash(@options))
+          }, @adapter.serializable_hash)
         end
 
         def test_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_transform_camel
           mock_request(:camel)
           assert_equal({
             Blog: { Id: 1, SpecialAttribute: 'neat', Articles: nil }
-          }, @adapter.serializable_hash(@options))
+          }, @adapter.serializable_hash)
         end
 
         def test_transform_camel_lower
           mock_request(:camel_lower)
           assert_equal({
             blog: { id: 1, specialAttribute: 'neat', articles: nil }
-          }, @adapter.serializable_hash(@options))
+          }, @adapter.serializable_hash)
         end
       end
     end

test/adapter/json_api/pagination_links_test.rb

@@ -25,13 +25,13 @@ module ActiveModelSerializers
           context = Minitest::Mock.new
           context.expect(:request_url, original_url)
           context.expect(:query_parameters, query_parameters)
-          @options = {}
+          context.expect(:key_transform, nil)
-          @options[:serialization_context] = context
         end
 
-        def load_adapter(paginated_collection, options = {})
+        def load_adapter(paginated_collection, mock_request = nil)
-          options = options.merge(adapter: :json_api)
+          render_options = { adapter: :json_api }
-          ActiveModelSerializers::SerializableResource.new(paginated_collection, options)
+          render_options[:serialization_context] = mock_request if mock_request
+          serializable(paginated_collection, render_options)
         end
 
         def using_kaminari(page = 2)
@@ -102,43 +102,38 @@ module ActiveModelSerializers
         end
 
         def test_pagination_links_using_kaminari
-          adapter = load_adapter(using_kaminari)
+          adapter = load_adapter(using_kaminari, mock_request)
 
-          mock_request
+          assert_equal expected_response_with_pagination_links, adapter.serializable_hash
-          assert_equal expected_response_with_pagination_links, adapter.serializable_hash(@options)
         end
 
         def test_pagination_links_using_will_paginate
-          adapter = load_adapter(using_will_paginate)
+          adapter = load_adapter(using_will_paginate, mock_request)
 
-          mock_request
+          assert_equal expected_response_with_pagination_links, adapter.serializable_hash
-          assert_equal expected_response_with_pagination_links, adapter.serializable_hash(@options)
         end
 
         def test_pagination_links_with_additional_params
-          adapter = load_adapter(using_will_paginate)
+          adapter = load_adapter(using_will_paginate, mock_request({ test: 'test' }))
 
-          mock_request({ test: 'test' })
           assert_equal expected_response_with_pagination_links_and_additional_params,
-            adapter.serializable_hash(@options)
+            adapter.serializable_hash
         end
 
         def test_last_page_pagination_links_using_kaminari
-          adapter = load_adapter(using_kaminari(3))
+          adapter = load_adapter(using_kaminari(3), mock_request)
 
-          mock_request
+          assert_equal expected_response_with_last_page_pagination_links, adapter.serializable_hash
-          assert_equal expected_response_with_last_page_pagination_links, adapter.serializable_hash(@options)
         end
 
         def test_last_page_pagination_links_using_will_paginate
-          adapter = load_adapter(using_will_paginate(3))
+          adapter = load_adapter(using_will_paginate(3), mock_request)
 
-          mock_request
+          assert_equal expected_response_with_last_page_pagination_links, adapter.serializable_hash
-          assert_equal expected_response_with_last_page_pagination_links, adapter.serializable_hash(@options)
         end
 
         def test_not_showing_pagination_links
-          adapter = load_adapter(@array)
+          adapter = load_adapter(@array, mock_request)
 
           assert_equal expected_response_without_pagination_links, adapter.serializable_hash
         end

test/adapter/json_api/transform_test.rb

@@ -67,8 +67,8 @@ module ActiveModelSerializers
         def test_success_document_transform_default
           mock_request
           serializer = PostSerializer.new(@post)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
           assert_equal({
             data: {
               id: '1337',
@@ -102,8 +102,8 @@ module ActiveModelSerializers
           mock_request
           result = with_config(key_transform: :camel_lower) do
             serializer = PostSerializer.new(@post)
-            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-            adapter.serializable_hash(@options)
+            adapter.serializable_hash
           end
           assert_equal({
             data: {
@@ -138,8 +138,8 @@ module ActiveModelSerializers
           mock_request(:camel)
           result = with_config(key_transform: :camel_lower) do
             serializer = PostSerializer.new(@post)
-            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-            adapter.serializable_hash(@options)
+            adapter.serializable_hash
           end
           assert_equal({
             Data: {
@@ -173,8 +173,8 @@ module ActiveModelSerializers
         def test_success_document_transform_dash
           mock_request(:dash)
           serializer = PostSerializer.new(@post)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
           assert_equal({
             data: {
               id: '1337',
@@ -207,8 +207,8 @@ module ActiveModelSerializers
         def test_success_document_transform_unaltered
           mock_request(:unaltered)
           serializer = PostSerializer.new(@post)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
           assert_equal({
             data: {
               id: '1337',
@@ -241,17 +241,18 @@ module ActiveModelSerializers
         def test_success_document_transform_undefined
           mock_request(:zoot)
           serializer = PostSerializer.new(@post)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          assert_raises NoMethodError do
+          exception = assert_raises NoMethodError do
-            adapter.serializable_hash(@options)
+            adapter.serializable_hash
           end
+          assert_match(/undefined method.*zoot/, exception.message)
         end
 
         def test_success_document_transform_camel
           mock_request(:camel)
           serializer = PostSerializer.new(@post)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
           assert_equal({
             Data: {
               Id: '1337',
@@ -284,8 +285,8 @@ module ActiveModelSerializers
         def test_success_document_transform_camel_lower
           mock_request(:camel_lower)
           serializer = PostSerializer.new(@post)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
           assert_equal({
             data: {
               id: '1337',
@@ -321,8 +322,8 @@ module ActiveModelSerializers
           resource.errors.add(:published_at, 'must be in the future')
           resource.errors.add(:title, 'must be longer')
           serializer = ActiveModel::Serializer::ErrorSerializer.new(resource)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
           expected_errors_object =
             { :errors =>
               [
@@ -345,8 +346,8 @@ module ActiveModelSerializers
             resource.errors.add(:published_at, 'must be in the future')
             resource.errors.add(:title, 'must be longer')
             serializer = ActiveModel::Serializer::ErrorSerializer.new(resource)
-            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-            adapter.serializable_hash(@options)
+            adapter.serializable_hash
           end
           expected_errors_object =
             { :Errors =>
@@ -371,8 +372,8 @@ module ActiveModelSerializers
             resource.errors.add(:published_at, 'must be in the future')
             resource.errors.add(:title, 'must be longer')
             serializer = ActiveModel::Serializer::ErrorSerializer.new(resource)
-            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+            adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-            adapter.serializable_hash(@options)
+            adapter.serializable_hash
           end
           expected_errors_object =
             { :Errors =>
@@ -398,8 +399,8 @@ module ActiveModelSerializers
           resource.errors.add(:title, 'must be longer')
 
           serializer = ActiveModel::Serializer::ErrorSerializer.new(resource)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
 
           expected_errors_object =
             { :errors =>
@@ -425,8 +426,8 @@ module ActiveModelSerializers
           resource.errors.add(:title, 'must be longer')
 
           serializer = ActiveModel::Serializer::ErrorSerializer.new(resource)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
 
           expected_errors_object =
             { :errors =>
@@ -446,11 +447,12 @@ module ActiveModelSerializers
           resource.errors.add(:title, 'must be longer')
 
           serializer = ActiveModel::Serializer::ErrorSerializer.new(resource)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
 
-          assert_raises NoMethodError do
+          exception = assert_raises NoMethodError do
-            adapter.serializable_hash(@options)
+            adapter.serializable_hash
           end
+          assert_match(/undefined method.*krazy/, exception.message)
         end
 
         def test_error_document_transform_camel
@@ -461,8 +463,8 @@ module ActiveModelSerializers
           resource.errors.add(:title, 'must be longer')
 
           serializer = ActiveModel::Serializer::ErrorSerializer.new(resource)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
 
           expected_errors_object =
             { :Errors =>
@@ -482,8 +484,8 @@ module ActiveModelSerializers
           resource.errors.add(:title, 'must be longer')
 
           serializer = ActiveModel::Serializer::ErrorSerializer.new(resource)
-          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
+          adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options)
-          result = adapter.serializable_hash(@options)
+          result = adapter.serializable_hash
 
           expected_errors_object =
             { :errors =>

test/support/isolated_unit.rb

@@ -41,7 +41,6 @@ require 'active_support/core_ext/string/access'
 
 # These files do not require any others and are needed
 # to run the tests
-require 'active_support/testing/autorun'
 require 'active_support/testing/isolation'
 
 module TestHelpers