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

Add support for nested serializers. #1157

Closed
wants to merge 15 commits into from

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Sep 16, 2015

Following discussion on #1113.
This adds support for defining serializers inside other serializers, so that when serializing a nested resource, the most nested serializer is used in priority.

@NullVoxPopuli
Copy link
Contributor

ya have a lot of chained PRs :-p

@@ -9,40 +9,12 @@ def initialize(serializer, options = {})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole file is not required for this PR.

@beauby beauby changed the title [WIP] Add support for nested serializers. Add support for nested serializers. Sep 21, 2015
@beauby beauby self-assigned this Sep 21, 2015
module ActiveModel::Serializer::Utils
module_function

def nested_lookup_paths(klass)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add some yardoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

belongs_to :author
class AuthorSerializer < ActiveModel::Serializer
attributes :id
end
Copy link
Member

Choose a reason for hiding this comment

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

How is this different from belongs_to: :author, serializer: TweetAuthorSerializer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing is that the namespacing makes it so that you don't have to use funky adapter names that you then have to specify manually.

Copy link
Member

Choose a reason for hiding this comment

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

So, this PR is more a syntactic sugar than a new feature?

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 sorta how I see it as well. But for me, the biggest benefit would be to greatly reduce the number of top level serializers that actually have VERY specific purposes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, indeed syntactic sugar. However, I could see in the future having, say the Json adapter default to including all nested relationships (default to ** instead of * currently), in a kind of safe way since the user would provide all the subsequent serializers to make the graph explicit (maybe with a warning when there is an undefined serializer and possibly when using a fallback serializer).

# @param [Object] klass
# @return [Array<Symbol>]
def nested_lookup_paths(klass)
paths = klass.to_s.split('::').reverse.reduce([]) { |a, e| a + [[e] + Array(a.last)] }.reverse
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind helping me read this code?

  1. TweetSerializer::AuthorSerializer.to_s.split('::').reverse #=> ['AuthorSerializer', 'TweetSerializer']
  2. _.reduce([]) {|result, element| result + [[element] + Array(result.last)] }.reverse
    1. paths = []
      1. paths = [] + [['AuthorSerializer'] + Array([]) ] #=> [['AuthorSerializer']]
      2. paths = [['AuthorSerializer']] + [ ['TweetSerializer'] + Array(['AuthorSerializer']) ] #=> [ ["AuthorSerializer"], ["TweetSerializer", "AuthorSerializer"] ]
    2. paths.reverse #=> ["TweetSerializer", "AuthorSerializer"], ["AuthorSerializer"]]
  3. ["TweetSerializer", "AuthorSerializer"], ["AuthorSerializer"]].map! {|path| path.join('::') } #=> ["TweetSerializer::AuthorSerializer", "AuthorSerializer"]
  4. [TweetSerializer::AuthorSerializer", "AuthorSerializer"].select! {|path| Object.const_defined?(path, false) }.map! {|path| Object.const_get(path) }.push(Object) #=> [TweetSerializer::AuthorSerializer, AuthorSerializer, Object]

Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed. I admit that part was slightly cryptic.

Copy link
Member

Choose a reason for hiding this comment

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

So that as the test says

Utils.nested_lookup_paths(TweetSerializer::ShareSerializer::AuthorSerializer) #=> [::TweetSerializer::ShareSerializer::AuthorSerializer, ::ShareSerializer::AuthorSerializer, ::AuthorSerializer, ::Object]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yessir.

Copy link
Member

Choose a reason for hiding this comment

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

And
Utils.nested_lookup([::TweetSerializer, ::Object], 'ShareSerializer') #=> ::TweetSerializer::ShareSerializer ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so.

Copy link
Member

Choose a reason for hiding this comment

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

So that

          @share1 = Share.new
TweetSerializer.serializer_for(@share1) #=> TweetSerializer::ShareSerializer
ActiveModel::Serializer.serializer_for(@share1) #=> ShareSerializer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.

Copy link
Member

Choose a reason for hiding this comment

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

So that you can build associations on tweet.author using serializer: TweetSerializer::AuthorSerializer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly, if available, and safely fallback to any less specialized serializer that can handle an Author.

def nested_lookup_paths(klass)
paths = klass.to_s.split('::').reverse.reduce([]) { |a, e| a + [[e] + Array(a.last)] }.reverse
paths.map! { |path| "#{path.join('::')}" }
paths.map! do |path|
Copy link
Contributor

Choose a reason for hiding this comment

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

@beauby can you add comments after each line, explaining what it does -- so that if debugging of this needs to happen in the future, we don't need to go through the exercise you and @bf4 went through?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll comment the first line, which might be pretty cryptic. The rest is pretty much self-explanatory, no?

@beauby
Copy link
Contributor Author

beauby commented Sep 22, 2015

The tests currently fail because ruby <= 2.0.0 does not allow namespaced constants to be checked via namespace.const_defined?.

This was referenced Sep 22, 2015
@NullVoxPopuli
Copy link
Contributor

how long is < ruby 2.0 supported for?

@beauby
Copy link
Contributor Author

beauby commented Sep 22, 2015

@NullVoxPopuli Official support for 1.9.3 has ended in Feb 2015 (so we could probably drop support for 1.9.3 as well...). However, this issue still arises with ruby 2.0.0... I believe it was fixed in 2.0.1.

@beauby
Copy link
Contributor Author

beauby commented Sep 23, 2015

Closing this in favor of #1193.

@beauby beauby closed this Sep 23, 2015
@joaomdmoura
Copy link
Member

Nice! #1193 looks awesome and way more mature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants