From aad7779a3fd918bed5c17438c4626883d6fe6c5f Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Mon, 11 Apr 2016 13:10:18 -0500 Subject: [PATCH] Restrict serializable_hash to accepted options (#1647) Restrict tests/impl from passing AMS options into serializable_hash --- CHANGELOG.md | 2 + lib/active_model_serializers/adapter/json.rb | 2 +- .../adapter/json_api.rb | 59 ++++++++-------- .../adapter/json_api/pagination_links.rb | 13 ++-- lib/active_model_serializers/adapter/null.rb | 3 +- .../serializable_resource.rb | 2 +- test/adapter/json/transform_test.rb | 28 ++++---- .../adapter/json_api/pagination_links_test.rb | 37 +++++----- test/adapter/json_api/transform_test.rb | 70 ++++++++++--------- test/support/isolated_unit.rb | 1 - 10 files changed, 109 insertions(+), 108 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e8225c5f..34eb2298a 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/lib/active_model_serializers/adapter/json.rb b/lib/active_model_serializers/adapter/json.rb index 5d182a515..aa2d8bb0e 100644 --- a/lib/active_model_serializers/adapter/json.rb +++ b/lib/active_model_serializers/adapter/json.rb @@ -4,7 +4,7 @@ class Json < Base 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 diff --git a/lib/active_model_serializers/adapter/json_api.rb b/lib/active_model_serializers/adapter/json_api.rb index c324798ca..0216c7a26 100644 --- a/lib/active_model_serializers/adapter/json_api.rb +++ b/lib/active_model_serializers/adapter/json_api.rb @@ -43,14 +43,13 @@ def self.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(*) 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 @@ 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, options) + primary_data, included = resource_objects_for(serializers) hash = {} # toplevel_data @@ -128,7 +127,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 hash @@ -148,7 +147,7 @@ def success_document(options) # }.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 @@ def failure_document(options) # ] 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 @@ def fragment_cache(cached_hash, non_cached_hash) # [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_relationships(serializer, @include_tree, options) } + serializers.each { |serializer| process_resource(serializer, true) } + serializers.each { |serializer| process_relationships(serializer, @include_tree) } [@primary, @included] end - def process_resource(serializer, primary, options) - resource_identifier = ResourceIdentifier.new(serializer, options).as_json + def process_resource(serializer, primary) + 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 @@ def process_resource(serializer, primary, options) 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 @@ def attributes_for(serializer, fields) 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 @@ def resource_object_for(serializer, options) 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 @@ def resource_object_for(serializer, options) # 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 @@ def links_for(serializer) # end # prs: # https://github.com/rails-api/active_model_serializers/pull/1041 - def pagination_links_for(serializer, options) - PaginationLinks.new(serializer.object, options[:serialization_context]).serializable_hash(options) + def pagination_links_for(serializer) + PaginationLinks.new(serializer.object, instance_options).as_json end # {http://jsonapi.org/format/#document-meta Docment Meta} diff --git a/lib/active_model_serializers/adapter/json_api/pagination_links.rb b/lib/active_model_serializers/adapter/json_api/pagination_links.rb index cb3ed6ba0..58c6f1df1 100644 --- a/lib/active_model_serializers/adapter/json_api/pagination_links.rb +++ b/lib/active_model_serializers/adapter/json_api/pagination_links.rb @@ -6,20 +6,25 @@ class PaginationLinks 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 diff --git a/lib/active_model_serializers/adapter/null.rb b/lib/active_model_serializers/adapter/null.rb index 6e690b1b0..9e5faf5cb 100644 --- a/lib/active_model_serializers/adapter/null.rb +++ b/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(_options = nil) + def serializable_hash(*) {} end end diff --git a/lib/active_model_serializers/serializable_resource.rb b/lib/active_model_serializers/serializable_resource.rb index 6040c79d9..10f9d9aba 100644 --- a/lib/active_model_serializers/serializable_resource.rb +++ b/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]) include ActiveModelSerializers::Logging delegate :serializable_hash, :as_json, :to_json, to: :adapter diff --git a/test/adapter/json/transform_test.rb b/test/adapter/json/transform_test.rb index 30e058a50..4a18746de 100644 --- a/test/adapter/json/transform_test.rb +++ b/test/adapter/json/transform_test.rb @@ -8,9 +8,11 @@ def mock_request(key_transform = nil) context = Minitest::Mock.new context.expect(:request_url, URI) context.expect(:query_parameters, {}) - @options = {} - @options[:key_transform] = key_transform if key_transform - @options[:serialization_context] = context + options = {} + options[:key_transform] = key_transform if key_transform + options[:serialization_context] = context + serializer = CustomBlogSerializer.new(@blog) + @adapter = ActiveModelSerializers::Adapter::Json.new(serializer, options) end Post = Class.new(::Model) @@ -18,24 +20,22 @@ class PostSerializer < ActiveModel::Serializer 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 @@ def test_transform_global_config 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 @@ def test_transform_undefined mock_request(:blam) result = nil assert_raises NoMethodError do - result = @adapter.serializable_hash(@options) + result = @adapter.serializable_hash end end @@ -64,28 +64,28 @@ def test_transform_dash 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 diff --git a/test/adapter/json_api/pagination_links_test.rb b/test/adapter/json_api/pagination_links_test.rb index 244f8108c..406d0a6e4 100644 --- a/test/adapter/json_api/pagination_links_test.rb +++ b/test/adapter/json_api/pagination_links_test.rb @@ -25,13 +25,13 @@ def mock_request(query_parameters = {}, original_url = URI) context = Minitest::Mock.new context.expect(:request_url, original_url) context.expect(:query_parameters, query_parameters) - @options = {} - @options[:serialization_context] = context + context.expect(:key_transform, nil) end - def load_adapter(paginated_collection, options = {}) - options = options.merge(adapter: :json_api) - 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 + serializable(paginated_collection, render_options) end def using_kaminari(page = 2) @@ -102,43 +102,38 @@ def expected_response_with_last_page_pagination_links 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(@options) + assert_equal expected_response_with_pagination_links, adapter.serializable_hash 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(@options) + assert_equal expected_response_with_pagination_links, adapter.serializable_hash 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(@options) + assert_equal expected_response_with_last_page_pagination_links, adapter.serializable_hash 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(@options) + assert_equal expected_response_with_last_page_pagination_links, adapter.serializable_hash 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 diff --git a/test/adapter/json_api/transform_test.rb b/test/adapter/json_api/transform_test.rb index 856110870..26f4b0050 100644 --- a/test/adapter/json_api/transform_test.rb +++ b/test/adapter/json_api/transform_test.rb @@ -67,8 +67,8 @@ def setup def test_success_document_transform_default mock_request serializer = PostSerializer.new(@post) - adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash assert_equal({ data: { id: '1337', @@ -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) + adapter.serializable_hash end assert_equal({ data: { @@ -138,8 +138,8 @@ def test_success_doc_transform_serialization_ctx_overrides_global mock_request(:camel) 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) + adapter.serializable_hash end assert_equal({ Data: { @@ -173,8 +173,8 @@ def test_success_doc_transform_serialization_ctx_overrides_global def test_success_document_transform_dash mock_request(:dash) serializer = PostSerializer.new(@post) - adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash assert_equal({ data: { id: '1337', @@ -207,8 +207,8 @@ def test_success_document_transform_dash def test_success_document_transform_unaltered mock_request(:unaltered) serializer = PostSerializer.new(@post) - adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash assert_equal({ data: { id: '1337', @@ -241,17 +241,18 @@ def test_success_document_transform_unaltered def test_success_document_transform_undefined mock_request(:zoot) serializer = PostSerializer.new(@post) - adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) - assert_raises NoMethodError do - adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + exception = assert_raises NoMethodError do + 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) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash assert_equal({ Data: { Id: '1337', @@ -284,8 +285,8 @@ def test_success_document_transform_camel def test_success_document_transform_camel_lower mock_request(:camel_lower) serializer = PostSerializer.new(@post) - adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash assert_equal({ data: { id: '1337', @@ -321,8 +322,8 @@ def test_error_document_transform_default 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) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash expected_errors_object = { :errors => [ @@ -345,8 +346,8 @@ def test_error_document_transform_global_config 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.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + adapter.serializable_hash end expected_errors_object = { :Errors => @@ -371,8 +372,8 @@ def test_error_document_transform_serialization_ctx_overrides_global 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.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + adapter.serializable_hash end expected_errors_object = { :Errors => @@ -398,8 +399,8 @@ def test_error_document_transform_dash resource.errors.add(:title, 'must be longer') serializer = ActiveModel::Serializer::ErrorSerializer.new(resource) - adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash expected_errors_object = { :errors => @@ -425,8 +426,8 @@ def test_error_document_transform_unaltered resource.errors.add(:title, 'must be longer') serializer = ActiveModel::Serializer::ErrorSerializer.new(resource) - adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash expected_errors_object = { :errors => @@ -446,11 +447,12 @@ def test_error_document_transform_undefined 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 - adapter.serializable_hash(@options) + exception = assert_raises NoMethodError do + adapter.serializable_hash end + assert_match(/undefined method.*krazy/, exception.message) end def test_error_document_transform_camel @@ -461,8 +463,8 @@ def test_error_document_transform_camel resource.errors.add(:title, 'must be longer') serializer = ActiveModel::Serializer::ErrorSerializer.new(resource) - adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash expected_errors_object = { :Errors => @@ -482,8 +484,8 @@ def test_error_document_transform_camel_lower resource.errors.add(:title, 'must be longer') serializer = ActiveModel::Serializer::ErrorSerializer.new(resource) - adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer) - result = adapter.serializable_hash(@options) + adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer, @options) + result = adapter.serializable_hash expected_errors_object = { :errors => diff --git a/test/support/isolated_unit.rb b/test/support/isolated_unit.rb index d1d18eb69..4adc96a15 100644 --- a/test/support/isolated_unit.rb +++ b/test/support/isolated_unit.rb @@ -41,7 +41,6 @@ # 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