Skip to content

Commit

Permalink
Ensure inheritance hooks run
Browse files Browse the repository at this point in the history
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
rails-api#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.
  • Loading branch information
bf4 committed Sep 6, 2015
1 parent f3918e1 commit a13ffcf
Show file tree
Hide file tree
Showing 15 changed files with 67 additions and 88 deletions.
36 changes: 36 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,42 @@ AllCops:
DisplayCopNames: true
DisplayStyleGuide: true

Style/IndentationConsistency:
Exclude:
- lib/active_model/serializer/adapter.rb
- 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.rb
- 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.rb
- 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:
Expand Down
2 changes: 1 addition & 1 deletion lib/active_model/serializable_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def serializer?
private

ActiveModelSerializers.silence_warnings do
attr_reader :resource, :adapter_opts, :serializer_opts
attr_reader :resource, :adapter_opts, :serializer_opts
end
end
end
8 changes: 4 additions & 4 deletions lib/active_model/serializer/adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ class Adapter
UnknownAdapterError = Class.new(ArgumentError)
ADAPTER_MAP = {}
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)
Expand Down
8 changes: 1 addition & 7 deletions lib/active_model/serializer/adapter/flatten_json.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -13,7 +10,4 @@ def serializable_hash(options = {})
def include_meta(json)
json
end
end
end
end
end
8 changes: 1 addition & 7 deletions lib/active_model/serializer/adapter/fragment_cache.rb
Original file line number Diff line number Diff line change
@@ -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)
Expand Down Expand Up @@ -75,7 +72,4 @@ def fragment_serializer(name, klass)
def to_valid_const_name(name)
name.gsub('::', '_')
end
end
end
end
end
13 changes: 4 additions & 9 deletions lib/active_model/serializer/adapter/json.rb
Original file line number Diff line number Diff line change
@@ -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)
Expand Down Expand Up @@ -44,9 +42,6 @@ def serializable_hash(options = nil)
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
11 changes: 1 addition & 10 deletions lib/active_model/serializer/adapter/json/fragment_cache.rb
Original file line number Diff line number Diff line change
@@ -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
29 changes: 11 additions & 18 deletions lib/active_model/serializer/adapter/json_api.rb
Original file line number Diff line number Diff line change
@@ -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: [] }
Expand Down Expand Up @@ -39,11 +37,9 @@ def serializable_hash(options = nil)

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

def add_relationships(resource, name, serializers)
resource[:relationships] ||= {}
resource[:relationships][name] ||= { data: [] }
Expand Down Expand Up @@ -169,17 +165,14 @@ def add_links(options)
end
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

def is_paginated?(collection)
collection.respond_to?(:current_page) &&
collection.respond_to?(:total_pages) &&
collection.respond_to?(:size)
def is_paginated?(resource)
resource.respond_to?(:current_page) &&
resource.respond_to?(:total_pages) &&
resource.respond_to?(:size)
end
end
end
end
end
11 changes: 1 addition & 10 deletions lib/active_model/serializer/adapter/json_api/fragment_cache.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -15,8 +10,4 @@ def fragment_cache(root, cached_hash, non_cached_hash)

hash.deep_merge no_root_non_cache.deep_merge no_root_cache
end
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -51,8 +47,4 @@ def original_url
def query_parameters
@query_parameters ||= context.query_parameters
end
end
end
end
end
end
8 changes: 1 addition & 7 deletions lib/active_model/serializer/adapter/null.rb
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion lib/active_model/serializer/fieldset.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def fields_for(serializer)
private

ActiveModelSerializers.silence_warnings do
attr_reader :raw_fields, :root
attr_reader :raw_fields, :root
end

def parsed_fields
Expand Down
2 changes: 1 addition & 1 deletion test/action_controller/serialization_scope_name_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def render_new_user
end
end

tests UserTestController
tests UserTestController

def test_default_scope_name
get :render_new_user
Expand Down
5 changes: 2 additions & 3 deletions test/serializers/adapter_for_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,11 @@ def test_inherited_adapter_hooks_register_subclass_of_registered_adapter
assert_equal ActiveModel::Serializer::Adapter.get(:my_adapter), my_adapter
assert_equal ActiveModel::Serializer::Adapter.get(:my_subclassed_adapter), my_subclassed_adapter
ensure
ActiveModel::Serializer::Adapter::ADAPTER_MAP.delete("my_adapter".freeze)
ActiveModel::Serializer::Adapter::ADAPTER_MAP.delete("my_subclassed_adapter".freeze)
ActiveModel::Serializer::Adapter::ADAPTER_MAP.delete('my_adapter'.freeze)
ActiveModel::Serializer::Adapter::ADAPTER_MAP.delete('my_subclassed_adapter'.freeze)
Object.send(:remove_const, :MyAdapter)
Object.send(:remove_const, :MySubclassedAdapter)
end

end
end
end
2 changes: 1 addition & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@
# eager load autoloaded adapters
# rubocop:disable Lint/Void
require 'active_model/serializer/adapter'
ActiveModel::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'

Expand Down

0 comments on commit a13ffcf

Please sign in to comment.