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

Add option to select adapter by the request header #1082

Closed
wants to merge 3 commits into from
Closed
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
26 changes: 23 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,36 @@ end

Generally speaking, you as a user of AMS will write (or generate) these
serializer classes. If you want to use a different adapter, such as a JsonApi, you can
change this in an initializer:
change this in your render, or in an initializer:

```ruby
#class
ActiveModel::Serializer.config.adapter = ActiveModel::Serializer::Adapter::JsonApi
```

#symbol
ActiveModel::Serializer.config.adapter = :json_api
```
or

```ruby
ActiveModel::Serializer.config.adapter = :json_api
render json: @posts, adapter: :json_api
```
Copy link
Member

Choose a reason for hiding this comment

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

Despite of being simple, would be nice to have in our new docs, maybe some same article on "How to use multiple adapters on the same app" or "How to use a specific adapter on a request"


You can use `Accept` HTTP header-field to select a specific adapter by enabling it.

```ruby
ActiveModel::Serializer.config.enabled_adapters_by_media_type = true
```
By default, AMS responds to media type `application/json` and `application/vnd.api+json` using the `FlattenJson` and `JsonApi` adapters respectively. If you want to add a different media type, override the defaults or even create your own, you can do it in your initializer.

To override media type `application/json` to respond with `Json` instead of `FlattenJson` adapter:
```ruby
ActiveModel::Serializer.config.custom_media_type_adapters = {"application/json" => :json}
```

If you have a custom adapter (`xml`, as example) you can create your own media type:
```ruby
ActiveModel::Serializer.config.custom_media_type_adapters = {"application/xml" => :xml}
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to explain how to look up process works. Right now it seems that the application/json are the key factor but the ActiveModel::Serializer.config.adapter have priority over it.

This can also be on the New docs, on the same article and even on a new one like "how to support older versions and build a new one using json-api" or something like it, because it's a really useful way to use multiple adapters, it even enables you to have older versions of your API using json or flatten-json e a new version using the Header and json-api adapter.

But here on README would be nice to make it clear the look up logic.

Copy link
Member

Choose a reason for hiding this comment

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

btw the look up afaik should be config on initializer > adapter option on render > Accept Header

```

You won't need to implement an adapter unless you wish to use a new format or
Expand Down
6 changes: 6 additions & 0 deletions lib/action_controller/serialization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ def get_serializer(resource, options = {})
"Please pass 'adapter: false' or see ActiveSupport::SerializableResource#serialize"
options[:adapter] = false
end

if ActiveModel::Serializer.enabled_adapters_by_media_type && !options.fetch(:adapter, nil)
adapter = ActiveModel::Serializer::Adapter.by_request(request)
options[:adapter] ||= adapter if adapter
end
Copy link
Member

Choose a reason for hiding this comment

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

Would be really great to merge #1017 first to take advantage of the register/get interface cc @joaomdmoura :)

Copy link
Member

Choose a reason for hiding this comment

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

I'll check #1017


ActiveModel::SerializableResource.serialize(resource, options) do |serializable_resource|
if serializable_resource.serializer?
serializable_resource.serialization_scope ||= serialization_scope
Expand Down
8 changes: 8 additions & 0 deletions lib/active_model/serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,14 @@ def self.adapter
adapter_class
end

def self.custom_media_type_adapters
config.custom_media_type_adapters
end

def self.enabled_adapters_by_media_type
config.enabled_adapters_by_media_type
end

def self.root_name
name.demodulize.underscore.sub(/_serializer$/, '') if name
end
Expand Down
12 changes: 12 additions & 0 deletions lib/active_model/serializer/adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ class Adapter

attr_reader :serializer

MEDIA_TYPE_ADAPTERS = {
"application/vnd.api+json" => "json_api",
"application/json" => "flatten_json"
}
Copy link
Member

Choose a reason for hiding this comment

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

👍


def initialize(serializer, options = {})
@serializer = serializer
@options = options
Expand All @@ -37,6 +42,13 @@ def self.adapter_class(adapter)
"ActiveModel::Serializer::Adapter::#{adapter_name}".safe_constantize
end

def self.by_request(request)
return unless request && request.accept
custom = ActiveModel::Serializer.custom_media_type_adapters
media_type_adapters = MEDIA_TYPE_ADAPTERS.merge(custom)
media_type_adapters[request.accept]
end

def fragment_cache(*args)
raise NotImplementedError, 'This is an abstract method. Should be implemented at the concrete adapter.'
end
Expand Down
2 changes: 2 additions & 0 deletions lib/active_model/serializer/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ module Configuration
included do |base|
base.config.array_serializer = ActiveModel::Serializer::ArraySerializer
base.config.adapter = :flatten_json
base.config.enabled_adapters_by_media_type = true
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be false by default, as the defined serializers may not play well with all possible adapters.

base.config.custom_media_type_adapters = {}
end
end
end
Expand Down
26 changes: 26 additions & 0 deletions test/action_controller/adapter_selector_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ def render_skipping_adapter
@profile = Profile.new({ name: 'Name 1', description: 'Description 1', comments: 'Comments 1' })
render json: @profile, adapter: false
end

def render_selecting_adapter_by_accept_header_field
@profile = Profile.new({ name: 'Name 1', description: 'Description 1', comments: 'Comments 1' })
render json: @profile
end
end

tests AdapterSelectorTestController
Expand Down Expand Up @@ -48,6 +53,27 @@ def test_render_skipping_adapter
get :render_skipping_adapter
assert_equal '{"attributes":{"name":"Name 1","description":"Description 1","comments":"Comments 1"}}', response.body
end

def test_render_using_adapter_selected_by_accept_header_field
ActiveModel::Serializer.config.enabled_adapters_by_media_type = true
request.headers['Accept'] = "application/vnd.api+json"

get :render_selecting_adapter_by_accept_header_field

expected = {
data: {
id: assigns(:profile).id.to_s,
type: "profiles",
attributes: {
name: "Name 1",
description: "Description 1",
}
}
}

assert_equal expected.to_json, response.body
end
Copy link
Member

Choose a reason for hiding this comment

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

Can you add soem tests covering the override of the media type as well? maybe one using multiple media type declarations too.


end
end
end