Skip to content

(WIP) Fix Serializer Lookup Chain for Namespaces #1883

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

(WIP) Fix Serializer Lookup Chain for Namespaces #1883

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd3d501
Fix Serializer Lookup Chain for Namespaces
thewatts Aug 16, 2016
9412a19
Extend Serializer lookup for multiple namespaces
thewatts Aug 16, 2016
7cff115
Tidy dynamic namespace lookup for serializers
thewatts Aug 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions lib/active_model/serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,41 @@ class << self

# @api private
def self.serializer_lookup_chain_for(klass)
chain = []
lookup_chain = []

resource_class_name = klass.name.demodulize
resource_namespace = klass.name.deconstantize
resource_class_name = klass.name.demodulize
resource_namespace = klass.name.deconstantize
serializer_class_name = "#{resource_class_name}Serializer"

chain.push("#{name}::#{serializer_class_name}") if self != ActiveModel::Serializer
chain.push("#{resource_namespace}::#{serializer_class_name}")
# Loops over a namespaced Serializer's name to add multiple
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of comment should be above the method definition, no?

# namespaced serializer candidates for lookup in the `lookup_chain` from
# the "parent" serializer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the kind of documentation I'd like to see all over AMS :-)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's a tough balance - solid documentation can be a bonus - but it's the first thing to become stale unless tightly monitored.

Typically I try to have the code be self documenting (so to speak), but at this point we're deep into the belly of the beast :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, everyone's goal is self-documenting code. But as long as churn is low, documentation is great to have. I don't see serializer lookup changing very often :-)

#
# Example:
# if `name.to_s` returns "Api::V1::Admin::UserSerializer" (parent serializer),
# and `klass` is Post,
# this would make the lookup `lookup_chain`:
# [
# "Api::V1::Admin::PostSerializer",
# "Api::V1::PostSerializer",
# "Api::PostSerializer",
# ]
parent_namespace = name.to_s

chain
if self != ActiveModel::Serializer
until parent_namespace.empty?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting there! Why not use parent namespace.each?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think using .each would turn this in to too much work. Haha

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could def use .each if we split the string on :: - but we'd have to build the namespaces a little differently.

I'm game for either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way you have it may be the cleanest.
Just trying to think of a way it could possibly throw an exception.. (I haven't thought of one so far)

lookup_chain.push("#{parent_namespace}::#{serializer_class_name}")
parent_namespace = parent_namespace.deconstantize
end
end

# And finally - this adds the last namespaced candidate based on the
# provided klass, returning the full chain.
#
# Examples:
# - Post -> "::PostSerializer"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these backwards? the top level one should be at the bottom, yeah?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want th ordered by longest namespace first, right?

Otherwise if a non-namespaced item exists, ex: CommentSerializer, it would be found before the namespaced version, Api::CommentSerializer (which definitely isn't what we want if the parent is itself namespaced)

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Aug 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we represent these in array form (with more than two, so we can be sure we're talking about the same direction)
Do you mean:

Api::V2::PostSerializer
Api::PostSerializer
PostSerializer

choosing Post Serializer if the top don't exist?

?

Or are you saying this more has to do with the namespace on the model? (I actually think namespacing in controllers vs models is why this gets punted :( )

So, for an Namespaced::Post object:

Api::V2::Namespaced::PostSerializer
Api::Namespaced::PostSerializer
Namespaced::PostSerializer
PostSerializer? # should this one even be here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the top example - but for me, this specifically broke when referencing a relationship from inside a namespaced serializer.

Ex: belongs_to :trait.

In that case, the klass being sent through the chain building method is never namespaced.

Is there another instance top of mind where this method gets used? Is it when being used from inside a controller?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently, controller namespace is a big one.

Like api/v2/namespace/posts_controller.

can you add tests?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no prob on those tests - but as a heads up - it'll be a little while before I can submit them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. The pace of AMS isn't anything speedy, and as far as i know, this feature isn't pressing.

# - Api::Post -> "Api::PostSerializer"
lookup_chain.push("#{resource_namespace}::#{serializer_class_name}")
end

# Used to cache serializer name => serializer class
Expand Down