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

Introduces per-request adapter switching based on the request mime. #2122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rescribet
Copy link

@rescribet rescribet commented May 2, 2017

Purpose

The current behaviour is to always use the same adapter, this is not very desirable in more formal hypermedia conforming systems where content-negotiation determines the format which should be used. This PR introduces the option to let the system choose the adapter based on the negotiation results.

Changes

Content-type determination is left to ActionController::MimeResponds, so that users won't have to specify the adapter in every render call.

Caveats

  • This implementation uses a non-existent virtual adapter :mime to let the subsystem know to look up the adapter dynamically, which may be considered inconsistent.
  • By using MimeResponds, an implicit dependency between mime types and serializers is created, which might not be suitable for projects needing to use different adapters under the same mime type.
  • I'm not aware how these changes will affect view caching.

Related GitHub issues

@remear's comment on PR #1082 was the cause to reimplement this feature.
#1039

@mention-bot
Copy link

@fletcher91, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bf4, @domitian and @bacarini to be potential reviewers.

@rescribet rescribet force-pushed the mime-based-adapter-switching branch from 1309f22 to c7bf9aa Compare May 2, 2017 15:28
Copy link
Member

@bf4 bf4 left a comment

Choose a reason for hiding this comment

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

IMO, only the documentation here and some tests are required since request.format is rails-speak for the negotiation mime type, which, for JSON:API is format.jsonapi, which uses render jsonapi: model

Please feel free to disagree and point out anything I'm not considering

# In controller action
respond_to do |format|
format.json { render json: Model.first } # Uses the JSON adapter
format.json_api { render json: Model.first } # Users the JSON:API adapter
Copy link
Member

Choose a reason for hiding this comment

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

👍

should be jsonapi not json_api, and render jsonapi: model rather than render json: model.

But really, this is just using the content type negotiation that respond_to already does. This would work without any changes to the PR, assuming mime -> adapter is 1:1

Copy link
Author

Choose a reason for hiding this comment

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

Would you prefer just a correction:

To automatically switch between adapters based on the request, set the adapter to :mime:

  # In configuration
  ActiveModelSerializers.config.adapter = :mime
  
  # In controller action
  respond_to do |format|
    format.json { render json: Model.first }     # Uses the JSON adapter
    format.jsonapi { render jsonapi: Model.first } # Users the JSON:API adapter
    format.bug { render json: Model.first } # Will raise `UnknownAdapterError`
  end

or to just leave it implicit:

To automatically switch between adapters based on the request, set the adapter to :mime;

ActiveModelSerializers.config.adapter = :mime

Copy link
Member

@bf4 bf4 May 4, 2017

Choose a reason for hiding this comment

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

Really, if you want to render different mime types, each mime type should be registered to its own formatter. That way, you can do the below, which is the Rails-way IMHO and requires no changes to the codebase.

 # In configuration
ActiveModelSerializers.config.adapter = :json
  
# In controller action
respond_to do |format|
    format.json { render json: model, serializer: ModelSerializer }     # Uses the JSON adapter
    format.jsonapi { render jsonapi: model, serializer: ModelSerializer } # Users the JSON:API adapter
    format.bug { render json: model, serializer: ModelSerializer } # Will raise something about bug not being a known mime type
  end

The respond_to block does the content type negotiation for you and usually we use a renderer format with the same name as the negotiated / acceptable mime types.

I think I'm making different assumptions than you, so please continue to discuss.

@rescribet rescribet force-pushed the mime-based-adapter-switching branch from c7bf9aa to fdc7a9a Compare May 2, 2017 19:14
@rescribet rescribet force-pushed the mime-based-adapter-switching branch from fdc7a9a to a1a78ab Compare May 2, 2017 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants