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.
This commit is contained in:
Benjamin Fleischer 2015-08-28 16:41:21 -05:00
parent d9e76c29d5
commit af99c0d9e6
15 changed files with 73 additions and 95 deletions

View File

@ -8,6 +8,39 @@ AllCops:
DisplayCopNames: true DisplayCopNames: true
DisplayStyleGuide: 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: Lint/NestedMethodDefinition:
Enabled: false Enabled: false
Exclude: Exclude:
@ -16,9 +49,6 @@ Lint/NestedMethodDefinition:
Style/StringLiterals: Style/StringLiterals:
EnforcedStyle: single_quotes EnforcedStyle: single_quotes
Style/SpecialGlobalVars:
Enabled: false
Metrics/AbcSize: Metrics/AbcSize:
Max: 35 # TODO: Lower to 15 Max: 35 # TODO: Lower to 15

View File

@ -76,7 +76,7 @@ module ActiveModel
private private
ActiveModelSerializers.silence_warnings do ActiveModelSerializers.silence_warnings do
attr_reader :resource, :adapter_opts, :serializer_opts attr_reader :resource, :adapter_opts, :serializer_opts
end end
end end
end end

View File

@ -5,11 +5,11 @@ module ActiveModel
ADAPTER_MAP = {} ADAPTER_MAP = {}
private_constant :ADAPTER_MAP if defined?(private_constant) private_constant :ADAPTER_MAP if defined?(private_constant)
extend ActiveSupport::Autoload extend ActiveSupport::Autoload
require 'active_model/serializer/adapter/json'
require 'active_model/serializer/adapter/json_api'
autoload :FlattenJson
autoload :Null
autoload :FragmentCache autoload :FragmentCache
autoload :Json
autoload :JsonApi
autoload :Null
autoload :FlattenJson
def self.create(resource, options = {}) def self.create(resource, options = {})
override = options.delete(:adapter) override = options.delete(:adapter)
@ -60,14 +60,14 @@ module ActiveModel
register(adapter_name, adapter_class) register(adapter_name, adapter_class)
adapter_class adapter_class
} }
rescue ArgumentError rescue ArgumentError => e
failure_message = failure_message =
"Unknown adapter: #{adapter.inspect}. Valid adapters are: #{adapters}" "Unknown adapter: #{adapter.inspect}. Valid adapters are: #{adapters}"
raise UnknownAdapterError, failure_message, $!.backtrace raise UnknownAdapterError, failure_message, e.backtrace
rescue NameError rescue NameError => e
failure_message = failure_message =
"NameError: #{$!.message}. Unknown adapter: #{adapter.inspect}. Valid adapters are: #{adapters}" "NameError: #{e.message}. Unknown adapter: #{adapter.inspect}. Valid adapters are: #{adapters}"
raise UnknownAdapterError, failure_message, $!.backtrace raise UnknownAdapterError, failure_message, e.backtrace
end end
# @api private # @api private

View File

@ -1,7 +1,4 @@
module ActiveModel class ActiveModel::Serializer::Adapter::FlattenJson < ActiveModel::Serializer::Adapter::Json
class Serializer
class Adapter
class FlattenJson < Json
def serializable_hash(options = {}) def serializable_hash(options = {})
super super
@result @result
@ -13,7 +10,4 @@ module ActiveModel
def include_meta(json) def include_meta(json)
json json
end end
end
end
end
end end

View File

@ -1,7 +1,4 @@
module ActiveModel class ActiveModel::Serializer::Adapter::FragmentCache
class Serializer
class Adapter
class FragmentCache
attr_reader :serializer attr_reader :serializer
def initialize(adapter, serializer, options) def initialize(adapter, serializer, options)
@ -75,7 +72,4 @@ module ActiveModel
def to_valid_const_name(name) def to_valid_const_name(name)
name.gsub('::', '_') name.gsub('::', '_')
end end
end
end
end
end end

View File

@ -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) def serializable_hash(options = nil)
options ||= {} options ||= {}
if serializer.respond_to?(:each) if serializer.respond_to?(:each)
@ -44,9 +42,6 @@ module ActiveModel
end end
def fragment_cache(cached_hash, non_cached_hash) 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
end end

View File

@ -1,14 +1,5 @@
require 'active_model/serializer/adapter/fragment_cache' class ActiveModel::Serializer::Adapter::Json::FragmentCache
module ActiveModel
class Serializer
class Adapter
class Json < Adapter
class FragmentCache
def fragment_cache(cached_hash, non_cached_hash) def fragment_cache(cached_hash, non_cached_hash)
non_cached_hash.merge cached_hash non_cached_hash.merge cached_hash
end end
end
end
end
end
end end

View File

@ -1,10 +1,8 @@
require 'active_model/serializer/adapter/json_api/fragment_cache' class ActiveModel::Serializer::Adapter::JsonApi < ActiveModel::Serializer::Adapter
require 'active_model/serializer/adapter/json_api/pagination_links' extend ActiveSupport::Autoload
autoload :PaginationLinks
autoload :FragmentCache
module ActiveModel
class Serializer
class Adapter
class JsonApi < Adapter
def initialize(serializer, options = {}) def initialize(serializer, options = {})
super super
@hash = { data: [] } @hash = { data: [] }
@ -49,7 +47,7 @@ module ActiveModel
def fragment_cache(cached_hash, non_cached_hash) def fragment_cache(cached_hash, non_cached_hash)
root = false if @options.include?(:include) 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 end
private private
@ -62,6 +60,12 @@ module ActiveModel
end end
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) def resource_identifier_id_for(serializer)
if serializer.respond_to?(:id) if serializer.respond_to?(:id)
serializer.id serializer.id
@ -161,8 +165,8 @@ module ActiveModel
@hash[:links] = add_pagination_links(links, collection, options) if paginated?(collection) @hash[:links] = add_pagination_links(links, collection, options) if paginated?(collection)
end end
def add_pagination_links(links, collection, options) def add_pagination_links(links, resources, options)
pagination_links = JsonApi::PaginationLinks.new(collection, options[:context]).serializable_hash(options) pagination_links = ActiveModel::Serializer::Adapter::JsonApi::PaginationLinks.new(resources, options[:context]).serializable_hash(options)
links.update(pagination_links) links.update(pagination_links)
end end
@ -171,7 +175,4 @@ module ActiveModel
collection.respond_to?(:total_pages) && collection.respond_to?(:total_pages) &&
collection.respond_to?(:size) collection.respond_to?(:size)
end end
end
end
end
end end

View File

@ -1,9 +1,4 @@
require 'active_model/serializer/adapter/fragment_cache' class ActiveModel::Serializer::Adapter::JsonApi::FragmentCache
module ActiveModel
class Serializer
class Adapter
class JsonApi < Adapter
class FragmentCache
def fragment_cache(root, cached_hash, non_cached_hash) def fragment_cache(root, cached_hash, non_cached_hash)
hash = {} hash = {}
core_cached = cached_hash.first core_cached = cached_hash.first
@ -15,8 +10,4 @@ module ActiveModel
hash.deep_merge no_root_non_cache.deep_merge no_root_cache hash.deep_merge no_root_non_cache.deep_merge no_root_cache
end end
end
end
end
end
end end

View File

@ -1,8 +1,4 @@
module ActiveModel class ActiveModel::Serializer::Adapter::JsonApi::PaginationLinks
class Serializer
class Adapter
class JsonApi < Adapter
class PaginationLinks
FIRST_PAGE = 1 FIRST_PAGE = 1
attr_reader :collection, :context attr_reader :collection, :context
@ -51,8 +47,4 @@ module ActiveModel
def query_parameters def query_parameters
@query_parameters ||= context.query_parameters @query_parameters ||= context.query_parameters
end end
end
end
end
end
end end

View File

@ -1,11 +1,5 @@
module ActiveModel class ActiveModel::Serializer::Adapter::Null < ActiveModel::Serializer::Adapter
class Serializer
class Adapter
class Null < Adapter
def serializable_hash(options = nil) def serializable_hash(options = nil)
{} {}
end end
end
end
end
end end

View File

@ -18,7 +18,7 @@ module ActiveModel
private private
ActiveModelSerializers.silence_warnings do ActiveModelSerializers.silence_warnings do
attr_reader :raw_fields, :root attr_reader :raw_fields, :root
end end
def parsed_fields def parsed_fields

View File

@ -25,7 +25,7 @@ class DefaultScopeNameTest < ActionController::TestCase
end end
end end
tests UserTestController tests UserTestController
def test_default_scope_name def test_default_scope_name
get :render_new_user get :render_new_user

View File

@ -5,6 +5,11 @@ module ActiveModel
def setup def setup
@previous_adapter = ActiveModel::Serializer.config.adapter @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 end
def teardown def teardown

View File

@ -39,15 +39,6 @@ else
end end
require 'active_model_serializers' 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' require 'support/stream_capture'