This adds namespace lookup to serializer_for #1968

NullVoxPopuli · 2016-11-08T04:03:41Z

Purpose

To Provide the ability to namespace serializers in the same fashion that controllers are namespaced -- as it is very common to add Api::V# to both controllers and serializers.

Changes

Pass the controllers namespace through serializer_options and serializer_for

Caveats

Namespaced lookup is first, given that namespace is passed in to the serializable resource options.

Related GitHub issues

Subset of the work in #1757

mention-bot · 2016-11-08T04:03:42Z

@NullVoxPopuli, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bf4, @beauby and @groyoh to be potential reviewers.

NullVoxPopuli · 2016-11-08T04:03:52Z

/cc @bf4 @remear

bf4 · 2016-11-08T05:15:12Z

lib/active_model/serializer.rb

      chain = []

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

+      chain.push("#{namespace.name}::#{serializer_class_name}") if namespace


Is namespace.name necessary? Seems it implies namespace is a class, whereas, if it is, when interpolating, Ruby will do the right thing whether it's a string, symbol, or class... i.e just namespace

not necessary. removed.

bf4 · 2016-11-08T05:15:47Z

@NullVoxPopuli what is the test/feature/configuration/serializer_lookup/in_controllers_test.rb in the diff?

bf4 · 2016-11-08T05:17:07Z

lib/action_controller/serialization.rb

@@ -18,6 +18,10 @@ def serialization_scope(scope)
      self._serialization_scope = :current_user
    end

+    def namespace_for_serializer
+      @namespace_for_serializer ||= self.class.parent unless self.class.parent == Object
+    end


do you think we should make this a public interface? Once we add it, we need to support it. (I know I resurrected it, just asking now in context :)

I'm sure people would appreciate it. And I don't think it would be a problem to support, as tons of people want namespaced-scoped serializer lookup -- but maybe that'd be better for the lookup customization PR?

Once we add it, we need to support it.

:)

bf4 · 2016-11-08T05:17:29Z

Where do you think docs should go? Any sense of where people would look for this?

NullVoxPopuli · 2016-11-08T11:48:41Z

@NullVoxPopuli what is the test/feature/configuration/serializer_lookup/in_controllers_test.rb in the diff?

that's a good question -- must have gotten carried over from the serializer-lookup branch -- which I also don't remember editing that file.

NullVoxPopuli · 2016-11-08T11:51:53Z

Where do you think docs should go? Any sense of where people would look for this?

I'd say under rendering -- I'll add something real quick

bf4 · 2016-11-08T17:24:35Z

.travis.yml

@@ -20,7 +20,8 @@ cache:

 script:
  - bundle exec rake ci
-
+after_success:
+  - codeclimate-test-reporter


Is this working?

It doesn't error -- I'm not familiar enough with code climate to see if the results are published per-pull-request. But it works locally

bf4 · 2016-11-08T17:25:48Z

docs/general/rendering.md

+The namespace for serializer lookup is based on the controller, via `ActionController::Serialization#namespace_for_serializer`.
+
+To configure the implicit namespace, in your controller, define
+


replace the implementation of namespace_for_serializer
or pass in the render option :namespace

?

i.e. below is the current implementation. ooh,, this would be a good reason to add a setter

attr_writer :namespace_for_serializer

makes it cleaner and can do in a before filter ?

bf4 · 2016-11-08T17:27:01Z

docs/general/rendering.md

+
+
+```ruby
+render json: @post, namespace: Api::V2


or namespace: 'Api::V2' or namespace: :'Api::V2' but not namespace: :api_v2'

bf4 · 2016-11-08T17:29:13Z

docs/general/rendering.md

+render json: @post, namespace: Api::V2
+```
+
+This tells the serializer lookup to check for the existence of `Api::V2::PostSerializer`, and if any relations are rendered with `@post`, they will also utilize the `Api::V2` namespace.


this would be a better example with @post = Post.first or something to reference the class of the model.

bf4 · 2016-11-08T17:31:00Z

lib/active_model/serializer.rb

      return nil unless config.serializer_lookup_enabled
      serializers_cache.fetch_or_store(klass) do
        # NOTE(beauby): When we drop 1.9.3 support we can lazify the map for perfs.
-        serializer_class = serializer_lookup_chain_for(klass).map(&:safe_constantize).find { |x| x && x < ActiveModel::Serializer }
+        lookup_chain = serializer_lookup_chain_for(klass, namespace)
+        serializer_class = lookup_chain.map(&:safe_constantize).find { |x| x && x < ActiveModel::Serializer }


bf4 · 2016-11-08T17:31:26Z

lib/active_model/serializer/reflection.rb

@@ -106,6 +106,10 @@ def value(serializer, include_slice)
      #
      def build_association(parent_serializer, parent_serializer_options, include_slice = {})
        reflection_options = options.dup
+
+        # Pass the parent's namespace onto the child serializer
+        reflection_options[:namespace] ||= parent_serializer_options[:namespace]


is this not included, oy

nah, without coping the namespace from the parent to the reflection_options, the namespace isn't passed to all the children

bf4 · 2016-11-08T17:32:15Z

lib/active_model_serializers/serializable_resource.rb

@@ -55,7 +55,7 @@ def serializer
      @serializer ||=
        begin
          @serializer = serializer_opts.delete(:serializer)
-          @serializer ||= ActiveModel::Serializer.serializer_for(resource)
+          @serializer ||= ActiveModel::Serializer.serializer_for(resource, serializer_opts)


bf4 · 2016-11-08T17:33:22Z

test/serializers/serializer_for_with_namespace_test.rb

+      class Page < ::Model; end
+      class Publisher < ::Model; end
+
+      module Api


can save space by

module Api; end module Api::V3; end class Api::V3::BookSerializer < ApplicationSerializer # etc.

Decided to keep the expanded syntax for a V2 serializer

bf4 · 2016-11-08T17:34:13Z

test/serializers/serializer_for_with_namespace_test.rb

+      #
+      #   expected = { title: 'A Post', author_name: 'hello' }
+      #   assert_equal expected, result
+      # end


looks about right except for the ::

The problem I was having with this test is that I couldn't get a root level BookSerializer to be used if a namespace wasn't present.

bf4 · 2016-11-08T17:34:45Z

test/serializers/serializer_for_with_namespace_test.rb

@@ -0,0 +1,85 @@
+require 'test_helper'
+
+module ActiveModel


looks good. needs controller test for implicit lookup as well as option passing, which is really covered below, here

tests added

…nd fix the tests for explicit namespace

bf4 · 2016-11-08T21:17:14Z

docs/general/rendering.md

+before_filter :use_my_namespace
+def use_my_namespace
+  self.namespace_for_serializer = Api::V2
+end


might as well just before_action { self.namespace_for_serializer = Api::V2 } (before_filter is such old news :( :))

bf4 · 2016-11-08T21:19:43Z

docs/general/rendering.md

+
+This tells the serializer lookup to check for the existence of `Api::V2::PostSerializer`, and if any relations are rendered with `@post`, they will also utilize the `Api::V2` namespace.  
+
+The namespace can _only_ be in one of the following formats:


Any object whose namespace can be represented by string interpolation (i.e. by callingto_s)

Module: Api::V2

String: 'Api::V2'

Symbol: :'Api::V2'

bf4 · 2016-11-08T21:20:01Z

lib/action_controller/serialization.rb

+    end
+
+    def namespace_for_serializer
+      @namespace_for_serializer ||= self.class.parent unless self.class.parent == Object


bf4 · 2016-11-08T21:21:40Z

lib/active_model/serializer.rb

      return nil unless config.serializer_lookup_enabled
-      serializers_cache.fetch_or_store(klass) do
+      serializers_cache.fetch_or_store("#{klass}-#{namespace}") do


this will look weird in some case.

ActiveSupport::Cache.expand_cache_key(klass, namespace)

also used to be in AMS...

cool, thanks

bf4 · 2016-11-08T21:45:51Z

lib/active_model/serializer.rb

      return nil unless config.serializer_lookup_enabled
-      serializers_cache.fetch_or_store(klass) do
+
+      cache_key = ActiveSupport::Cache.expand_cache_key(klass, namespace)


for now, is ok, but really needs to be in https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/concerns/caching.rb

https://github.com/rails-api/active_model_serializers/blob/v0.10.2/lib/active_model/serializer/caching.rb wow we're behind on releases

cache updates and namespacing / configurable lookup chain sounds like a good set of updates for a release? ;-)

@bf4

* This adds namespace lookup to serializer_for * address rubocop issue * address @bf4's feedback * add docs * update docs, add more tests * apparently rails master doesn't have before filter * try to address serializer cache issue between tests * update cache for serializer lookup to include namespace in the key, and fix the tests for explicit namespace * update docs, and use better cache key creation method * update docs [ci skip] * update docs [ci skip] * add to changelog [ci skip]

This adds namespace lookup to serializer_for

e529466

NullVoxPopuli mentioned this pull request Nov 8, 2016

configurable serializer_lookup option #1757

Merged

5 tasks

address rubocop issue

3c98d48

bf4 reviewed Nov 8, 2016

View reviewed changes

address @bf4's feedback

526738b

add docs

771af73

bf4 reviewed Nov 8, 2016

View reviewed changes

NullVoxPopuli added 4 commits November 8, 2016 13:35

update docs, add more tests

81e743a

apparently rails master doesn't have before filter

44b06a3

try to address serializer cache issue between tests

aaa94af

update cache for serializer lookup to include namespace in the key, a…

c847e63

…nd fix the tests for explicit namespace

bf4 reviewed Nov 8, 2016

View reviewed changes

NullVoxPopuli added 2 commits November 8, 2016 16:24

update docs, and use better cache key creation method

eb630e8

update docs [ci skip]

0fa8196

bf4 reviewed Nov 8, 2016

View reviewed changes

NullVoxPopuli added 2 commits November 8, 2016 16:45

update docs [ci skip]

29d14a8

add to changelog [ci skip]

679ed72

NullVoxPopuli merged commit b29395b into rails-api:master Nov 9, 2016

NullVoxPopuli deleted the controller-namespace-lookup branch November 9, 2016 12:57

thewatts mentioned this pull request Nov 13, 2016

(WIP) Fix Serializer Lookup Chain for Namespaces #1883

Open

		The namespace for serializer lookup is based on the controller, via `ActionController::Serialization#namespace_for_serializer`.

		To configure the implicit namespace, in your controller, define


		This tells the serializer lookup to check for the existence of `Api::V2::PostSerializer`, and if any relations are rendered with `@post`, they will also utilize the `Api::V2` namespace.

		The namespace can _only_ be in one of the following formats:

This adds namespace lookup to serializer_for #1968

This adds namespace lookup to serializer_for #1968

Conversation

NullVoxPopuli commented Nov 8, 2016

Purpose

Changes

Caveats

Related GitHub issues

mention-bot commented Nov 8, 2016

NullVoxPopuli commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Nov 8, 2016

NullVoxPopuli commented Nov 8, 2016

NullVoxPopuli commented Nov 8, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Nov 8, 2016 • edited

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

NullVoxPopuli Nov 8, 2016 •

edited