(WIP) Fix Serializer Lookup Chain for Namespaces #1883

thewatts · 2016-08-16T17:21:51Z

Purpose

This fixes dynamic lookup of namespaced serializers through relationships.

Changes

This adjusts the internal API of the serializer method used for dynamic lookup by adding an additional constant string to the lookup chain used to find the related serializer.

Caveats

No tests. I'm a testing guy - so, if someone can point me to where I should be testing this in the project, I will 100% write them.

I'm guessing I'll need to start with test/serializers/associations_test.rb

Also - this is currently only working for a single nested resource, I'll adjust in short order to provide a dynamic amount of namespaced items.

Related GitHub issues

Not sure

Additional helpful information

-- The Problem:

Currently, when using a relationship from within a namespace, ex:

module Api
  class LessonSerializer < ApplicationSerializer
    belongs_to :trait
  end
end

The method to fetch a serializer builds a lookup chain of possible
constant names that gets looped over with safe_constantize. When using
relationships from within a namespace (like above), the lookup chain
would return this:

[
  "Api::LessonSerializer::TraitSerializer",
  "::TraitSerializer",
]

This works fine the first time, as calling .safe_constantize on
"Api::LessonSerializer::TraitSerializer" returns the correct constant
in this case: Api::TraitSerializer.

However, the second time that the lookup has to happen (from a
different relationship), ex:

module Api
  class CourseSerializer < ApplicationSerializer
    belongs_to :trait
  end
end

The lookup chain returns a similar response:

[
  "Api::CourseSerializer::TraitSerializer",
  "::TraitSerializer",
]

But at this point, calling .safe_constantize on
"Api::CourseSerializer::TraitSerializer" returns nil.

Here's an example from the Rails console:

It looks like the main problem lies in this issue here
(many thanks to @willcosgrove for finding it!)

So this means that calling .safe_constantize on the same name string
returns the constant the first time (based on
ActiveSupport::Dependencies), but the second time it gets called on
the same string it returns nil.

-- The Fix:

I get the namespace of the name of the parent serializer, and use that
to also build and add a string to the lookup chain array.

So in our first example above, it adds: "Api::TraitSerializer" to the
lookup chain, allowing the serializer to be found every time.

mention-bot · 2016-08-16T17:21:52Z

@thewatts, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bf4, @beauby and @steveklabnik to be potential reviewers

@willcosgrove

-- The Problem: Currently, when using a relationship from within a namespace, ex: ```ruby module Api class LessonSerializer < ApplicationSerializer belongs_to :trait end end ``` The method to fetch a serializer builds a lookup chain of possible constant names that gets looped over with `safe_constantize`. When using relationships from within a namespace (like above), the lookup chain would return this: ```ruby [ "Api::LessonSerializer::TraitSerializer", "::TraitSerializer", ] ``` This works fine the first time, as calling `.safe_constantize` on `"Api::LessonSerializer::TraitSerializer"` returns the correct constant in this case: `Api::TraitSerializer`. However, the _second_ time that the lookup has to happen (from a different relationship), ex: ```ruby module Api class CourseSerializer < ApplicationSerializer belongs_to :trait end end ``` The lookup chain returns a similar response: ```ruby [ "Api::CourseSerializer::TraitSerializer", "::TraitSerializer", ] ``` But at this point, calling `.safe_constantize` on `"Api::CourseSerializer::TraitSerializer"` returns `nil`. Here's an example from the Rails console: ![safe_constantize example](https://thewatts.s3-us-west-1.amazonaws.com/screenshots/Screen-Shot-2016-08-16-10-45-05.png) It looks like the main problem [lies in this issue here](rails/rails#9286) (many thanks to @willcosgrove for finding it!) So this means that calling `.safe_constantize` on the same name string returns the constant the first time (based on `ActiveSupport::Dependencies`), but the _second_ time it gets called on the same string it returns `nil`. ![safe_constantize of the same string](https://thewatts.s3-us-west-1.amazonaws.com/screenshots/Screen-Shot-2016-08-16-10-53-15.png) -- The Fix: I get the namespace of the `name` of the parent serializer, and use that to also build and add a string to the lookup chain array. So in our first example above, it adds: `"Api::TraitSerializer"` to the lookup chain, allowing the serializer to be found every time.

NullVoxPopuli · 2016-08-16T17:38:20Z

Hey @thewatts thanks for your PR!

There have actually been many approaches to this before:

My favorite: #1757 (bias though)
#1436
#1338
#1295
#1196
#1157

I think that rails issue you linked to comes down to how ruby does constant lookup in an auto-loading environment (rails) http://cirw.in/blog/constant-lookup.html

That said, for tests, I'd would add to the main AM::Serializer tests and add tests for the look up chain method

NullVoxPopuli · 2016-08-16T20:04:23Z

I suppose you'd also wasn't to add integration tests to action controller tests somewhere. Maybe a new file for lookup?

thewatts · 2016-08-16T20:04:30Z

@NullVoxPopuli thanks for the reply!

And sorry for the late reply on my end - that's quite the chain of issues/PRs to follow.
(I'll admit - I skimmed to get the gist).

Honestly, dynamically defining namespace by controller (or through configuration as in your PR) hadn't crossed my mind.

I had just noticed that the dynamic lookup of some belongs_to relationships were failing when two different serializers shared the same relationship resource.

I've looked at making things more dynamic, so that deeper nested items could be found correctly (ie: Api::V1::SuperDuper::TraitSerializer). I've added the commit to this PR.

I can see the desire for configuration though, especially for relationships where you have a V2 serializer that needs to pull a serializer relationship from a V1 type.

Ex:

module Api
  module V1
    class TraitSerializer < ApplicationSerializer
    end
  end
end

module Api
  module V2
    class LessonSerializer < ApplicationSerializer
      belongs_to :trait
    end
  end
end

In that case - Api::V1::TraitSerializer wouldn't be found.

Though my additions create a quick fix (at least to my own problem) - it looks like there's already some solid direction going with those other PRs.

Has there been a consensus on the final direction? If so - I won't be offended if this PR is closed ;)

thewatts · 2016-08-16T20:06:24Z

@NullVoxPopuli if this PR is helpful, I absolutely don't mind adding tests (and of course - fixing the one I just broke 😅 )

NullVoxPopuli · 2016-08-16T23:03:11Z

lib/active_model/serializer.rb

+      tmp_namespace   = name.to_s
+      name_namespaces = [tmp_namespace]
+
+      until tmp_namespace.empty?


do you need to have two loops here? I imagine you could add to the chain array as you deconstantized the namespaces.

Could you also add some documentation (comments) in here explaining how the chain array looks after different steps of this process?

@NullVoxPopuli Good call! Thanks for the feedback!

Adjustments added in most recent commit. Let me know your thoughts!

NullVoxPopuli · 2016-08-16T23:04:00Z

@thewatts I think this is def worth while (it's why I've worked on it a bit). It's also essential for API versioning. (for that, I'd also add prior versions to the lookup chain)

I can't remember what the hesitation is in changing this is though. Maybe @bf4 can speak more to that.

thewatts · 2016-08-17T03:23:21Z

@NullVoxPopuli for the lookup chain and versioning... would we try to specifically look for versioned namespaces?

ex: if V2 exists, assume V1 does as well? That could be tricky, since technically relationships could work both ways. V1 of a Serializer starts using a V2 of another.

NullVoxPopuli · 2016-08-17T10:01:23Z

lib/active_model/serializer.rb


-      chain
+      if self != ActiveModel::Serializer
+        until parent_namespace.empty?


This is getting there! Why not use parent namespace.each?

Actually, I think using .each would turn this in to too much work. Haha

We could def use .each if we split the string on :: - but we'd have to build the namespaces a little differently.

I'm game for either.

I think the way you have it may be the cleanest.
Just trying to think of a way it could possibly throw an exception.. (I haven't thought of one so far)

NullVoxPopuli · 2016-08-17T10:05:50Z

Could you add a change log entry as well?
Thanks!

NullVoxPopuli · 2016-08-17T10:17:18Z

lib/active_model/serializer.rb

-      chain.push("#{resource_namespace}::#{serializer_class_name}")
+      # Loops over a namespaced Serializer's name to add multiple
+      # namespaced serializer candidates for lookup in the `lookup_chain` from
+      # the "parent" serializer.


This is the kind of documentation I'd like to see all over AMS :-)

Yeah it's a tough balance - solid documentation can be a bonus - but it's the first thing to become stale unless tightly monitored.

Typically I try to have the code be self documenting (so to speak), but at this point we're deep into the belly of the beast :)

Yeah, everyone's goal is self-documenting code. But as long as churn is low, documentation is great to have. I don't see serializer lookup changing very often :-)

bf4 · 2016-08-17T20:51:45Z

Just wanna let you all know I'm aware of this but haven't yet looked at it :)

bf4 · 2016-08-31T06:13:05Z

lib/active_model/serializer.rb

      serializer_class_name = "#{resource_class_name}Serializer"

-      chain.push("#{name}::#{serializer_class_name}") if self != ActiveModel::Serializer
-      chain.push("#{resource_namespace}::#{serializer_class_name}")
+      # Loops over a namespaced Serializer's name to add multiple


This kind of comment should be above the method definition, no?

NullVoxPopuli · 2016-11-13T03:16:07Z

is this still an issue on master?

thewatts · 2016-11-13T03:27:19Z

@NullVoxPopuli - I see that you just pushed an update here, #1968.

This specific PR was for trying to auto-build the namespace lookup chain used during declarations in the Serializer classes.

ex:

module Api
  module V1
    class AuthorSerializer < ApplicationSerializer
      has_many :posts # this would fail to fine the correct serializer

My apologies for not staying on top of it - shortly after the PR was submitted - I found out I was losing my job @ my employer (cutbacks). I've since been in the transition to other employment.

It looks like your PR adds the ability to specify the namespace directly in a before_action callback which may render the needs for this PR obsolete.

If you still want it in master - I can rebase off of master and make the necessary adjustments, add tests, and update the Changelog (will take me a week or so to get the available time).

If not - no worries 😄

NullVoxPopuli · 2016-11-13T03:47:35Z

sorry for your loss -- company cut backs are never fun -- even for those that stay.

Anywho, for this PR, I'd like to see some tests so that we know exactly what situation your PR addresses.

but being able to specify a namespace hopefully will help a ton of people.

Extend Serializer lookup for multiple namespaces

9412a19

NullVoxPopuli reviewed Aug 16, 2016
View reviewed changes

Tidy dynamic namespace lookup for serializers

7cff115

NullVoxPopuli reviewed Aug 17, 2016
View reviewed changes

NullVoxPopuli added C: Feature S: Changelog Missing C: Namespacing/Versioning labels Aug 17, 2016

NullVoxPopuli added the S: Needs Tests label Aug 17, 2016

NullVoxPopuli reviewed Aug 17, 2016
View reviewed changes

bf4 reviewed Aug 31, 2016
View reviewed changes

bf4 mentioned this pull request Oct 31, 2016

AMS should error when no serializer can be found #1853

Open

benlangfeld mentioned this pull request Oct 14, 2021

Namespace serializers for consuming controller rubyforgood/Flaredown#538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(WIP) Fix Serializer Lookup Chain for Namespaces #1883

(WIP) Fix Serializer Lookup Chain for Namespaces #1883

thewatts commented Aug 16, 2016

mention-bot commented Aug 16, 2016

NullVoxPopuli commented Aug 16, 2016 •

edited

Loading

NullVoxPopuli commented Aug 16, 2016

thewatts commented Aug 16, 2016

thewatts commented Aug 16, 2016

NullVoxPopuli Aug 16, 2016

thewatts Aug 17, 2016

NullVoxPopuli commented Aug 16, 2016 •

edited

Loading

thewatts commented Aug 17, 2016

NullVoxPopuli Aug 17, 2016

NullVoxPopuli Aug 17, 2016

thewatts Aug 17, 2016

NullVoxPopuli Aug 17, 2016

NullVoxPopuli commented Aug 17, 2016

NullVoxPopuli Aug 17, 2016

thewatts Aug 17, 2016

NullVoxPopuli Aug 17, 2016

bf4 commented Aug 17, 2016

bf4 Aug 31, 2016

NullVoxPopuli commented Nov 13, 2016

thewatts commented Nov 13, 2016

NullVoxPopuli commented Nov 13, 2016

(WIP) Fix Serializer Lookup Chain for Namespaces #1883

Are you sure you want to change the base?

(WIP) Fix Serializer Lookup Chain for Namespaces #1883

Conversation

thewatts commented Aug 16, 2016

Purpose

Changes

Caveats

Related GitHub issues

Additional helpful information

mention-bot commented Aug 16, 2016

NullVoxPopuli commented Aug 16, 2016 • edited Loading

NullVoxPopuli commented Aug 16, 2016

thewatts commented Aug 16, 2016

thewatts commented Aug 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Aug 16, 2016 • edited Loading

thewatts commented Aug 17, 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

NullVoxPopuli commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Aug 17, 2016

Choose a reason for hiding this comment

NullVoxPopuli commented Nov 13, 2016

thewatts commented Nov 13, 2016

NullVoxPopuli commented Nov 13, 2016

NullVoxPopuli commented Aug 16, 2016 •

edited

Loading

NullVoxPopuli commented Aug 16, 2016 •

edited

Loading