Namespaced serializers #1338

bdmac · 2015-11-20T00:18:57Z

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 #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 #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:

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:

['Public::V1::PostSerializer::AuthorSerializer',
'Public::V1::AuthorSerializer', 'Public::AuthorSerializer',
'::AuthorSerializer']

bf4 · 2015-11-20T21:28:31Z

test/serializers/associations_test.rb

@@ -207,6 +207,49 @@ def test_associations_namespaced_resources
          end
        end
      end
+
+      class NamespaceNestedSerializersTest < Minitest::Test


This snippet from the PR description would also make a great test showing the old behavior and the behavior this PR introduces. It wasn't obvious to me from the examples why you'd want to 'step back' one level in the serializer lookup.

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:

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:

['Public::V1::PostSerializer::AuthorSerializer',
'Public::V1::AuthorSerializer', 'Public::AuthorSerializer',
'::AuthorSerializer']

beauby · 2015-11-22T22:33:31Z

@bdmac I actually did that in #1157, but it didn't get traction back then, so it was never merged. Now I'm not too convinced anymore it would be useful, but if people want it, I'm not opposed to this PR getting merged.

beauby · 2015-11-22T22:35:21Z

lib/active_model/serializer.rb

-      chain.push("#{name}::#{serializer_class_name}") if self != ActiveModel::Serializer
+      if self != ActiveModel::Serializer
+        chain.push("#{name}::#{serializer_class_name}")
+        chain.push(*name.deconstantize.split('::').each_with_object([]) { |element, array| array.push((array.last ? array.last + '::' : '') + element) }.reverse.map { |k| k + "::#{serializer_class_name}" })


A slightly shorter version that leads to the same result:

klass.to_s.split('::').reverse.reduce([]) { |a, e| a + [[e] + Array(a.last)] }.reverse

beauby · 2015-11-22T22:38:32Z

The reason why I'm not sure it's that useful is because the "magic" can get confusing in some cases (think of having nested serializers like PostSerializer::CommentSerializer, AuthorSerializer, CommentSerializer::AuthorSerializer: when you serialize a Post along with its comments, should the authors of the comments be serialized according to AuthorSerializer or CommentSerializer::AuthorSerializer? Is it obvious to users which will be used when adding the whole suffix chain to the lookup chain?).

bdmac · 2015-11-23T02:18:59Z

Totally get that the magic can get confusing but there is not currently a valid solution supporting all uses with namespaced versions. Specifically polymorphic associations. I wouldn't be opposed to something more explicit but it would need to work with controller and non rendering paths.

beauby · 2015-11-23T12:54:14Z

Two possible solutions (for which I issued PRs at some point but they ended up in limbo):

Hit the controller's namespace (will be really easy to do with the whole context thing)
Hit the "root serializer"'s namespace, that is the one that was specified/inferred for the actual render call

bdmac · 2015-11-23T16:05:03Z

Would 1 work outside of a controller render? I can't find much docs on the whole context thing you mentioned anywhere.

The root serializer doesn't allow for any nesting. For example we have Public::V1 with most of our stuff but then a Public::V1::ConversationElement namespace with some polymorphic resources that reference things in the parent namespace that aren't polymorphic

Maybe the root serializer specifies just the highest level in which to look. I was thinking it could be an actual class level setting that you could set on a parent serializer to opt-in to this feature. Something like:

class Public::V1::Serializer < ActiveModel::Serializer
  root_namespace 'Public::V1'
end

class Public::V1::CustomerSerializer < Public::V1::Serializer
end

beauby · 2015-11-24T21:53:15Z

could be made to work outside of a controller render, although one would have to manually specify the root namespace, a bit like what you suggested in the second part of your post, except that it would be an option to SerializableResource's constructor, rather than a class method on ActiveModel::Serializer. I'll try to put something together tonight or tomorrow if I find time. In the meantime, I'd like for other people to chip in to the conversation so that we make sure we're not missing out on an easier solution to your problem.

bf4 · 2015-12-23T05:59:43Z

Are we still working on this?

bf4 · 2016-01-15T06:54:04Z

ref: #1436

@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'] ```

bdmac · 2016-01-22T16:58:14Z

I'm certainly still hopeful to get this in. Would rather not have to keep rebasing my fork/branch. :)

Also open to other working solutions to the original problem if there has been something added that I have missed.

beauby · 2016-01-22T17:00:10Z

@bdmac Totally understandable ;) Would you mind chipping in to the #1442? I think we finally got most people on board with the idea of a stronger automatic lookup, we just have to decide how exactly it should work.

bdmac · 2016-01-22T17:09:54Z

Rebased.

bdmac force-pushed the feature/serializer-namespaces branch from d64adba to b44eee1 Compare November 20, 2015 00:51

bf4 reviewed Nov 20, 2015
View reviewed changes

beauby reviewed Nov 22, 2015
View reviewed changes

bf4 added C: Feature V: 0.10.x S: Needs Submitter Response labels Dec 23, 2015

bdmac mentioned this pull request Jan 22, 2016

[RFC] Automatic serializer lookup #1442

Open

bdmac force-pushed the feature/serializer-namespaces branch from b44eee1 to 2553d68 Compare January 22, 2016 17:09

remear mentioned this pull request Mar 17, 2016

Serializer files were broken by versioning #1597

Closed

remear closed this Mar 28, 2016

NullVoxPopuli mentioned this pull request Aug 16, 2016

(WIP) Fix Serializer Lookup Chain for Namespaces #1883

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Namespaced serializers #1338

Namespaced serializers #1338

Uh oh!

bdmac commented Nov 20, 2015

Uh oh!

bf4 Nov 20, 2015

Uh oh!

beauby commented Nov 22, 2015

Uh oh!

beauby Nov 22, 2015

Uh oh!

beauby commented Nov 22, 2015

Uh oh!

bdmac commented Nov 23, 2015

Uh oh!

beauby commented Nov 23, 2015

Uh oh!

bdmac commented Nov 23, 2015

Uh oh!

beauby commented Nov 24, 2015

Uh oh!

bf4 commented Dec 23, 2015

Uh oh!

bf4 commented Jan 15, 2016

Uh oh!

bdmac commented Jan 22, 2016

Uh oh!

beauby commented Jan 22, 2016

Uh oh!

bdmac commented Jan 22, 2016

Uh oh!

Uh oh!

Namespaced serializers #1338

Namespaced serializers #1338

Uh oh!

Conversation

bdmac commented Nov 20, 2015

Uh oh!

bf4 Nov 20, 2015

Choose a reason for hiding this comment

Uh oh!

beauby commented Nov 22, 2015

Uh oh!

beauby Nov 22, 2015

Choose a reason for hiding this comment

Uh oh!

beauby commented Nov 22, 2015

Uh oh!

bdmac commented Nov 23, 2015

Uh oh!

beauby commented Nov 23, 2015

Uh oh!

bdmac commented Nov 23, 2015

Uh oh!

beauby commented Nov 24, 2015

Uh oh!

bf4 commented Dec 23, 2015

Uh oh!

bf4 commented Jan 15, 2016

Uh oh!

bdmac commented Jan 22, 2016

Uh oh!

beauby commented Jan 22, 2016

Uh oh!

bdmac commented Jan 22, 2016

Uh oh!

Uh oh!