-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from all commits
d9e76c2
af99c0d
363345b
28345ad
880f235
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
.config | ||
.yardoc | ||
Gemfile.lock | ||
Gemfile.local | ||
InstalledFiles | ||
_yardoc | ||
coverage | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ | |
when 'jruby', 'rbx' | ||
96.0 | ||
else | ||
98.3 | ||
98.1 | ||
end | ||
}.to_f.round(2) | ||
# rubocop:disable Style/DoubleNegation | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,83 @@ | ||
module ActiveModel | ||
class Serializer | ||
class Adapter | ||
UnknownAdapterError = Class.new(ArgumentError) | ||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
adapter_map.update(name.to_s.underscore => klass) | ||
self | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joaomdmoura Do we want a
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sandstrom re: ActiveJob QueueAdapter and QueueAdapters I think I like However, I think there are some limitations in that interface in that
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ref: #1017 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ruby Style Guide encourages the previous style There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -13,7 +10,4 @@ def serializable_hash(options = {}) | |
def include_meta(json) | ||
json | ||
end | ||
end | ||
end | ||
end | ||
end |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. But I'm no Rails-expert, I may be wrong on the conventions. Perhaps @spastorino can weight in? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that's the only way if the I'm thinking that it would be better to register adapters manually, without using the hook, i.e. have people call Possibly, some inspiration can be taken from how [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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 In terms of borrowing the interface from ActiveJob, I think that's a really great idea! cc @seuros There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Not even |
||
def fragment_cache(cached_hash, non_cached_hash) | ||
non_cached_hash.merge cached_hash | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
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: [] } | ||
|
@@ -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 | ||
|
@@ -62,6 +60,12 @@ def resource_identifier_type_for(serializer) | |
end | ||
end | ||
|
||
def add_relationships(resource, name, serializers) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why, why, why? :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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 @@ 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 | ||
|
||
|
@@ -171,7 +175,4 @@ def paginated?(collection) | |
collection.respond_to?(:total_pages) && | ||
collection.respond_to?(:size) | ||
end | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
andStruct.new
forms for dynamic class construction.Foo = Struct.new
because otherwise you get superclass mismatch exceptionsClass.new
for one-liner Exception or sentinal objects, e.g.BadObject = Class.new; if obj == BadOjbect....
. (or when super serious meta-programming requires it).