From af99c0d9e65a0264eea63d2c363327576247a39e Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Fri, 28 Aug 2015 16:41:21 -0500 Subject: [PATCH] Ensure inheritance hooks run I was seeing transient failures where adapters may not be registered. e.g. https://travis-ci.org/rails-api/active_model_serializers/builds/77735382 Since we're using the Adapter, JsonApi, and Json classes as namespaces, some of the conventions we use for modules don't apply. Basically, we don't want to define the class anywhere besides itself. Otherwise, the inherited hooks may not run, and some adapters may not be registered. For example: If we have a class Api `class Api; end` And Api is also used as a namespace for `Api::Product` And the classes are defined in different files. In one file: ```ruby class Api autoload :Product def self.inherited(subclass) puts p [:inherited, subclass.name] puts end end ``` And in another: ```ruby class Api class Product < Api def sell_sell_sell! # TODO: sell end end end ``` If we load the Api class file first, the inherited hook will be defined on the class so that when we load the Api::Product class, we'll see the output: ```plain [ :inherited, Api::Product] ``` However, if we load the Api::Product class first, since it defines the `Api` class and then inherited from it, the Api file was never loaded, the hook never defined, and thus never run. By defining the class as `class Api::Product < Api` We ensure the the Api class MUST be defined, and thus, the hook will be defined and run and so sunshine and unicorns. Appendix: The below would work, but triggers a circular reference warning. It's also not recommended to mix require with autoload. ```ruby require 'api' class Api class Product < Api def sell_sell_sell! # TODO: sell end end end ``` This failure scenario was introduced by removing the circular reference warnings in https://github.com/rails-api/active_model_serializers/pull/1067 Style note: To make diffs on the adapters smalleer and easier to read, I've maintained the same identention that was in the original file. I've decided to prefer ease of reading the diff over style, esp. since we may later return to the preferred class declaration style. with '#' will be ignored, and an empty message aborts the commit. --- .rubocop.yml | 36 +++++++++++++++++-- lib/active_model/serializable_resource.rb | 2 +- lib/active_model/serializer/adapter.rb | 18 +++++----- .../serializer/adapter/flatten_json.rb | 8 +---- .../serializer/adapter/fragment_cache.rb | 8 +---- lib/active_model/serializer/adapter/json.rb | 13 +++---- .../serializer/adapter/json/fragment_cache.rb | 11 +----- .../serializer/adapter/json_api.rb | 25 ++++++------- .../adapter/json_api/fragment_cache.rb | 11 +----- .../adapter/json_api/pagination_links.rb | 10 +----- lib/active_model/serializer/adapter/null.rb | 8 +---- lib/active_model/serializer/fieldset.rb | 2 +- .../serialization_scope_name_test.rb | 2 +- test/serializers/adapter_for_test.rb | 5 +++ test/test_helper.rb | 9 ----- 15 files changed, 73 insertions(+), 95 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index d808271c..43ccb9f5 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -8,6 +8,39 @@ AllCops: DisplayCopNames: true DisplayStyleGuide: true +Style/IndentationConsistency: + Exclude: + - lib/active_model/serializer/adapter/flatten_json.rb + - lib/active_model/serializer/adapter/fragment_cache.rb + - lib/active_model/serializer/adapter/json.rb + - lib/active_model/serializer/adapter/json/fragment_cache.rb + - lib/active_model/serializer/adapter/json_api.rb + - lib/active_model/serializer/adapter/json_api/fragment_cache.rb + - lib/active_model/serializer/adapter/json_api/pagination_links.rb + - lib/active_model/serializer/adapter/null.rb + +Style/IndentationWidth: + Exclude: + - lib/active_model/serializer/adapter/flatten_json.rb + - lib/active_model/serializer/adapter/fragment_cache.rb + - lib/active_model/serializer/adapter/json.rb + - lib/active_model/serializer/adapter/json/fragment_cache.rb + - lib/active_model/serializer/adapter/json_api.rb + - lib/active_model/serializer/adapter/json_api/fragment_cache.rb + - lib/active_model/serializer/adapter/json_api/pagination_links.rb + - lib/active_model/serializer/adapter/null.rb + +Style/AccessModifierIndentation: + Exclude: + - lib/active_model/serializer/adapter/flatten_json.rb + - lib/active_model/serializer/adapter/fragment_cache.rb + - lib/active_model/serializer/adapter/json.rb + - lib/active_model/serializer/adapter/json/fragment_cache.rb + - lib/active_model/serializer/adapter/json_api.rb + - lib/active_model/serializer/adapter/json_api/fragment_cache.rb + - lib/active_model/serializer/adapter/json_api/pagination_links.rb + - lib/active_model/serializer/adapter/null.rb + Lint/NestedMethodDefinition: Enabled: false Exclude: @@ -16,9 +49,6 @@ Lint/NestedMethodDefinition: Style/StringLiterals: EnforcedStyle: single_quotes -Style/SpecialGlobalVars: - Enabled: false - Metrics/AbcSize: Max: 35 # TODO: Lower to 15 diff --git a/lib/active_model/serializable_resource.rb b/lib/active_model/serializable_resource.rb index feae1e8e..d3565f9a 100644 --- a/lib/active_model/serializable_resource.rb +++ b/lib/active_model/serializable_resource.rb @@ -76,7 +76,7 @@ module ActiveModel private ActiveModelSerializers.silence_warnings do - attr_reader :resource, :adapter_opts, :serializer_opts + attr_reader :resource, :adapter_opts, :serializer_opts end end end diff --git a/lib/active_model/serializer/adapter.rb b/lib/active_model/serializer/adapter.rb index 2b9fb350..3797c39e 100644 --- a/lib/active_model/serializer/adapter.rb +++ b/lib/active_model/serializer/adapter.rb @@ -5,11 +5,11 @@ module ActiveModel ADAPTER_MAP = {} private_constant :ADAPTER_MAP if defined?(private_constant) extend ActiveSupport::Autoload - require 'active_model/serializer/adapter/json' - require 'active_model/serializer/adapter/json_api' - autoload :FlattenJson - autoload :Null autoload :FragmentCache + autoload :Json + autoload :JsonApi + autoload :Null + autoload :FlattenJson def self.create(resource, options = {}) override = options.delete(:adapter) @@ -60,14 +60,14 @@ module ActiveModel register(adapter_name, adapter_class) adapter_class } - rescue ArgumentError + rescue ArgumentError => e failure_message = "Unknown adapter: #{adapter.inspect}. Valid adapters are: #{adapters}" - raise UnknownAdapterError, failure_message, $!.backtrace - rescue NameError + raise UnknownAdapterError, failure_message, e.backtrace + rescue NameError => e failure_message = - "NameError: #{$!.message}. Unknown adapter: #{adapter.inspect}. Valid adapters are: #{adapters}" - raise UnknownAdapterError, failure_message, $!.backtrace + "NameError: #{e.message}. Unknown adapter: #{adapter.inspect}. Valid adapters are: #{adapters}" + raise UnknownAdapterError, failure_message, e.backtrace end # @api private diff --git a/lib/active_model/serializer/adapter/flatten_json.rb b/lib/active_model/serializer/adapter/flatten_json.rb index 7ed57034..0c10f3e6 100644 --- a/lib/active_model/serializer/adapter/flatten_json.rb +++ b/lib/active_model/serializer/adapter/flatten_json.rb @@ -1,7 +1,4 @@ -module ActiveModel - class Serializer - class Adapter - class FlattenJson < Json +class ActiveModel::Serializer::Adapter::FlattenJson < ActiveModel::Serializer::Adapter::Json def serializable_hash(options = {}) super @result @@ -13,7 +10,4 @@ module ActiveModel def include_meta(json) json end - end - end - end end diff --git a/lib/active_model/serializer/adapter/fragment_cache.rb b/lib/active_model/serializer/adapter/fragment_cache.rb index 8463b5a2..13558c08 100644 --- a/lib/active_model/serializer/adapter/fragment_cache.rb +++ b/lib/active_model/serializer/adapter/fragment_cache.rb @@ -1,7 +1,4 @@ -module ActiveModel - class Serializer - class Adapter - class FragmentCache +class ActiveModel::Serializer::Adapter::FragmentCache attr_reader :serializer def initialize(adapter, serializer, options) @@ -75,7 +72,4 @@ module ActiveModel def to_valid_const_name(name) name.gsub('::', '_') end - end - end - end end diff --git a/lib/active_model/serializer/adapter/json.rb b/lib/active_model/serializer/adapter/json.rb index b3fa6e9c..2c0b6455 100644 --- a/lib/active_model/serializer/adapter/json.rb +++ b/lib/active_model/serializer/adapter/json.rb @@ -1,9 +1,7 @@ -require 'active_model/serializer/adapter/json/fragment_cache' +class ActiveModel::Serializer::Adapter::Json < ActiveModel::Serializer::Adapter + extend ActiveSupport::Autoload + autoload :FragmentCache -module ActiveModel - class Serializer - class Adapter - class Json < Adapter def serializable_hash(options = nil) options ||= {} if serializer.respond_to?(:each) @@ -44,9 +42,6 @@ module ActiveModel end def fragment_cache(cached_hash, non_cached_hash) - Json::FragmentCache.new().fragment_cache(cached_hash, non_cached_hash) + ActiveModel::Serializer::Adapter::Json::FragmentCache.new().fragment_cache(cached_hash, non_cached_hash) end - end - end - end end diff --git a/lib/active_model/serializer/adapter/json/fragment_cache.rb b/lib/active_model/serializer/adapter/json/fragment_cache.rb index 846a216f..5e687241 100644 --- a/lib/active_model/serializer/adapter/json/fragment_cache.rb +++ b/lib/active_model/serializer/adapter/json/fragment_cache.rb @@ -1,14 +1,5 @@ -require 'active_model/serializer/adapter/fragment_cache' -module ActiveModel - class Serializer - class Adapter - class Json < Adapter - class FragmentCache +class ActiveModel::Serializer::Adapter::Json::FragmentCache def fragment_cache(cached_hash, non_cached_hash) non_cached_hash.merge cached_hash end - end - end - end - end end diff --git a/lib/active_model/serializer/adapter/json_api.rb b/lib/active_model/serializer/adapter/json_api.rb index ac3bf4a1..f273fc65 100644 --- a/lib/active_model/serializer/adapter/json_api.rb +++ b/lib/active_model/serializer/adapter/json_api.rb @@ -1,10 +1,8 @@ -require 'active_model/serializer/adapter/json_api/fragment_cache' -require 'active_model/serializer/adapter/json_api/pagination_links' +class ActiveModel::Serializer::Adapter::JsonApi < ActiveModel::Serializer::Adapter + extend ActiveSupport::Autoload + autoload :PaginationLinks + autoload :FragmentCache -module ActiveModel - class Serializer - class Adapter - class JsonApi < Adapter def initialize(serializer, options = {}) super @hash = { data: [] } @@ -49,7 +47,7 @@ module ActiveModel def fragment_cache(cached_hash, non_cached_hash) root = false if @options.include?(:include) - JsonApi::FragmentCache.new.fragment_cache(root, cached_hash, non_cached_hash) + ActiveModel::Serializer::Adapter::JsonApi::FragmentCache.new().fragment_cache(root, cached_hash, non_cached_hash) end private @@ -62,6 +60,12 @@ module ActiveModel end end + def add_relationships(resource, name, serializers) + resource[:relationships] ||= {} + resource[:relationships][name] ||= { data: [] } + resource[:relationships][name][:data] += serializers.map { |serializer| { type: serializer.json_api_type, id: serializer.id.to_s } } + end + def resource_identifier_id_for(serializer) if serializer.respond_to?(:id) serializer.id @@ -161,8 +165,8 @@ module ActiveModel @hash[:links] = add_pagination_links(links, collection, options) if paginated?(collection) end - def add_pagination_links(links, collection, options) - pagination_links = JsonApi::PaginationLinks.new(collection, options[:context]).serializable_hash(options) + def add_pagination_links(links, resources, options) + pagination_links = ActiveModel::Serializer::Adapter::JsonApi::PaginationLinks.new(resources, options[:context]).serializable_hash(options) links.update(pagination_links) end @@ -171,7 +175,4 @@ module ActiveModel collection.respond_to?(:total_pages) && collection.respond_to?(:size) end - end - end - end end diff --git a/lib/active_model/serializer/adapter/json_api/fragment_cache.rb b/lib/active_model/serializer/adapter/json_api/fragment_cache.rb index 070371ac..ab348130 100644 --- a/lib/active_model/serializer/adapter/json_api/fragment_cache.rb +++ b/lib/active_model/serializer/adapter/json_api/fragment_cache.rb @@ -1,9 +1,4 @@ -require 'active_model/serializer/adapter/fragment_cache' -module ActiveModel - class Serializer - class Adapter - class JsonApi < Adapter - class FragmentCache +class ActiveModel::Serializer::Adapter::JsonApi::FragmentCache def fragment_cache(root, cached_hash, non_cached_hash) hash = {} core_cached = cached_hash.first @@ -15,8 +10,4 @@ module ActiveModel hash.deep_merge no_root_non_cache.deep_merge no_root_cache end - end - end - end - end end diff --git a/lib/active_model/serializer/adapter/json_api/pagination_links.rb b/lib/active_model/serializer/adapter/json_api/pagination_links.rb index 55e3280b..8661f3b1 100644 --- a/lib/active_model/serializer/adapter/json_api/pagination_links.rb +++ b/lib/active_model/serializer/adapter/json_api/pagination_links.rb @@ -1,8 +1,4 @@ -module ActiveModel - class Serializer - class Adapter - class JsonApi < Adapter - class PaginationLinks +class ActiveModel::Serializer::Adapter::JsonApi::PaginationLinks FIRST_PAGE = 1 attr_reader :collection, :context @@ -51,8 +47,4 @@ module ActiveModel def query_parameters @query_parameters ||= context.query_parameters end - end - end - end - end end diff --git a/lib/active_model/serializer/adapter/null.rb b/lib/active_model/serializer/adapter/null.rb index 1728f88e..78f9b8e3 100644 --- a/lib/active_model/serializer/adapter/null.rb +++ b/lib/active_model/serializer/adapter/null.rb @@ -1,11 +1,5 @@ -module ActiveModel - class Serializer - class Adapter - class Null < Adapter +class ActiveModel::Serializer::Adapter::Null < ActiveModel::Serializer::Adapter def serializable_hash(options = nil) {} end - end - end - end end diff --git a/lib/active_model/serializer/fieldset.rb b/lib/active_model/serializer/fieldset.rb index 30e68334..935aea81 100644 --- a/lib/active_model/serializer/fieldset.rb +++ b/lib/active_model/serializer/fieldset.rb @@ -18,7 +18,7 @@ module ActiveModel private ActiveModelSerializers.silence_warnings do - attr_reader :raw_fields, :root + attr_reader :raw_fields, :root end def parsed_fields diff --git a/test/action_controller/serialization_scope_name_test.rb b/test/action_controller/serialization_scope_name_test.rb index 6e2b15e6..51abf108 100644 --- a/test/action_controller/serialization_scope_name_test.rb +++ b/test/action_controller/serialization_scope_name_test.rb @@ -25,7 +25,7 @@ class DefaultScopeNameTest < ActionController::TestCase end end - tests UserTestController + tests UserTestController def test_default_scope_name get :render_new_user diff --git a/test/serializers/adapter_for_test.rb b/test/serializers/adapter_for_test.rb index c55c3a9e..b246b404 100644 --- a/test/serializers/adapter_for_test.rb +++ b/test/serializers/adapter_for_test.rb @@ -5,6 +5,11 @@ module ActiveModel def setup @previous_adapter = ActiveModel::Serializer.config.adapter + # Eager load adapters + ActiveModel::Serializer::Adapter.eager_load! + [:json_api, :flatten_json, :null, :json].each do |adapter_name| + ActiveModel::Serializer::Adapter.lookup(adapter_name) + end end def teardown diff --git a/test/test_helper.rb b/test/test_helper.rb index c523041d..e6ba0dff 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -39,15 +39,6 @@ else end require 'active_model_serializers' -# eager load autoloaded adapters -# rubocop:disable Lint/Void -require 'active_model/serializer/adapter' -ActiveModel::Serializer::Adapter::Null -ActiveModel::Serializer::Adapter::Json -ActiveModel::Serializer::Adapter::FlattenJson -ActiveModel::Serializer::Adapter::JsonApi -# rubocop:enable Lint/Void -require 'active_model/serializer/adapter' require 'support/stream_capture'