Namespaced serializer lookup for associations #1196

beauby · 2015-09-24T03:57:01Z

Given the following situation:

module MyNamespace
  class MyModel < ActiveRecord::Base
    has_many :my_related_models
  end
  class MyRelatedModel < ActiveRecord::Base
    ...
  end
end

module MyNamespace
  class MyModelSerializer < ActiveModel::Serializer
    has_many :my_related_models
    ...
  end
  class MyRelatedModelSerializer < ActiveModel::Serializer
    ...
  end
end

AMS currently has troubles inferring the right serializer class for MyRelatedModel (it should be MyNamespace::MyRelatedModelSerializer, but it only looks up MyRelatedModelSerializer). This PR fixes that by keeping trace of the "root" serializer, that is the first non-ArraySerializer serializer in the chain of nested serializers, and looking up for a serializer in that "root serializer"'s namespace. Especially cool nor polymorphic associations in the namespaced scenario.

Solves stuff such as #886, #916, #879 but without having to manually specify the namespace.

Based on #1193 because it sort of paved the way.

[TODO] Add some tests.

NullVoxPopuli · 2015-09-24T10:19:02Z

lib/active_model/serializer.rb

@@ -108,20 +110,29 @@ def self.digest_caller_file(caller_line)
      Digest::MD5.hexdigest(serializer_file_contents)
    end

-    def self.get_serializer_for(klass)
+    def self.get_serializer_for(klass, parent_serializer)


can you add some yardoc to various parts of this method? explaining what each section does, and what scenarios will be applicable to each block of code?

NullVoxPopuli · 2015-09-24T10:20:31Z

I like this overall, but here is a scenario, which I'd like your opinion on:

rather than MyNamespace::MyModel correlating to MyNamespace::MyModelSerializer, If someone wanted to use a different set of serializers for different versions of their API, like desiring API::V1::MyModelsController to use API::V1::MyModelSerializer, do you think it would be too complicated to support the namespace lookup on both the model and the controller? (always falling back to MyModelSerializer, of course)

Back when I was at my previous company, the lack of controller namespacing was the one thing I didn't like about AMS. ha :-) (there was a requirement to have a versioned API, which it turned out ended up not being all that important :-\ )

But anyway, just wanted your thoughts on controller namespacing as well - but maybe that should be in an RFC

beauby · 2015-09-24T14:40:30Z

@NullVoxPopuli Just added the controller namespace lookup.

NullVoxPopuli · 2015-09-24T14:45:06Z

lib/active_model/serializer.rb

+          serializer_class ||= namespaced_serializer_class_name.safe_constantize
+        elsif controller
+          controller_namespaced_serializer_class_name = "#{controller.class.name.deconstantize}::#{serializer_class_name}"
+          serializer_class ||= controller_namespaced_serializer_class_name.safe_constantize


what if this serializer isn't found? like, what if someone wants to use a straight up PostSerializer in /blog/id/posts?

tests for this would be great :-)

That's why I use ||=. What this piece does is look for a RelatedSerializer serializer in:

CurrentSerializer::RelatedSerializer (which would be ActiveModel::Serializer::RelatedSerializer if it is not actually a related serializer, but that would fail so it's ok)

revert to My::Name::Space::RelatedSerializer if called from My::Name::Space::CurrentSerializer

or revert to My::Controller::Name::Space::RelatedSerializer if called from My::Controller::Name::Space::SomeController

finally, revert to ::RelatedSerializer if none of the above exist

This part could clearly be improved though. Suggestions?

oh ok, I thought it was going the other way (if RelatedSerializer isn't found, try the namespaced version)

in any case, tests for this new logic would be stellar.

beauby · 2015-09-24T15:56:58Z

lib/active_model/serializer.rb

+      chain = []
+
+      # Look for a serializer nested inside the current serializer first, if inside a user-defined serializer
+      chain.push("#{self}::#{serializer_class_name}") if self.class != ActiveModel::Serializer


That kind of check makes me believe it might be time to make serializer_for an instance method?

It's been around for a while, so I'd rather deprecate it

NullVoxPopuli · 2015-09-24T16:07:11Z

thanks for the docs!

beauby · 2015-09-24T16:17:51Z

Tests fail because of the lack of lazy enumerators in ruby 1.9.3.

bf4 · 2015-09-25T00:32:57Z

lib/action_controller/serialization.rb

@@ -25,7 +25,8 @@ def get_serializer(resource, options = {})
          "Please pass 'adapter: false' or see ActiveSupport::SerializableResource.new"
        options[:adapter] = false
      end
-      serializable_resource = ActiveModel::SerializableResource.new(resource, options)
+      serializable_options = options.merge(controller: self)


why not mutate?

also why change options when not serializing

and do we need the whole controller? I'd rather not and looks like you just want the class name

Currently we just need the class name but there's virtually 0 overhead to getting the instance, so why not?

If you make it easy to use the controller directly, people will, and the scope will creep and and new ams will gain responsibilities it shouldn't gave and cause pain and bugs to users and maintainers

Failure of Interface segregation, not keepig it narrow

In my opinion, the real issue lies in the fact that we miss a layer of separation between the serializer definition and the serializer instances.

itsmechlark · 2015-10-02T02:39:58Z

+1

bf4 · 2015-10-02T02:47:21Z

CHANGELOG.md

@@ -16,7 +16,9 @@ Features:
 - [#1172](https://github.com/rails-api/active_model_serializers/pull/1172) Better serializer registration, get more than just the first module (@bf4)
 - [#1158](https://github.com/rails-api/active_model_serializers/pull/1158) Add support for wildcards in `include` option (@beauby)
 - [#1127](https://github.com/rails-api/active_model_serializers/pull/1127) Add support for nested
-    associations for JSON and Attributes adapters via the `include` option (@NullVoxPopuli, @beauby).
+    associations for JSON and Attributes adapters via the `include` option (@NullVoxPopuli, @beauby)
+- [#1193](https://github.com/rails-api/active_model_serializers/pull/1193) Add support for inline nested serializers (@beauby)


This PR is based on #1193, that's why.

bf4 · 2015-10-02T03:21:12Z

Example test case

module ActiveModel
  class Serializer
    class SerializerForTest < Minitest::Test
      class Tweet < Model; end
      class Share < Model; end
      class TweetSerializer < ActiveModel::Serializer
        attributes :id, :body, :date
        has_many :shares
        class ShareSerializer < ActiveModel::Serializer
          attributes :id, :platform, :date
        end
      end
      class ShareSerializer < ActiveModel::Serializer
        attributes :id, :platform, :date
      end

      def test_serializer_for_namespaced_serializers
        tweet = Tweet.new
        tweet.shares << share = Share.new
        # called from the has_many :shares on TweetSerializer 
        # when passed a tweet with shares, which returns an ArraySerializer
        # which uses the TweetSerializer::ShareSerializer on each share
        assert_equal(TweetSerializer.serializer_for(share), TweetSerializer::ShareSerializer)
        assert_equal(Serializer.serializer_for(share), ShareSerializer)
        # assert_equal(HasManyReflection.new(:shares, {}).build_association(tweet_serializer, tweet_serializer.instance_options), TweetSerializer::ShareSerializer)
      end

      class ActiveModel::Serializer

        def self.serializer_for(resource)
          serializers_cache.fetch_or_store(klass) do
            serializer_class_name = "#{klass.name}Serializer"
            tries = 0
            serializer_base_class = self
            serializer_class = nil
            begin
              serializer = serializer_base_class.constants.find {|constant| constant.to_s == serializer_class_name}
              # serializer_class = serializer_class_name.safe_constantize # or whatever
              if serializer
                serializer_class = const_get(:"#{serializer}")
              elsif tries < 1
                tries = 1
                serializer_base_class = Serializer # or some better logic to figure out the nesting, like the amazing `klass.to_s.split('::')[0..-2].join('::').constantize` or using nesting...
                retry # just being lazy for now
              end
            end

            if serializer_class
              serializer_class
            elsif klass.superclass
              get_serializer_for(klass.superclass)
            else
              nil
            end
          end
          class Reflection

            def build_association(subject, parent_serializer_options)
            association_value = subject.send(name)
            # ? association_value = subject.public_send(name)
            # ? association_value = subject.read_attribute_for_serialization(name)
            reflection_options = options.dup
            # serializer_class = ActiveModel::Serializer.serializer_for(association_value, reflection_options)
            serializer_class = subject.class.serializer_for(association_value, reflection_options)
            # ... etc
          end

          end

        end             
      end

    end
  end
end

drafted, not tested

bf4 · 2015-10-02T04:04:48Z

Closed by #1029 ?

beauby · 2015-10-05T05:37:15Z

Closing this in favor of #1225/#1226.

bf4 · 2016-01-15T00:07:53Z

ref: #1436

beauby added 9 commits September 23, 2015 05:32

Add support for implicit nested serializers.

6e3be30

Add tests for implicit nested serializers.

36cbc51

Minor style fixes.

1896039

Add more tests.

b4ebcb7

Fix test.

95607f6

Fix namespaced lookup for ruby < 2.0.1

3773fa3

Add changelog.

34ba9f6

Namespaced serializer lookup for relationships.

96834b6

Add changelog.

2b6a390

beauby added C: Feature V: 0.10.x S: Needs Code Review labels Sep 24, 2015

NullVoxPopuli reviewed Sep 24, 2015
View reviewed changes

Controller-namespaced lookup for serializers.

f924a19

NullVoxPopuli reviewed Sep 24, 2015
View reviewed changes

Refactor serializer lookup.

3b914ca

beauby reviewed Sep 24, 2015
View reviewed changes

bf4 reviewed Sep 25, 2015
View reviewed changes

bf4 reviewed Oct 2, 2015
View reviewed changes

beauby closed this Oct 5, 2015

groyoh mentioned this pull request Nov 30, 2015

Update travis-ci file runtastic/active_model_serializers#17

Merged

beauby mentioned this pull request Jan 15, 2016

Controller namespace lookup. #1436

Closed

olegantonyan mentioned this pull request Jan 24, 2016

Namespacing serializers as namespaced models gives undefined method `all' #1463

Closed

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

Namespaced serializer lookup for associations #1196

Namespaced serializer lookup for associations #1196

beauby commented Sep 24, 2015

NullVoxPopuli Sep 24, 2015

NullVoxPopuli commented Sep 24, 2015

beauby commented Sep 24, 2015

NullVoxPopuli Sep 24, 2015

beauby Sep 24, 2015

beauby Sep 24, 2015

NullVoxPopuli Sep 24, 2015

beauby Sep 24, 2015

bf4 Oct 2, 2015

NullVoxPopuli commented Sep 24, 2015

beauby commented Sep 24, 2015

bf4 Sep 25, 2015

bf4 Sep 25, 2015

bf4 Sep 25, 2015

beauby Sep 25, 2015

bf4 Sep 25, 2015

beauby Sep 27, 2015

itsmechlark commented Oct 2, 2015

bf4 Oct 2, 2015

beauby Oct 2, 2015

bf4 commented Oct 2, 2015

bf4 commented Oct 2, 2015

beauby commented Oct 5, 2015

bf4 commented Jan 15, 2016

Namespaced serializer lookup for associations #1196

Namespaced serializer lookup for associations #1196

Conversation

beauby commented Sep 24, 2015

Choose a reason for hiding this comment

NullVoxPopuli commented Sep 24, 2015

beauby commented Sep 24, 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

NullVoxPopuli commented Sep 24, 2015

beauby commented Sep 24, 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

itsmechlark commented Oct 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Oct 2, 2015

bf4 commented Oct 2, 2015

beauby commented Oct 5, 2015

bf4 commented Jan 15, 2016