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

Overriding adapter #1039

Closed
bolshakov opened this issue Aug 5, 2015 · 10 comments
Closed

Overriding adapter #1039

bolshakov opened this issue Aug 5, 2015 · 10 comments

Comments

@bolshakov
Copy link

I can override adapter for specific action this way

render json: movie, adapter: :json_api

This is not convenient enough if you use your own adapter, since adapter string is converted to adapter class in the ActiveModel namespace:

"ActiveModel::Serializer::Adapter::#{adapter_name}".safe_constantize

My suggestion is to pass adapter as a class instead of symbol, or allow to pass both class an symbol.

@dbwieland18
Copy link

Wondering how you're able to override the adapter as shown above--any other setup required to pass

adapter: :json_api

and have it change adapters? I get a NoMethodError currently when trying it that way.

@bolshakov
Copy link
Author

Yep, it should change adapter. When you are passing :adapter options to render method, it forwards options to ActiveModel::Serializer::Adapter.create method. Look at the code here https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/adapter.rb#L30-L38

AMS changing a lot now, so you may be looking to not latest version.

@joaomdmoura
Copy link
Member

Hey @bolshakov, indeed, this would be an improvement.
You can workaround this by using the same namespace, but that would be quite a hack.
If it's something system-wide you can also use the config.adapter, then you can use a class.

It seems an edge-case for me, but it's a valid scenario, I'll keep it open! 😄

@mrageh
Copy link

mrageh commented Aug 25, 2015

Hi @joaomdmoura and @bolshakov

I am thinking of tackling this issue but I am not sure which approach is better

Pass adapter as a class instead of symbol
Allow to pass both class an symbol.

Secondly if we allow users to pass the class name should it be name spaced under
ActiveModel::Serializer::Adapter::CustomSerializer?

Or should we allow users to just define a class CustomerSerializer and then pass in the constant name?

@bacarini
Copy link
Contributor

@mrageh, there is already something about that on https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer.rb#L92, but it works only if you are not using in your action.

I believe we could improve it, what do you think?

About your question, I would go for defining a class instead of declare the entire path ActiveModel::Serializer::Adapter::CustomSerializer.

@mrageh
Copy link

mrageh commented Aug 26, 2015

So do we want to pass both a class and a symbol to adapter?
Or do we only want to pass the class to adapter?

This is a PR I can finish today but I need guidance on how we want this api to work. So far I know that we won't require users to name space their custom serialisers thanks to @bacarini.

Do we still want to substitute API to Api (.sub("API", "Api")) in their custom serialiser?

Thanks

@sandstrom
Copy link

Would also be very useful to override the adapter within the serializer (i.e. not in the controller), based on scope/user properties.

A common use-case is client-versioning, e.g. the mobile client < Y.x should get data serialized using the adapter-A, and newer clients should use adapter-B.

Changing the adapter on-the-fly must also take caching into consideration (pretty obvious), since the output would differ.

@bf4
Copy link
Member

bf4 commented Aug 30, 2015

ref: #1017

@joaomdmoura
Copy link
Member

@sandstrom that would be nice, but would be kind complex, I think that right now I would let ppl use conditionals inside controllers in order to render a specific Adapter,
there is also some WIP PR on letting AMS to use adapter taking into consideration the Accept http Header, what would also enable you to automatically use multiple adapters.

@mrageh sorry for take so long to reply my last week was crazy and the next one might be as well, but I'm trying to catch up with everything right now.
Answering you question, yup, as class and as symbol.
No need for the whole namespace, the user can define it's own namespace if he/she wants to.

@mrageh
Copy link

mrageh commented Sep 3, 2015

@joaomdmoura I think someone else already created a really nice PR for this issue :)
I'll try to find something else I can do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants