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

bf4 · 2015-07-17T19:54:57Z

Changes:

Introduce Adapter::lookup for use by Serializer.adapter
Move Adapter-finding logic from Adapter::adapter_class into Adapter::lookup
Fix circular load warning as described in cf6a074
Allowed for gem devs to use a Gemfile.local for gems not in the gemspec or Gemfile e.g. pry-byebug (which was my usecase).

Background Ref:

Add top-level jsonapi member #1050 (comment)
Fix warnings #1067 (comment).
The code in this PR was my first attempt to deal with Fix adapter inflection bug for api -> API #1006 till I realized a simpler solution..

Introduced interfaces:

non-inherited methods

ActiveModel::Serializer::Adapter.adapter_map  # a Hash<adapter_name, adapter_class>
ActiveModel::Serializer::Adapter.adapters     # an Array<adapter_name>
ActiveModel::Serializer::Adapter.register(name, klass) # adds an adapter to the adapter_map
ActiveModel::Serializer::Adapter.lookup(name_or_klass)    # raises UnknownAdapterError error when adapter not found

Automatically register adapters when subclassing

     def self.inherited(subclass)
       ActiveModel::Serializer::Adapter.register(subclass.to_s.demodulize,
subclass)
     end

Preserves subclass method ::adapter_class(adapter)

     def self.adapter_class(adapter)
       ActiveModel::Serializer::Adapter.lookup(adapter)
     end

Serializer.adapter now uses Adapter.lookup(config.adapter) rather than have
duplicate logic

TODO

Test

ActiveModel::Serializer::Adapter.adapter_map
ActiveModel::Serializer::Adapter.adapters
ActiveModel::Serializer::Adapter.register(name, klass)
ActiveModel::Serializer::Adapter.get(name_or_klass)
ActiveModel::Serializer::Adapter::inherited
ActiveModel::Serializer.adapter

TO CONSIDER

wrap 'finding adapter by name' logic outside of 'get' method
Change exception from ArgumentError to UnknownAdapter

TODO in future PR:

Change Adapter to module (backwards compatible public methods) and make Adapter::Base or Adapter::AbstractClass

Full commit message from cf6a074

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
#1067

bf4 · 2015-08-21T02:37:39Z

@joaomdmoura Ready for review!

bf4 · 2015-08-21T02:42:36Z

test/adapter_test.rb

moved to adapter_for file

joaomdmoura · 2015-08-26T08:00:17Z

lib/active_model/serializer/adapter.rb

bf4 · 2015-08-28T19:18:53Z

rebased off of master...

bf4 · 2015-08-28T21:58:24Z

You'll likely want to take a look at the commit message in cf6a074

sandstrom · 2015-08-30T09:47:37Z

lib/active_model/serializer/adapter/flatten_json.rb

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.

ref: #1017 (comment)

Ruby Style Guide encourages the previous style

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

bolshakov · 2015-09-08T08:20:19Z

lib/active_model/serializer/adapter.rb

Better:

@return [Array<Symbol>] list of adapter names

See http://yardoc.org/types.html

yeah, just me being a little lazy because we're not actually using yardoc (yet?)

Per rails-api#1017 (comment) comment by sandstrom in discussion of the inherited hook > 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

Changes: - Introduce Adapter::get for use by Serializer.adapter - Move Adapter-finding logic from Adapter::adapter_class into Adapter::get Introduced interfaces: - non-inherited methods ```ruby ActiveModel::Serializer::Adapter.adapter_map # a Hash<adapter_name, adapter_class> ActiveModel::Serializer::Adapter.adapters # an Array<adapter_name> ActiveModel::Serializer::Adapter.register(name, klass) # adds an adapter to the adapter_map ActiveModel::Serializer::Adapter.get(name_or_klass) # raises Argument error when adapter not found ``` - Automatically register adapters when subclassing ```ruby def self.inherited(subclass) ActiveModel::Serializer::Adapter.register(subclass.to_s.demodulize, subclass) end ``` - Preserves subclass method `::adapter_class(adapter)` ```ruby def self.adapter_class(adapter) ActiveModel::Serializer::Adapter.get(adapter) end ``` - Serializer.adapter now uses `Adapter.get(config.adapter)` rather than have duplicate logic

Per rails-api#1017 (comment) comment by sandstrom in discussion of the inherited hook > 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/a11571cec3213753d63ac3e6b4bb3b97fe2594a6/activejob/lib/active_job/queue_adapter.rb#L52-L56

bf4 · 2015-09-09T04:13:23Z

@joaomdmoura @bolshakov @sandstrom All cleaned up!

sandstrom · 2015-09-09T11:14:16Z

@bf4 Awesome work on this!

While we disagreed on some of the details, I'm overall very positive to making adapters more flexible. ⛵

bolshakov · 2015-09-09T12:10:00Z

👍

bf4 · 2015-09-09T13:47:27Z

Failures on appveyor

 1) Failure:
ActiveModel::Serializer::AdapterForTest#test_adapters [C:/projects/active-model-serializers/test/serializers/adapter_for_test.rb:73]:
--- expected
+++ actual
@@ -1 +1 @@
-["flatten_json", "json", "json_api"]
+["flatten_json", "json", "json_api", "null"]

 1) Failure:
ActionController::Serialization::ImplicitSerializerTest#test_cache_expiration_on_update [C:/projects/active-model-serializers/test/action_controller/serialization_test.rb:394]:
--- expected
+++ actual
@@ -1 +1 @@
-"{\"id\":1,\"title\":\"ZOMG a New Post\",\"body\":\"Body\",\"comments\":[{\"id\":1,\"body\":\"ZOMG A COMMENT\"}],\"blog\":{\"id\":999,\"name\":\"Custom blog\"},\"author\":{\"id\":1,\"name\":\"Joao Moura.\"}}"
+"{\"id\":1,\"title\":\"New Post\",\"body\":\"Body\",\"comments\":[{\"id\":1,\"body\":\"ZOMG A COMMENT\"}],\"blog\":{\"id\":999,\"name\":\"Custom blog\"},\"author\":{\"id\":1,\"name\":\"Joao Moura.\"}}"

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. with '#' will be ignored, and an empty message aborts the commit.

Per rails-api#1017 (comment) comment by sandstrom in discussion of the inherited hook > 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/a11571cec3213753d63ac3e6b4bb3b97fe2594a6/activejob/lib/active_job/queue_adapter.rb#L52-L56

(Thanks to sandstrom for the reference to ActiveJob::QueueAdapters https://github.com/rails/rails/blob/a11571cec3213753d63ac3e6b4bb3b97fe2594a6/activejob/lib/active_job/queue_adapters.rb#L123-L133

bf4 · 2015-09-09T14:27:37Z

Fixed the build

joaomdmoura · 2015-09-11T05:24:31Z

Nice work! Really thank you everyone for helping out with this one!
It's a big one and will definitely help us into building new functionalities.
Merging.

Make Adapters registerable so they are not namespace-constrained

beauby · 2015-09-14T22:21:47Z

lib/active_model/serializer/adapter/json_api.rb

Why, why, why? :D

Rebase foul

…#1017.

Remove legacy method accidentally reintroduced in #1017

* master: (170 commits) Comment private accessor warnings Distinguish options ivar from local; Extract latent Adapter::CachedSerializer Fix or skip appveyor failure on cache expiration Fixing the travis build svg to amster updating version to new release Remove duplicate test helper outside controller use tutorial rubocop-fixes Remove unnecessary parentheses accidentally reintroduced in rails-api#1017. Remove legacy method accidentally reintroduced in rails-api#1017. Update README with nested included association example. Split `serializable_hash` into two methods. Refactor `add_links` in JSONAPI adapter. Fix Markdown to adapters documentation Extended format for JSONAPI `include` option. Updating wording on cache expiry in README Fix typo in fieldset exception Fixed indentation in readme under 'using without render' Documentation for serializing resources without render Get rid of unnecessary instance variables, and implied dependencies. ... Conflicts: lib/active_model/serializer/adapter/json_api.rb

bf4 mentioned this pull request Jul 17, 2015

Fix adapter inflection bug for api -> API #1006

Merged

bf4 force-pushed the registerable_adapters branch 3 times, most recently from a19096d to 0d617cd Compare August 21, 2015 02:36

bf4 reviewed Aug 21, 2015
View reviewed changes

test/adapter_test.rb

Copy link

Member Author

bf4 Aug 21, 2015

Choose a reason for hiding this comment

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

moved to adapter_for file

bf4 mentioned this pull request Aug 21, 2015

Add top-level jsonapi member #1050

Merged

bf4 force-pushed the registerable_adapters branch 3 times, most recently from 92df577 to 8da44d7 Compare August 21, 2015 06:01

joaomdmoura reviewed Aug 26, 2015
View reviewed changes

lib/active_model/serializer/adapter.rb Outdated

Copy link

Member

joaomdmoura Aug 26, 2015

Choose a reason for hiding this comment

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

👍

joaomdmoura self-assigned this Aug 26, 2015

joaomdmoura added the V: 0.10.x label Aug 26, 2015

This was referenced Aug 26, 2015

Add option to select adapter by the request header #1082

Closed

[RFC] JSON API Errors (Initial implementation and roadmap for full feature-set) #1004

Merged

bf4 force-pushed the registerable_adapters branch from 8da44d7 to 18e5812 Compare August 28, 2015 19:18

bf4 mentioned this pull request Aug 30, 2015

Overriding adapter #1039

Closed

sandstrom reviewed Aug 30, 2015
View reviewed changes

This was referenced Aug 31, 2015

multiple circular requires cause very lengthy warnings #571

Closed

[WIP] Nested Sideloading for the JSON adapter #1107

Closed

bolshakov reviewed Sep 8, 2015
View reviewed changes

bf4 force-pushed the registerable_adapters branch from a13ffcf to 9952073 Compare September 9, 2015 03:55

bf4 force-pushed the registerable_adapters branch from e096d22 to a72b76a Compare September 9, 2015 04:07

bf4 force-pushed the registerable_adapters branch from a72b76a to 6e886c0 Compare September 9, 2015 04:12

bf4 added 4 commits September 9, 2015 08:55

Use Adapter.const_get instead of safe_constantize

28345ad

(Thanks to sandstrom for the reference to ActiveJob::QueueAdapters https://github.com/rails/rails/blob/a11571cec3213753d63ac3e6b4bb3b97fe2594a6/activejob/lib/active_job/queue_adapters.rb#L123-L133

Lower minimum coverage, minimal changes in this PR

880f235

bf4 force-pushed the registerable_adapters branch from 4599d9f to 880f235 Compare September 9, 2015 13:55

joaomdmoura added a commit that referenced this pull request Sep 11, 2015

Merge pull request #1017 from bf4/registerable_adapters

4399c1a

Make Adapters registerable so they are not namespace-constrained

joaomdmoura merged commit 4399c1a into rails-api:master Sep 11, 2015

beauby reviewed Sep 14, 2015
View reviewed changes

beauby added a commit to beauby/active_model_serializers that referenced this pull request Sep 14, 2015

Remove legacy method accidentally reintroduced in rails-api#1017.

319a907

beauby added a commit to beauby/active_model_serializers that referenced this pull request Sep 14, 2015

Remove unnecessary parentheses accidentally reintroduced in rails-api…

fb7ec88

…#1017.

NullVoxPopuli mentioned this pull request Sep 14, 2015

Remove legacy method accidentally reintroduced in #1017 #1150

Merged

NullVoxPopuli added a commit that referenced this pull request Sep 14, 2015

Merge pull request #1150 from beauby/fix-jsonapi-add-relationships

d7534a6

Remove legacy method accidentally reintroduced in #1017

bf4 deleted the registerable_adapters branch August 31, 2016 06:19

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

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

Uh oh!

Conversation

bf4 commented Jul 17, 2015

Uh oh!

bf4 commented Aug 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf4 commented Aug 28, 2015

Uh oh!

bf4 commented Aug 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf4 commented Sep 9, 2015

Uh oh!

sandstrom commented Sep 9, 2015

Uh oh!

bolshakov commented Sep 9, 2015

Uh oh!

bf4 commented Sep 9, 2015

Uh oh!

bf4 commented Sep 9, 2015

Uh oh!

joaomdmoura commented Sep 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants