Include namespace when looking up superclass

[dpogue]

Purpose

When falling back to the superclass for finding a serializer, the namespace was getting dropped.

Changes

Pass the namespace through when finding a serializer for the superclass.

[NullVoxPopuli]

can you provide a test demonstrating your scenario?
as-is, superclass should include the namespace:

irb(main):001:0> class A; end
=> nil
irb(main):003:0> class A::C; end
=> nil
irb(main):004:0> class D < A::C; end
=> nil
irb(main):005:0> D.superclass
=> A::C

also, is this a problem that could be solved by:

ActiveModelSerializers.config.serializer_lookup_chain.unshift(
  lambda do |resource_class, _, namespace|
    "#{namespace.name}::#{resource_class.name}Serializer" if namespace
  end
)

[dpogue]

It was an issue with the controller namespace not getting passed up to the superclass.

# Superclass model
class User < ActiveRecord::Base
end

# Subclass model
class AppUser < User
end

# Non-namespaced superclass serializer (Wrong)
class UserSerializer < ActiveModel::Serializer
end

# Namespaced superclass serializer (Correct)
class Admin::UserSerializer < ActiveModel::Serializer
end

# Controller
class Admin::UsersController < ApplicationController
  def show
    render :json => AppUser.first
  end
end

When Admin::UsersController tried to render a AppUser, the lookup chains were like this:

["Admin::AppUserSerializer", "AppUserSerializer", "::AppUserSerializer"]
["UserSerializer", "::UserSerializer"]

With this patch, it correctly preserves the controller namespace when searching for the superclass:

["Admin::AppUserSerializer", "AppUserSerializer", "::AppUserSerializer"]
["Admin::UserSerializer", "UserSerializer", "::UserSerializer"]

[NullVoxPopuli]

What happens when you try using the config change to the lookup chain?

[dpogue]

The config change to the lookup chain does not resolve the issue, because it is still trying to use a nil namespace for the superclass.

[NullVoxPopuli]

Can you change it to:

ActiveModelSerializers.config.serializer_lookup_chain.unshift(
  lambda do |resource_class, iforgotwhatthisis, namespace|
    puts '-----------------------------------------------'
    puts resource_class
    puts iforgotwhatthisis
    puts namespace


    "#{namespace.name}::#{resource_class.name}Serializer" if namespace
  end
)

and then restart your server, try to load your resource, and copy the console output to here?
This'll help, cause I actually don't remember which namespace that is in the lookups.

[dpogue]

-----------------------------------------------
AppUser
ActiveModel::Serializer
Admin
-----------------------------------------------
User
ActiveModel::Serializer

[NullVoxPopuli]

So then, this should solve your use case?

A modification of

ActiveModelSerializers.config.serializer_lookup_chain.unshift(
  lambda do |resource_class, serializer_class, namespace|
    "#{namespace.name}::#{resource_class.name}Serializer" if namespace
  end

cause namespace is Admin it the first execution, and 'resource_class' is AppUser, so putting them together would be Admin::AppUserSerializer

So, I guess the root of this is that you want to use a Serializer based off the superclass of the model rather than specifically add namespacing support.

So then, you could add another lookup chain entry that did something like this:

ActiveModelSerializers.config.serializer_lookup_chain.unshift(
  lambda do |resource_class, serializer_class, namespace|
    if namespace && resource_class.superclass != ActiveRecord::Base # or whatever your top level class is
      "#{namespace.name}::#{resource_class.superclass.name}Serializer"
    end
  end

does that work?

[NullVoxPopuli]

It seems we need another PR that fixes the tests on master, as at least one of them seems unrelated to this PR.

I 'think' I agree that this PR is valuable, though. the namespace should def continue getting passed through the recursion.

[dpogue]

#2185 makes the same change, but includes tests, so that would probably be a better one to merge

[dpogue]

I have cherry-picked the commit from #2185 that includes tests, and rebased it on 0-10-stable, as requested in #2242.

This PR now supersedes both #2242 and #2185. The cherry-picked commit is attributed to all the authors of all 3 PRs.

[wasifhossain]

@bf4 you might have had some reason to close a similar PR. can you please share the point with us?
but if you have changed your mind any way, I would like to merge it.

@dpogue can you please leave a changelog entry under the Fixes section. thanks

[dpogue]

Changelog entry added :)

[wasifhossain]

tests are looking good 👌 let's give it a go! also have an eye open for any regression in case