search

ruby-grape / grape-active_model_serializers

User active_model_serializers with Grape
MIT License
140 stars 68 forks source link

Fix #56

Closed casperbrike closed 8 years ago

casperbrike commented 8 years ago

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 commented 8 years ago

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

drn commented 8 years ago

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 commented 8 years ago

@casperbrike can you check your project against master?

drn commented 8 years ago

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

casperbrike commented 8 years ago

@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 commented 8 years ago

Thanks @casperbrike - the collection handling in the namespace-inferred-serializer branch will be merged with https://github.com/ruby-grape/grape-active_model_serializers/pull/61 sometime today. I forgot to include it with my previous push to master. Appreciate you following up!

drn commented 8 years ago

@casperbrike - https://github.com/ruby-grape/grape-active_model_serializers/pull/61 has been merged - let me know if you have any issues with it