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

Fix #56

Closed
Closed

Conversation

casperbrike
Copy link

ActiveModel::Serializer.serializer_for(resource, ams_options)) called even if options[:serializer] exists. This causes some strange behavior.
Here is example.
Lets assume we have model User and serializer UserSerializer in module V1 (model is in no module, serializer is in serializers/v1/user_serializer.rb file).
And we have simple api for user:

get '/', serializer: V1::UserSerializer do
  user = User.find 1
  user
end

In that case, Grape-AMS will trying to load UserSerializer (on the assumption of model name). AMS can find file user_serializer.rb, but he can't get const UserSerializer in this file, because in this file we have V1::UserSerializer const, so we get error 'Unable to autoload constant UserSerializer, expected /.../serializers/v1/user_serializer.rb to define it'.
But we already specified that we want V1::UserSerializer.
If we specify some other serializer, V1::OtherSerializer or just WithoutModuleSerializer, we will get this error again.

@dblock
Copy link
Member

dblock commented Jul 13, 2016

This needs tests, CHANGELOG entry, a better commit message, etc., please. The build has to pass too.

@drn
Copy link
Member

drn commented Jul 13, 2016

FWIW, I believe this will be fixed by this pull if you are running into this with ASM v0.10.0+
v0.10.x ASM ActiveModel::Serializer.serializer_for no longer handles a passed in namespace key, so we have to handle namespacing within grape-active_model_serializers.

@drn
Copy link
Member

drn commented Jul 15, 2016

@casperbrike can you check your project against master?

@drn
Copy link
Member

drn commented Jul 22, 2016

@casperbrike - this should be fixed on master. please test this against master and create another pull request or issue if this isn't resolved

@drn drn closed this Jul 22, 2016
@casperbrike
Copy link
Author

@drn I tested against master and got the same error, but against namespace-inferred-serializer everything works fine. I think, this is no need create another pull request

@drn
Copy link
Member

drn commented Jul 25, 2016

Thanks @casperbrike - the collection handling in the namespace-inferred-serializer branch will be merged with #61 sometime today. I forgot to include it with my previous push to master. Appreciate you following up!

@drn
Copy link
Member

drn commented Jul 26, 2016

@casperbrike - #61 has been merged - let me know if you have any issues with it

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.

3 participants