Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Adapters registerable so they are not namespace-constrained #1017

Merged
merged 5 commits into from
Sep 11, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
.config
.yardoc
Gemfile.lock
Gemfile.local
InstalledFiles
_yardoc
coverage
Expand Down
33 changes: 33 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion .simplecov
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
when 'jruby', 'rbx'
96.0
else
98.3
98.1
end
}.to_f.round(2)
# rubocop:disable Style/DoubleNegation
Expand Down
4 changes: 4 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
source 'https://rubygems.org'
#
# Add a Gemfile.local to locally bundle gems outside of version control
local_gemfile = File.join(File.expand_path('..', __FILE__), 'Gemfile.local')
eval_gemfile local_gemfile if File.readable?(local_gemfile)

# Specify your gem's dependencies in active_model_serializers.gemspec
gemspec
Expand Down
55 changes: 53 additions & 2 deletions docs/general/adapters.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ resources in the `"included"` member when the resource names are included in the

## Choosing an adapter

If you want to use a different adapter, such as JsonApi, you can change this in an initializer:
If you want to use a specify a default adapter, such as JsonApi, you can change this in an initializer:

```ruby
ActiveModel::Serializer.config.adapter = ActiveModel::Serializer::Adapter::JsonApi
Expand All @@ -44,8 +44,59 @@ or
ActiveModel::Serializer.config.adapter = :json_api
```

If you want to have a root key in your responses you should use the Json adapter, instead of the default FlattenJson:
If you want to have a root key for each resource in your responses, you should use the Json or
JsonApi adapters instead of the default FlattenJson:

```ruby
ActiveModel::Serializer.config.adapter = :json
```

## Advanced adapter configuration

### Registering an adapter

The default adapter can be configured, as above, to use any class given to it.

An adapter may also be specified, e.g. when rendering, as a class or as a symbol.
If a symbol, then the adapter must be, e.g. `:great_example`,
`ActiveModel::Serializer::Adapter::GreatExample`, or registered.

There are two ways to register an adapter:

1) The simplest, is to subclass `ActiveModel::Serializer::Adapter`, e.g. the below will
register the `Example::UsefulAdapter` as `:useful_adapter`.

```ruby
module Example
class UsefulAdapter < ActiveModel::Serializer::Adapter
end
end
```

You'll notice that the name it registers is the class name underscored, not the full namespace.

Under the covers, when the `ActiveModel::Serializer::Adapter` is subclassed, it registers
the subclass as `register(:useful_adapter, Example::UsefulAdapter)`

2) Any class can be registered as an adapter by calling `register` directly on the
`ActiveModel::Serializer::Adapter` class. e.g., the below registers `MyAdapter` as
`:special_adapter`.

```ruby
class MyAdapter; end
ActiveModel::Serializer::Adapter.register(:special_adapter, MyAdapter)
```

### Looking up an adapter

| `ActiveModel::Serializer::Adapter.adapter_map` | A Hash of all known adapters { adapter_name => adapter_class } |
| `ActiveModel::Serializer::Adapter.adapters` | A (sorted) Array of all known adapter_names |
| `ActiveModel::Serializer::Adapter.lookup(name_or_klass)` | The adapter_class, else raises an `ActiveModel::Serializer::Adapter::UnknownAdapter` error |
| `ActiveModel::Serializer::Adapter.adapter_class(adapter)` | delegates to `ActiveModel::Serializer::Adapter.lookup(adapter)` |
| `ActiveModel::Serializer.adapter` | a convenience method for `ActiveModel::Serializer::Adapter.lookup(config.adapter)` |

The registered adapter name is always a String, but may be looked up as a Symbol or String.
Helpfully, the Symbol or String is underscored, so that `get(:my_adapter)` and `get("MyAdapter")`
may both be used.

For more information, see [the Adapter class on GitHub](https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/adapter.rb)
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
14 changes: 2 additions & 12 deletions lib/active_model/serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,19 +94,9 @@ def self.serializer_for(resource, options = {})
end
end

# @see ActiveModel::Serializer::Adapter.lookup
def self.adapter
adapter_class = case config.adapter
when Symbol
ActiveModel::Serializer::Adapter.adapter_class(config.adapter)
when Class
config.adapter
end
unless adapter_class
valid_adapters = Adapter.constants.map { |klass| ":#{klass.to_s.downcase}" }
raise ArgumentError, "Unknown adapter: #{config.adapter}. Valid adapters are: #{valid_adapters}"
end

adapter_class
ActiveModel::Serializer::Adapter.lookup(config.adapter)
end

def self.root_name
Expand Down
73 changes: 67 additions & 6 deletions lib/active_model/serializer/adapter.rb
Original file line number Diff line number Diff line change
@@ -1,22 +1,83 @@
module ActiveModel
class Serializer
class Adapter
UnknownAdapterError = Class.new(ArgumentError)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think class UnknownAdapterError < ArgumentError; end is a more common syntax.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're both pretty common for exceptions, and is pretty much required for structs. That the poro fixtures do it, though, I think is problematic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, both are common, I'm up for both, but let's keep if in order to keep the way we are doing this across the code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only use the Class.new and Struct.new forms for dynamic class construction.

  • Foo = Struct.new because otherwise you get superclass mismatch exceptions
  • Class.new for one-liner Exception or sentinal objects, e.g. BadObject = Class.new; if obj == BadOjbect..... (or when super serious meta-programming requires it).

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)
klass = override ? adapter_class(override) : ActiveModel::Serializer.adapter
klass.new(resource, options)
end

# @see ActiveModel::Serializer::Adapter.lookup
def self.adapter_class(adapter)
adapter_name = adapter.to_s.classify.sub('API', 'Api')
"ActiveModel::Serializer::Adapter::#{adapter_name}".safe_constantize
ActiveModel::Serializer::Adapter.lookup(adapter)
end

# Only the Adapter class has these methods.
# None of the sublasses have them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

class << ActiveModel::Serializer::Adapter
# @return Hash<adapter_name, adapter_class>
def adapter_map
ADAPTER_MAP
end

# @return [Array<Symbol>] list of adapter names
def adapters
adapter_map.keys.sort
end

# Adds an adapter 'klass' with 'name' to the 'adapter_map'
# Names are stringified and underscored
# @param [Symbol, String] name of the registered adapter
# @param [Class] klass - adapter class itself
# @example
# AMS::Adapter.register(:my_adapter, MyAdapter)
def register(name, klass)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@param [Symbol, String] name of the registered adapter
@param [Class] klass - adapter class itself
@example 
    AMS::Adapter.register(:my_adapter, MyAdapter)

adapter_map.update(name.to_s.underscore => klass)
self
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaomdmoura Do we want a remove method? In the tests I faked it.. edge case..

ActiveModel::Serializer::Adapter::ADAPTER_MAP.delete(name)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, edge case, if someone bring this up then we can check


# @param adapter [String, Symbol, Class] name to fetch adapter by
# @return [ActiveModel::Serializer::Adapter] subclass of Adapter
# @raise [UnknownAdapterError]
def lookup(adapter)
# 1. return if is a class
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandstrom re: ActiveJob QueueAdapter and QueueAdapters I think I like lookup more than get, so thanks for pointing me that way ;) You think I should change it?

However, I think there are some limitations in that interface in that

  1. activejob queue adapters are only used when assigned via queue_adapter =, but there's no way to know if and when an AMS adapter will be used
  2. I like const_get instead of safe_constantize, but we'd still need to rescue a NameError and use the AM::S::Adapter namespace, at least for dynamic lookup
  3. I like that you can pass interpret_adapter a class and it can check it for behavior, but I think that's outside the scope of this PR
  4. I'm glad to get the reminder about private_constant

return adapter if adapter.is_a?(Class)
adapter_name = adapter.to_s.underscore
# 2. return if registered
adapter_map.fetch(adapter_name) {
# 3. try to find adapter class from environment
adapter_class = find_by_name(adapter_name)
register(adapter_name, adapter_class)
adapter_class
}
rescue NameError, ArgumentError => e
failure_message =
"NameError: #{e.message}. Unknown adapter: #{adapter.inspect}. Valid adapters are: #{adapters}"
raise UnknownAdapterError, failure_message, e.backtrace
end

# @api private
def find_by_name(adapter_name)
adapter_name = adapter_name.to_s.classify.tr('API', 'Api')
ActiveModel::Serializer::Adapter.const_get(adapter_name.to_sym) or # rubocop:disable Style/AndOr
fail UnknownAdapterError
end
private :find_by_name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split this off just to make it a little easier to read..

end

# Automatically register adapters when subclassing
def self.inherited(subclass)
ActiveModel::Serializer::Adapter.register(subclass.to_s.demodulize, subclass)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a possibility here that two adapters will have the same namespace...

end

attr_reader :serializer
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the previous style more readable. That's also the syntax (I think) Rails uses internally, perhaps it would be a good idea to align with Rails code style.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruby Style Guide encourages the previous style

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even for classes as namespaces? link plz? (besides that it doesn't work in this case, I think it's wrong)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, I find the previous class syntax more readable, and it aligns with Rails coding style.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments! I think aligning with Rails style is a good idea. Re: this change plz see the commit for details why

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(for reference, the commit message is here bf4@cf6a074)

To avoid the collision issue I think it's good to only use module for namespacing. I think that should avoid the issue, right?

I also think that automatic registration when subclassing is very magical. My guess is the common 'Rails-way' is to do on-the-fly lookups when an adapter is needed (constantize a string and look in some pre-defined directory, e.g. app/serializers/adapters) or to register them explicitly in an initializer.

But I'm no Rails-expert, I may be wrong on the conventions. Perhaps @spastorino can weight in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adapter is a class. For now, adapter can't be a module. Anyway, the module convention exists to make things work. This change forces all non-loaded superclasses to be autoloaded when referenced. That's the only way to do it now.

If you think using an inherited hook is magical... Well, it's a pretty fundamental part of Ruby meta programming. Rails uses these hooks all over the place. If you forget the hook exists, do you prefer the way this pr behaves? I.e. You can name your adapter whatever you want.

P.s. I do intend to fix the name spacing issue after this is merged

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that's the only way if the inherited hook is used.

I'm thinking that it would be better to register adapters manually, without using the hook, i.e. have people call ActiveModel::Serializer::Adapter.register directly (perhaps in an initializer).

Possibly, some inspiration can be taken from how ActiveJob adapters are wired[1].

[1] https://github.com/rails/rails/blob/master/activejob/lib/active_job/queue_adapter.rb


Also, I hope I don't come across as negative, I eager to see more flexibility around adapters! That's how I noticed this PR in the first place. I'm just suggesting that some aspects be structured a little differently.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I don't come across as negative,

Me as well. Thanks for bringing it up. I think I was being a little snippy.

In terms of the feature, I'm not married to the inner workings, but I do like the Adapter.get and Adapter.register as the source of truth for getting and setting adapters. And that it removes the requirement that a custom adapter be in the ActiveModel::Serializer::Adapter namespace

In terms of borrowing the interface from ActiveJob, I think that's a really great idea! cc @seuros

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: the namespace issue, I'm going to update the PR description to reference it. I also discussed it in #1050 (comment) and #1067 (comment). The code in this PR was my first attempt to deal with #1006 till I realized a simpler solution..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great discussion, indeed I agree that the previous class syntax was more readable, but have the hook might be a good trade off, this is a big refactor and also add some new behaviors, let's keep it the way it is now, and try to address a way of doing this and gaining back readability on other PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to remove the inherited hook, and replace it with a method in the Adapter class itself, but even then, I'd need to prefix every file that may define class Adapter rather than reopen it, with something like adapter = adapter = ActiveModel::Serializer::Adapter # Need to load the Adapter before opening it below

Not even Adapter.eager_load! is sufficient.

def fragment_cache(cached_hash, non_cached_hash)
non_cached_hash.merge cached_hash
end
end
end
end
end
end
25 changes: 13 additions & 12 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 @@ -49,7 +47,7 @@ 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
Expand All @@ -62,6 +60,12 @@ def resource_identifier_type_for(serializer)
end
end

def add_relationships(resource, name, serializers)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why, why, why? :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase foul

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
Expand Down Expand Up @@ -161,8 +165,8 @@ def add_links(options)
@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

Expand All @@ -171,7 +175,4 @@ def paginated?(collection)
collection.respond_to?(:total_pages) &&
collection.respond_to?(:size)
end
end
end
end
end
Loading