configurable serializer_lookup option #1757

NullVoxPopuli · 2016-05-30T13:24:44Z

Purpose

people need to be able to customize how the serializer look up is performed. Especially when it comes to API versioning, as there are quite a few ways to do serializer lookup and determining what way to map api version to serializers.

Changes

Adds a config option for adding additional serializer lookups.

Caveats

When configuring this, the dev can easily break everything.

Related GitHub issues

Additional helpful information

Things left to do:

Tests
- Test for changing the Proc to have a different name matching scheme
- Test to demonstrate taking the version from a request header
Benchmarks vs master
Documentation

bf4 · 2016-05-31T00:56:25Z

lib/active_model/serializer/configuration.rb

+            lookups << '::' + current_namespace + '::' + resource_serializer
+          end
+
+          lookups


This should be in serialization_context, maybe?.

It's also pretty easy to solve with explicit code in the controller. then just pass in serializer: resource_serializer (or in index, each_serializer: resource_serializer) and you're good to go

def resource_serializer instance_variable_get("@#{resource_name}_serializer") || self.resource_serializer = "#{self.class.parent}::#{resource_class.name}Serializer".constantize end def resource_serializer=(new_resource_serializer) instance_variable_set("@#{resource_name}_serializer", new_resource_serializer) end # The resource class based on the controller # @return [Class] def resource_class @resource_class ||= resource_name.classify.constantize end # The singular name for the resource class based on the controller # @return [String] def resource_name @resource_name ||= controller_name.singularize end

Is that what we want to recommend to people?

I think it's pretty straightforward to do this, but maybe it could be documented somewhere? So people who ask can work with something? Idk

The other thing I was thinking, is to Lee this option nil, so existing behavior remains the same, and provide the option?

But yeah, I'm on the fence on if ams should even provide the capability to configure the lookup.

Maybe, if rails pushes more API centric usage, it would wake more sense?

You guys could have a look at #1745. There is a discussion around the same idea (lookup based on controller). Maybe we can come up with a consensus on this.

oh!, @bf4 that serializer lookup defined in the controller wouldn't keep relationships under the same namespace, would it? I think that would be harder to debug than the proc here

NullVoxPopuli · 2016-09-07T19:34:51Z

needs rebase.

Is there a direction we want to go with this?
I think having serializer lookup being configurable would be a good way to help out people who have weird namespacing situations.

NullVoxPopuli · 2016-09-07T19:35:06Z

@rails-api/ams ^

NullVoxPopuli · 2016-09-09T14:45:45Z

rebased

NullVoxPopuli · 2016-09-09T15:21:50Z

@bf4 @richmolj I'd like to revisit this, and get your thoughts on it.

What is present in this PR, is the change to support using a proc for serializer lookup and an example proc.
Obviously, the final version of this PR would have the proc be empty (if we wanted it that way).

I am in favor of the either of the following

Move serializer lookup entirely into the proc

or

have the proc be empty, keeping current / default lookup. The proc would only provide additional lookups, and the default would not be able to be changed (just prepended to the custom)

For both scenarios:

Provide tests that show procs and the common situations we get asked about with customizing the lookup chain. (API versioning, API fallback, etc)

richmolj · 2016-09-09T15:33:11Z

@NullVoxPopuli after reading this comment of yours, I'm sold.

Move serializer lookup entirely into the proc

I'd favor a slight modification to this, moving the serializer lookup into a collection of procs. Something like:

ActiveModelSerializers.config.serializer_lookup_chain << proc_to_check_object_instance_method
ActiveModelSerializers.config.serializer_lookup_chain << proc_for_automatic_namespaced_lookup
ActiveModelSerializers.config.serializer_lookup_chain << proc_for_automatic_lookup

That way our internals match the externals (simpler), and users can easily re-order or add new ones that would take precedence. So basically middleware, where the first one to return a match wins.

bf4 · 2016-09-09T16:09:46Z

lib/active_model/serializer/concerns/configuration.rb

+        # "::ResourceSerializer"]
+        config.serializer_lookup = proc { |controller_name|
+          lookups = []
+          current_resource = controller_name.gsub(/Controller\z/, '')


controller_name isn't correct. It's more lookup_namespace and then you want to lookup_namespace.remove(/Controller\z/) because you know someone might pass in a controller?

that's true. I'll switch it to lookup_namespace, and mention in a comment that it can and often is the controller name, cause of https://github.com/rails-api/active_model_serializers/pull/1757/files#diff-5624f9cf35be4dc254412d979fd293c4R63 klass is a controller (I guess, most of the time)

NullVoxPopuli · 2016-09-09T17:02:17Z

moving the serializer lookup into a collection of procs.

@richmolj: I think I could get on board with that -- but we need a place to store the procs that makes sense.

I looking at the existing code, I think it only makes sense to have the following as 'default':

  module AMS::SerializerLookup
     DEFAULT = -> (lookup_namespace) {
        chain = []

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

      # Example: 
      # BlogSerializer::AuthorSerializer
      chain.push("#{name}::#{serializer_class_name}") if self != ActiveModel::Serializer
      # Example: 
      # Api::AuthorSerializer
      chain.push("#{resource_namespace}::#{serializer_class_name}")

      chain
     }    
end

I don't think it makes sense to split this up, because both chain.push lines need serializer_class_name

then in serializer.rb:

    def self.serializer_lookup_chain_for(klass)
      ActiveModelSerializers.config.serializer_lookup_chain.map { |p| p.call(klass) }.flatten
    end

NullVoxPopuli · 2016-09-09T17:04:22Z

I just realized, that these procs are going to need the serializer class name to be passed to them as well (esp for the case of defining nested serializers)

richmolj · 2016-09-09T17:14:36Z

My thinking was something like

chain.push ->(klass) { klass.serializer if klass.respond_to?(:serializer) }
chain.push ->(klass) { "#{klass.name}Serializer".safe_constantize }
chain.push ->(klass) { "#{klass.name.demodulize}Serializer".safe_constantize }

Even better, pass an instance instead of the class and do the same.

But I'm splitting hairs here. As long as this is a customizable array of procs being passed an instance to be serialized, or the class of that instance, I'm 👍

NullVoxPopuli · 2016-09-09T17:35:07Z

I think it the interface would have to be:

String, Class
-> (lookup_namespace, serializer_class) { ... }

Because, serializer_lookup_chain_for only gets passed the controller class (not the instance). I also thinking passing the instance would invite too much coupling.

But, with the above change to the proc API, the procs could be:

# Downside to splitting this up so much, is that there is some duplicate code
# Like creating the serializer name string from since it's going to be the suffix
# of everything
# This could be a third parameter so it's not calculated all the time.
# however, this proc would then have 3 parameters, and I don't know how I feel about that.
module AMS::SerializerLookup
   DEFAULT = [
      IN_PARENT_SERIALIZER
      BY_NAMESPACE
   ] 

  IN_PARENT_SERIALIZER = -> (lookup_namespace, serializer_class) {
    # logic for getting the parent::child serializer name
  }

  BY_NAMESPACE = -> (lookup_namespace, serializer_class) {
    # logic for getting a serializer of the same namespace as lookup_namespace
  }
end

# serializer.rb
    def self.serializer_lookup_chain_for(klass)
      ActiveModelSerializers.config.serializer_lookup_chain.map { |p| 
        p.call(klass, self) 
      }.flatten.compact
    end

Config Somewhere:

# ( already default (in configuration.rb), so this would be redundant )
AMS.config.serializer_lookup_chain = AMS::SerializerLookup::DEFAULT
# or 
AMS.config.serializer_lookup_chain << whatever_proc
# or
AMS.config.serializer_lookup_chain = []
AMS.config.serializer_lookup_chain << some_proc

richmolj · 2016-09-09T17:56:29Z

Because, serializer_lookup_chain_for only gets passed the controller class (not the instance).

Right, but I think we could change this line pretty easily. However, you note the performance impact of string instantiation which I agree with. It looks like the result is cached so it's not a performance issue if we pass the class, but it would be if we passed the instance.

So I can get behind the idea of passing the class, instead of the instance, for that reason. My only thought with the instance was you could pick the serializer at runtime based on a property or something.

I'm not totally sure why lookup_namespace is needed in addition to the class though.

richmolj · 2016-09-09T18:00:40Z

Oh hold on I think I may have found the source of my confusion. The goal is to based the lookup on the controller namespace, not the object's namespace. That makes sense now. My preference would be to pass the controller instance itself, instead of just its namespace, but either way.

NullVoxPopuli · 2016-09-09T18:02:23Z

I'll get some tests written for this, this weekend to demonstrate the different scenarios I can think of where this could be useful.

richmolj · 2016-09-09T18:52:08Z

After thinking about it a little bit more, I think you were right. lookup_namespace and serializer_klass should be the arguments passed to the procs. Then use the caching in serializer_lookup_chain_for to avoid a performance penalty.

This is a great idea. I don't have this use case yet, but I expect to eventually.

NullVoxPopuli · 2016-09-09T19:20:38Z

lib/active_model/serializer.rb


+      lookups = ActiveModelSerializers.config.serializer_lookup_chain


@richmolj this looks nice - It's already chached, see line 86

NullVoxPopuli · 2016-09-09T20:06:47Z

At the very least all this is finally getting documented

NullVoxPopuli · 2016-09-20T00:07:44Z

@remear do you have opinions on this?

NullVoxPopuli · 2016-09-21T21:30:16Z

test/feature/configuration/serializer_lookup/using_serializable_resource_test.rb

+  describe 'configuration' do
+    describe 'serializer lookup' do
+      describe 'using SerializableResource' do
+        it 'uses the child serializer' do


more tests need to exist in this file such that it can be the example of possible lookup chain scenarios.

Including:

Versioned APIs (both via URL and request header)

Versioned APIs with fallback to earlier version

Mimicing the controller's namespace.

this'll be todo, as we'd just be testing different ways to configure -- which is open ended

groyoh · 2016-11-15T10:05:29Z

@NullVoxPopuli have a look at https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/collection_serializer.rb#L74. We do not pass the namespace options when initializing the serializers meaning that if we have:

module Api
  module V1
    class PrimaryResourceSerializer < ActiveModel::Serializer
      attributes :title, :body
      has_many :has_many_relationships
    end
    class HasManyRelationshipSerializer < ActiveModel::Serializer
      attribute :body
    end
  end
end

The has_many lookup chain will be will be

[
  "AmsBench::Api::V1::PrimaryResourceSerializer::HasManyRelationshipSerializer",   
  "::HasManyRelationshipSerializer",
  "HasManyRelationshipSerializer"
]

instead of

[
  "AmsBench::Api::V1::PrimaryResourceSerializer::HasManyRelationshipSerializer",   
  "AmsBench::Api::V1::HasManyRelationshipSerializer",
  "HasManyRelationshipSerializer"
]

Pretty sure that's a bug but don't have time right now to look further into it. Will check again later. Also not sure if it belongs to this PR either or a different bug PR.

NullVoxPopuli · 2016-11-15T10:15:40Z

Oh, collections. Kk. Good find. I can make a PR to fix soon

On Tue, Nov 15, 2016, 5:05 AM Yohan Robert [email protected] wrote:

@NullVoxPopuli https://github.com/NullVoxPopuli have a look at
https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/collection_serializer.rb#L74.
We do not pass the namespace options when initializing the serializers
meaning that if we have:

module Api
module V1
class PrimaryResourceSerializer < ActiveModel::Serializer
attributes :title, :body
has_many :has_many_relationships
end
class HasManyRelationshipSerializer < ActiveModel::Serializer
attribute :body
end
endend

The has_many lookup chain will be will be

[
"AmsBench::Api::V1::PrimaryResourceSerializer::HasManyRelationshipSerializer",
"::HasManyRelationshipSerializer",
"HasManyRelationshipSerializer"
]

instead of

[
"AmsBench::Api::V1::PrimaryResourceSerializer::HasManyRelationshipSerializer",
"AmsBench::Api::V1::HasManyRelationshipSerializer",
"HasManyRelationshipSerializer"
]

Pretty sure that's a bug but don't have time right now to look further
into it. Will check again later. Also not sure if it belongs to this PR
either or a different bug PR.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1757 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMJaoUs76jVIsSY03KwRgD0x2a_Yh33ks5q-YPrgaJpZM4Ipy62
.

groyoh · 2016-11-15T13:17:04Z

Once #1973 is merged, I'll add my benchmark changes/results. Otherwise the benchmark won't be accurate.

NullVoxPopuli · 2016-11-15T13:17:59Z

cool. I'm just waiting for / watching travis

NullVoxPopuli · 2016-11-15T13:43:41Z

@groyoh #1973 is merged

groyoh · 2016-11-15T20:32:05Z

This is the final results:

{
  "label": "Configurable Lookup Chain",
  "version": "0.10.2",
  "rails_version": "4.2.7.1",
  "iterations_per_second": 2104.3974604253326,
  "iterations_per_second_standard_deviation": 2.280937936044573,
  "total_allocated_objects_per_iteration": 593
}
{
  "label": "Old Lookup Chain (v0.10)",
  "version": "0.10.2",
  "rails_version": "4.2.7.1",
  "iterations_per_second": 2097.130687654142,
  "iterations_per_second_standard_deviation": 6.055893452308528,
  "total_allocated_objects_per_iteration": 593
}

groyoh · 2016-11-15T20:41:52Z

I just figured out there is something strange with the benchmark. I need to test it further.

NullVoxPopuli · 2016-11-15T21:23:22Z

bro. rubocop

NullVoxPopuli · 2016-11-15T21:25:53Z

I wish I could 👍 a commit

NullVoxPopuli · 2016-11-15T21:26:24Z

wait, so the old lookup chain is slower than the new configurable one? with the same objects? wat

NullVoxPopuli · 2016-11-15T21:28:12Z

anywho, that benchmark looks good to me 👍

beauby · 2016-11-15T21:31:32Z

lib/active_model/serializer/concerns/configuration.rb

+        #
+        # If CustomNamespace::ResourceSerializer exists, it will be used
+        # for serialization
+        config.serializer_lookup_chain = ActiveModelSerializers::LookupChain::DEFAULT.dup


Why the .dup?

DEFAULT is a frozen array, and we want mutatability?

DEFAULT should be frozen, but are there real use cases for injecting stuff in the lookup chain rather than replacing it altogether?

I'm not entirely sure on that one.

Do we get a performance boost if serializer_lookup_chain is frozen? I wouldn't think it matters, since config is a singletonish thing

groyoh · 2016-11-15T23:36:57Z

@NullVoxPopuli turns out the whole serializer lookup is cached (https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer.rb#L97) so we cannot see any difference once it's cached...in the end the benchmark I added does not actually says much.

NullVoxPopuli · 2016-11-16T02:05:53Z

Excellent

On Tue, Nov 15, 2016, 6:37 PM Yohan Robert [email protected] wrote:

@NullVoxPopuli https://github.com/NullVoxPopuli turns out the whole
serializer lookup is cached (
https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer.rb#L97)
so we cannot see any difference once it's cached...in the end the benchmark
I added does not actually says much.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1757 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMJagqJWxSoaBVFjlLZCTQ50AELgZo5ks5q-kIbgaJpZM4Ipy62
.

NullVoxPopuli · 2016-11-16T12:54:32Z

any objections to merging?

groyoh · 2016-11-16T16:45:13Z

Yup, sorry gotta push a last commit. Will do it in 5min (and I will make sure I run rubocop 😁 ).

…chmark

groyoh · 2016-11-16T16:55:13Z

So the end result is:

if I have this code

configurable.call
old.call

{
  "label": "Configurable Lookup Chain",
  "version": "0.10.2",
  "rails_version": "4.2.7.1",
  "iterations_per_second": 2097.0843979297792,
  "iterations_per_second_standard_deviation": 2.6226889129638966,
  "total_allocated_objects_per_iteration": 593
}
{
  "label": "Old Lookup Chain (v0.10)",
  "version": "0.10.2",
  "rails_version": "4.2.7.1",
  "iterations_per_second": 2140.142899144878,
  "iterations_per_second_standard_deviation": 3.831519850041984,
  "total_allocated_objects_per_iteration": 593
}

if I have this code

old.call
configurable.call

{
  "label": "Old Lookup Chain (v0.10)",
  "version": "0.10.2",
  "rails_version": "4.2.7.1",
  "iterations_per_second": 2103.7895845497637,
  "iterations_per_second_standard_deviation": 2.8044629763972666,
  "total_allocated_objects_per_iteration": 593
}
{
  "label": "Configurable Lookup Chain",
  "version": "0.10.2",
  "rails_version": "4.2.7.1",
  "iterations_per_second": 2177.0852661503927,
  "iterations_per_second_standard_deviation": 1.3779892072416242,
  "total_allocated_objects_per_iteration": 593
}

So it looks that the one running last is running faster (?). But looking at both result I'd say the difference is not significant.

NullVoxPopuli · 2016-11-16T16:56:43Z

cool. Running the old one actually would invalidate the results of the configurable lookup chain, because of the in-bench monkey patch.

But excellent! once travis passes, lets merge.

groyoh · 2016-11-16T16:58:06Z

So I'll merge once Travis give the go and no one objects!

groyoh · 2016-11-16T17:00:23Z

@NullVoxPopuli in my last commit I make sure to clear the cache before each benchmark. That way we ensure that the lookup chain method is actually called and that both benchmark are similar.

NullVoxPopuli added V: 0.10.x S: Needs Team Discussion C: Namespacing/Versioning C: Config labels May 30, 2016

bf4 reviewed May 31, 2016
View reviewed changes

NullVoxPopuli mentioned this pull request Jul 11, 2016

[RFC] Automatic serializer lookup #1442

Open

NullVoxPopuli mentioned this pull request Aug 16, 2016

(WIP) Fix Serializer Lookup Chain for Namespaces #1883

Open

NullVoxPopuli force-pushed the serializer-lookup branch from 51a0091 to c1a1858 Compare September 9, 2016 14:45

bf4 reviewed Sep 9, 2016
View reviewed changes

NullVoxPopuli reviewed Sep 9, 2016
View reviewed changes

richmolj mentioned this pull request Sep 21, 2016

Question: Wrap Serializer in CollectionSerializer? #1934

Closed

NullVoxPopuli commented Sep 21, 2016

View reviewed changes

bf4 mentioned this pull request Oct 31, 2016

AMS should error when no serializer can be found #1853

Open

Merge remote-tracking branch 'rails-api/master' into serializer-lookup

9aeace8

groyoh mentioned this pull request Nov 15, 2016

Fix namespace lookup for collections and has_many #1973

Merged

NullVoxPopuli mentioned this pull request Nov 15, 2016

add test and fix for collection not supporting namespaces #1974

Closed

Update benchmark to benchmark whole serialization

9202deb

Fix rubocop errors

1c318dc

beauby reviewed Nov 15, 2016

View reviewed changes

Make sure serializers cache is cleaned before running each lookup ben…

143c004

…chmark

groyoh merged commit d31d741 into rails-api:master Nov 16, 2016

NullVoxPopuli deleted the serializer-lookup branch January 31, 2017 13:18

GregPK pushed a commit to GregPK/active_model_serializers that referenced this pull request Apr 25, 2017

Make serializer lookup configurable (rails-api#1757)

716447b

benlangfeld mentioned this pull request Oct 14, 2021

Namespace serializers for consuming controller rubyforgood/Flaredown#538

Merged


		lookups = ActiveModelSerializers.config.serializer_lookup_chain

configurable serializer_lookup option #1757

configurable serializer_lookup option #1757

Conversation

NullVoxPopuli commented May 30, 2016 • edited Loading

Purpose

Changes

Caveats

Related GitHub issues

Additional helpful information

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 7, 2016

NullVoxPopuli commented Sep 7, 2016

NullVoxPopuli commented Sep 9, 2016

NullVoxPopuli commented Sep 9, 2016

richmolj commented Sep 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Sep 9, 2016 • edited Loading

NullVoxPopuli commented Sep 9, 2016

richmolj commented Sep 9, 2016

NullVoxPopuli commented Sep 9, 2016 • edited Loading

richmolj commented Sep 9, 2016

richmolj commented Sep 9, 2016

NullVoxPopuli commented Sep 9, 2016

richmolj commented Sep 9, 2016

Choose a reason for hiding this comment

NullVoxPopuli commented Sep 9, 2016

NullVoxPopuli commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groyoh commented Nov 15, 2016

NullVoxPopuli commented Nov 15, 2016

groyoh commented Nov 15, 2016

NullVoxPopuli commented Nov 15, 2016

NullVoxPopuli commented Nov 15, 2016

groyoh commented Nov 15, 2016

groyoh commented Nov 15, 2016

NullVoxPopuli commented Nov 15, 2016

NullVoxPopuli commented Nov 15, 2016

NullVoxPopuli commented Nov 15, 2016

NullVoxPopuli commented Nov 15, 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

groyoh commented Nov 15, 2016

NullVoxPopuli commented Nov 16, 2016

NullVoxPopuli commented Nov 16, 2016

groyoh commented Nov 16, 2016

groyoh commented Nov 16, 2016

NullVoxPopuli commented Nov 16, 2016

groyoh commented Nov 16, 2016

groyoh commented Nov 16, 2016

NullVoxPopuli commented May 30, 2016 •

edited

Loading

NullVoxPopuli commented Sep 9, 2016 •

edited

Loading

NullVoxPopuli commented Sep 9, 2016 •

edited

Loading