active_model_serializers icon indicating copy to clipboard operation
active_model_serializers copied to clipboard

Namespace ignored when fetching serializer for parent class

Open Charizard opened this issue 7 years ago • 7 comments
trafficstars

Consider an STI case with only parent class serializers and no separate child serializers. Now throw in different namespaces.

With model's:

class Parent < ActiveRecord::Base
end

class ChildA < Parent
end

class ChildB < Parent
end

Controllers & Serializers with namespaces looks like: app/controllers/v1/parents_controller.rb app/serializers/v1/parents_serializer.rb

Expected behavior vs actual behavior

Now when I render child records's from my V1::ParentsController controller, no serializer is being used (ActiveModel::Serializer::Null).

I've tracked down the issue to get_serializer_for method in ActiveModel::Serializer. When recursively calling get_serializer_for for super_class. Here, the namespace needs to be passed in as well.

Environment

Rails: 5 AMS Version: 1.10-stable

Additonal helpful information

Seems to be introduced in https://github.com/rails-api/active_model_serializers/commit/b29395b0ac160c2ee28cc333467954eaafd917bc

Charizard avatar Oct 16 '18 10:10 Charizard

Do I understand you're saying that the V1 namespace isn't being used?

Thanks for digging into the code.

You don't include your render code. Have you tried specifying namespace or serializer? I get it would be nice if the namespace lookup worked as expected, but is there anything which makes specifying it inconvenient?

bf4 avatar Oct 16 '18 15:10 bf4

@bf4 Yes the namespace is not respected even though the render is called from a V1 namespaced controller.

I tried specifying namespace but it's the same. But, I've been bypassing this by specifying the serializer.

Charizard avatar Oct 17 '18 11:10 Charizard

Did you try the fix you suggested?

bf4 avatar Oct 17 '18 14:10 bf4

@bf4 You mean the fix in AMS itself? No, not yet.

I can try it out and make a PR with tests over the weekend.

Charizard avatar Oct 18 '18 05:10 Charizard

@Charizard yeah, fork it, change it, point your gemfile to it, try it.

(Or clone it to a relative dir and point Gemfile to it, same story)

Or just patch it

bf4 avatar Oct 18 '18 19:10 bf4

related I think https://github.com/rails-api/active_model_serializers/issues/2295

bf4 avatar Nov 01 '18 17:11 bf4

@bf4 Yes, it's partly related. When the namespace is explicitly specified like so,

class ResourceNamespace
  class PostSerializer < ActiveModel::Serializer
    has_many :comments, namespace: ResourceNamespace
 end
  class CommentSerializer     < ActiveModel::Serializer; end
end

then #2295 takes care of it. But, in cases where the namespace is not specified like so,

class ResourceNamespace
  class PostSerializer < ActiveModel::Serializer
    has_many :comments
 end
  class CommentSerializer     < ActiveModel::Serializer; end
end

don't you think comments should implicitly pickup ResourceNamespace::CommentSerializer?

The lookup chain for this would be,

  1. ResourceNamespace::PostSerializer::CommentSerializer
  2. <namespace>::CommentSerializer (if a namespace is specified)
  3. ResourceNamespace::CommentSerializer (new item in the lookup chain that I'm proposing) - This would use the namespace of the parent serializer to check for any compatible serializers in it.
  4. CommentSerializer

I've added a failing test here for my proposal.

Charizard avatar Jan 03 '19 03:01 Charizard