Add support for nested serializers #1225

beauby · 2015-10-02T14:23:03Z

This is half of #1193. This PR brings lookup for serializer classes in different namespaces (namely that of the parent serializer, that of the resource), so that the following is possible:

class PostSerializer < AMS
  has_many :comments
  class CommentSerializer < AMS
    ...
  end
end

(this is a way to make single-purpose nested serializers contained in one class instead of polluting the global namespace and having to call them PostCommentSerializer).

Cases not handled yet:

looking up for a serializer in the root namespace of a chain of serializers (post has_many comments belongs_to author: when a Post is serialized with a Namespace::PostSerializer, we might want to look for Namespace::AuthorSerializer),
looking up for a serializer in the controller's namespace (when Namespace::PostController serializes a Post, we might want to look for Namespace::PostSerializer).

NullVoxPopuli · 2015-10-02T17:08:13Z

lib/active_model/serializer.rb

@@ -112,10 +112,25 @@ def self.digest_caller_file(caller_line)
      Digest::MD5.hexdigest(serializer_file_contents)
    end

+    # TODO(beauby): Cache this.
+    def self.serializer_lookup_chain_for(klass)


I like this extraction.

some documentation on how this works (when to use, expected input/output, etc) would be fantabulous

Looks pretty much self-explanatory to me. What kind of comment would you add that would not be direct paraphrasing of the code?

me too, but I would like to prefer to over document than to under document. Like, look at how rails documents everything with plenty of examples: https://github.com/rails/rails/blob/master/activemodel/lib/active_model/errors.rb

(I guess that is more or less direct paraphrasing in some cases though) :-\

Yeah but serializer_lookup_chain_for is part of the private API.

ref: https://github.com/rails-api/active_model_serializers/pull/1225/files#r41045890

bf4 · 2015-10-02T17:20:37Z

test/serializers/serializer_for_test.rb

+        def test_serializer_for_nested_resource
+          comment = ResourceNamespace::Comment.new
+          serializer = ResourceNamespace::PostSerializer.serializer_for(comment)
+          assert_equal(ResourceNamespace::PostSerializer::CommentSerializer, serializer)


what's missing from the tests is when ResourceNamespace::PostSerializer.serializer_for is called. the tests don't confirm this is hooked up anywhere

I don't understand. ResourceNamespace::PostSerializer.serializer_for is called just above?

What I see is:

ActiveModel::Serializer.serializer_for(post) #=> ResourceNamespace::PostSerializer ResourceNamespace::PostSerializer.serializer_for(comment) #=> ResourceNamespace::PostSerializer::CommentSerializer

which tests serialization from an AM::S subclass, but not that the subclass is ever used.. maybe my comment just means there should be another test somewhere else

We need to add a test to test/serializers/associations_test.rb to make sure the corresponding serializer gets used. This will be doable in a decent way once the test infrastructure refactor has been done.

Added tests.

bf4 · 2015-10-02T17:33:18Z

Nice and slim! good encapsulation!

beauby · 2015-10-04T21:57:18Z

lib/active_model/serializer.rb

+      chain.push(serializer_class_name)
+
+      chain
+    end


@bf4 So if you were referring to this part of code saying it was not easily understandable, then I agree. Suggestions? I could add comments, but I don't see how to make the code itself clearer.

beauby · 2015-10-07T16:20:22Z

Squashed.

bf4 · 2015-10-08T16:08:49Z

lib/active_model/serializer/array_serializer.rb

-            ActiveModel::Serializer.serializer_for(resource)
-          end
+          serializer_context_class = options.fetch(:serializer_context_class, ActiveModel::Serializer)
+          serializer_class = options.fetch(:serializer) { serializer_context_class.serializer_for(resource) }


Potential 🐛 serializer_context_class is a required option. Would you mind adding a comment to the initializer to that effect? and if it's absent, it'll throw a KeyError. How do we want to handle that?

Maybe just say "don't use ArraySerializer directly, Just use SerializableResource" :)

The previous line(serializer_context_class = options.fetch(:serializer_context_class, ActiveModel::Serializer)) ensures it is not required. Am I misunderstanding?

nope, I'm 👊 (facepalm?) oops never mind me

bf4 · 2015-10-08T17:04:50Z

lib/active_model/serializer.rb

+      chain
+    end
+
+    # @api private


giving us the license to change it :) 👍

beauby · 2015-10-08T22:41:32Z

The build failed on appveyor, not sure why as I can't consult the logs.

beauby · 2015-10-08T22:54:51Z

Actually, I have the answer:

Build execution time has reached the maximum allowed time for your plan (40 minutes).

bf4 · 2015-10-09T00:48:49Z

Yikes. I'll write a timeout max for ci

B mobile phone

On Oct 8, 2015, at 5:54 PM, Lucas Hosseini notifications@github.com wrote:

Actually, I have the answer:

Build execution time has reached the maximum allowed time for your plan (40 minutes).

—
Reply to this email directly or view it on GitHub.

bf4 · 2015-10-09T03:10:44Z

Yikes. I'll write a timeout max for ci

hmm, I don't really know how to do around hooks Minitest without hacking it

bf4 · 2015-10-09T03:14:50Z

oh weird, it succeeded build success, just never exited, it appears

bf4 · 2015-10-09T03:16:23Z

try git commit -m 'rebuild appveyour' --allow-empty; git push -f to force it to rebuild?

NullVoxPopuli · 2015-10-09T13:57:44Z

👍 LGTM

Add support for nested serializers

@beauby

Adds support for finding serializers based on the "current" serializers namespace to support things like serializer versioning. Without this you have to be explicit in naming which serializer to use in all of your serializer associations. That's OK usually (albeit annoying) but it breaks polymorphism since you can't really specify the right serializer ahead of time. I came across a bunch of PRs such as rails-api#1225 that talk about different versions of namespacing based on modules or controllers or whatever and some of them even talk about supporting serializer namespaces but I have been unable to find one that actually does this part of it. If there is already work being done for this, feel free to ignore this PR but we needed this for our current work. This PR builds on top of the work done by @beauby in rails-api#1225 to add support for namespaced serializer lookups. @beauby's work only allowed for actual nesting of serializers withing a namespaced serializer (e.g. for overrides) but did not actually walk up the tree to siblings or serializers farther up the inheritance/namespace tree. It simply modifies `Serializer#serializer_lookup_chain_for` to add all of the possible levels of namespaces based on the current serializer. With this patch for example if we have: ```ruby class Public::V1::PostSerializer belongs_to :author end class Public::V1::AuthorSerializer end ``` we will wind up with the following items in the lookup chain for an author when serializing a Post with Public::V1::PostSerializer: ```ruby ['Public::V1::PostSerializer::AuthorSerializer', 'Public::V1::AuthorSerializer', 'Public::AuthorSerializer', '::AuthorSerializer'] ```

@beauby

Adds support for finding serializers based on the "current" serializers namespace to support things like serializer versioning. Without this you have to be explicit in naming which serializer to use in all of your serializer associations. That's OK usually (albeit annoying) but it breaks polymorphism since you can't really specify the right serializer ahead of time. I came across a bunch of PRs such as rails-api#1225 that talk about different versions of namespacing based on modules or controllers or whatever and some of them even talk about supporting serializer namespaces but I have been unable to find one that actually does this part of it. If there is already work being done for this, feel free to ignore this PR but we needed this for our current work. This PR builds on top of the work done by @beauby in rails-api#1225 to add support for namespaced serializer lookups. @beauby's work only allowed for actual nesting of serializers withing a namespaced serializer (e.g. for overrides) but did not actually walk up the tree to siblings or serializers farther up the inheritance/namespace tree. It simply modifies `Serializer#serializer_lookup_chain_for` to add all of the possible levels of namespaces based on the current serializer. With this patch for example if we have: ```ruby class Public::V1::PostSerializer belongs_to :author end class Public::V1::AuthorSerializer end ``` we will wind up with the following items in the lookup chain for an author when serializing a Post with Public::V1::PostSerializer: ```ruby ['Public::V1::PostSerializer::AuthorSerializer', 'Public::V1::AuthorSerializer', 'Public::AuthorSerializer', '::AuthorSerializer'] ```

@beauby

Adds support for finding serializers based on the "current" serializers namespace to support things like serializer versioning. Without this you have to be explicit in naming which serializer to use in all of your serializer associations. That's OK usually (albeit annoying) but it breaks polymorphism since you can't really specify the right serializer ahead of time. I came across a bunch of PRs such as rails-api#1225 that talk about different versions of namespacing based on modules or controllers or whatever and some of them even talk about supporting serializer namespaces but I have been unable to find one that actually does this part of it. If there is already work being done for this, feel free to ignore this PR but we needed this for our current work. This PR builds on top of the work done by @beauby in rails-api#1225 to add support for namespaced serializer lookups. @beauby's work only allowed for actual nesting of serializers withing a namespaced serializer (e.g. for overrides) but did not actually walk up the tree to siblings or serializers farther up the inheritance/namespace tree. It simply modifies `Serializer#serializer_lookup_chain_for` to add all of the possible levels of namespaces based on the current serializer. With this patch for example if we have: ```ruby class Public::V1::PostSerializer belongs_to :author end class Public::V1::AuthorSerializer end ``` we will wind up with the following items in the lookup chain for an author when serializing a Post with Public::V1::PostSerializer: ```ruby ['Public::V1::PostSerializer::AuthorSerializer', 'Public::V1::AuthorSerializer', 'Public::AuthorSerializer', '::AuthorSerializer'] ```

Match current API endpoints (/links, /characters) There is a pull request that is currently active (within the last day) for Active Model Serializers that will allow for nested and inline Serializers. This will allow `CardSerializer` to go from the complicated monstrosity that it is to a much simpler representation. For example ``` has_one :character ``` instead of: ``` def character Api::V1::CharacterSerializer.new(object.character) end ``` follow this pull request at rails-api/active_model_serializers#1225 See merge request !1

beauby force-pushed the nested-serializer-lookup branch 3 times, most recently from 450744a to 9da2061 Compare October 2, 2015 14:44

beauby changed the title ~~Add support for nested and namespaced serializers~~ [WIP] Add support for nested and namespaced serializers Oct 2, 2015

beauby changed the title ~~[WIP] Add support for nested and namespaced serializers~~ Add support for nested and namespaced serializers Oct 2, 2015

beauby mentioned this pull request Oct 2, 2015

Add support for inline serializers #1226

Closed

beauby added C: Feature V: 0.10.x labels Oct 2, 2015

NullVoxPopuli reviewed Oct 2, 2015
View reviewed changes

beauby force-pushed the nested-serializer-lookup branch 2 times, most recently from b47d5e7 to 745cd82 Compare October 2, 2015 17:15

bf4 reviewed Oct 2, 2015
View reviewed changes

bf4 added the S: Needs Submitter Response label Oct 2, 2015

beauby force-pushed the nested-serializer-lookup branch from 223e597 to 8692e94 Compare October 3, 2015 02:50

beauby mentioned this pull request Oct 4, 2015

Inline nested serializers #1193

Closed

beauby reviewed Oct 4, 2015
View reviewed changes

This was referenced Oct 5, 2015

Namespaced serializer lookup for associations #1196

Closed

Nested serializers/attributes #1190

Closed

beauby removed the S: Needs Submitter Response label Oct 5, 2015

beauby added this to the 0.10 milestone Oct 5, 2015

beauby self-assigned this Oct 5, 2015

beauby force-pushed the nested-serializer-lookup branch from 96d4b22 to d913dbf Compare October 7, 2015 16:19

beauby force-pushed the nested-serializer-lookup branch from d913dbf to de2c834 Compare October 7, 2015 22:08

bf4 reviewed Oct 8, 2015
View reviewed changes

Extend serializer lookup to the parent serializer.

9147469

beauby force-pushed the nested-serializer-lookup branch from 46d0915 to 9147469 Compare October 8, 2015 16:23

beauby changed the title ~~Add support for nested and namespaced serializers~~ Add support for nested serializers Oct 8, 2015

bf4 reviewed Oct 8, 2015
View reviewed changes

lib/active_model/serializer.rb

chain

end

# @api private

Copy link

Member

bf4 Oct 8, 2015

Choose a reason for hiding this comment

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

giving us the license to change it :) 👍

beauby force-pushed the nested-serializer-lookup branch 2 times, most recently from 67c72e7 to 9147469 Compare October 9, 2015 13:57

NullVoxPopuli added a commit that referenced this pull request Oct 9, 2015

Merge pull request #1225 from beauby/nested-serializer-lookup

6018ef1

Add support for nested serializers

NullVoxPopuli merged commit 6018ef1 into rails-api:master Oct 9, 2015

bf4 mentioned this pull request Oct 12, 2015

Serialization and Cache Documentation #1260

Merged

bf4 deleted the nested-serializer-lookup branch October 12, 2015 00:57

beauby mentioned this pull request Oct 29, 2015

default serializer selection should scope to namespace? #1306

Closed

bdmac mentioned this pull request Nov 20, 2015

Namespaced serializers #1338

Closed

bf4 mentioned this pull request Feb 10, 2016

Added serializer namespacing option #879

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for nested serializers #1225

Add support for nested serializers #1225

beauby commented Oct 2, 2015

NullVoxPopuli Oct 2, 2015

NullVoxPopuli Oct 2, 2015

beauby Oct 2, 2015

NullVoxPopuli Oct 2, 2015

NullVoxPopuli Oct 2, 2015

beauby Oct 2, 2015

NullVoxPopuli Oct 2, 2015

bf4 Oct 2, 2015

bf4 Oct 2, 2015

beauby Oct 2, 2015

bf4 Oct 2, 2015

beauby Oct 3, 2015

beauby Oct 3, 2015

beauby Oct 4, 2015

bf4 commented Oct 2, 2015

beauby Oct 4, 2015

beauby commented Oct 7, 2015

bf4 Oct 8, 2015

beauby Oct 8, 2015

bf4 Oct 8, 2015

bf4 Oct 8, 2015

beauby commented Oct 8, 2015

beauby commented Oct 8, 2015

bf4 commented Oct 9, 2015

bf4 commented Oct 9, 2015

bf4 commented Oct 9, 2015

bf4 commented Oct 9, 2015

NullVoxPopuli commented Oct 9, 2015

Add support for nested serializers #1225

Add support for nested serializers #1225

Conversation

beauby commented Oct 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Oct 2, 2015

Choose a reason for hiding this comment

beauby commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Oct 8, 2015

beauby commented Oct 8, 2015

bf4 commented Oct 9, 2015

bf4 commented Oct 9, 2015

bf4 commented Oct 9, 2015

bf4 commented Oct 9, 2015

NullVoxPopuli commented Oct 9, 2015