Add explicit serializer not found policy #1949

GregPK · 2016-10-20T14:35:53Z

Purpose

Allow users to setup a policy to fail explicity when an ActiveRecord::Base inherited model is being used without an explicit Serializer defined.

Currently, when rendering a ActiveRecord::Base model it falls back on the default JSON serializer, exposing all attributes.

Changes

ActiveModel::Serializer#get_serializer_for

Related GitHub issues

AMS should error when no serializer can be found #1853

NullVoxPopuli

Looks like a good start, just a few comments / questions.

NullVoxPopuli · 2016-10-20T15:16:05Z

lib/active_model/serializer.rb

+
+      if serializer_class
+        serializer_class
+      elsif klass.superclass && ![BasicObject, Object].include?(klass.superclass)


why'd you add to the condition here?

I wanted to get a meaningful error message. Otherwise, we always get klass == BasicObject and hence ("Serializer for [BasicObject] not found"), since it's at the bottom of the object hierarchy.
I agree this is a bit wonky, but I couldn't think of anything better.

perhaps,

But since we don't just deal with ActiveRecord objects, I think this scenario would behave unexpectedly with your code.

@NullVoxPopuli As I understand it (from reading this) we cannot find a serializer for any object:

An ActiveModel::Serializer wraps a serializable resource and exposes an attributes method ...

So the method in question would have to at least have :serialized_hash implemented, probably inherit ActiveModel::Model or include ActiveModel::Serialization.

Also: I've refined the test to show what I want to do with the class handed to the policy:
837df53

right, but what is klass right now? (this is a huge chance in improve the in-code docs).

does it represent a serializable object? (PORO, AR, etc -- none of these need serialized_hash)

So, with your test, what happens if Comment were defined as:

class Comment; end

Not sure what you mean - I've added a test to see if this doesn't mess up POROs - please take a look if this is what you're concerned about.

This is of course under the assumption that if an object is not serializable then it should not trigger the policy.

NullVoxPopuli · 2016-10-20T15:18:04Z

lib/active_model/serializer.rb

+      elsif klass.superclass && ![BasicObject, Object].include?(klass.superclass)
+        get_serializer_for(klass.superclass)
+      elsif config.on_serializer_not_found.respond_to?(:call) && klass.respond_to?(:serialized_attributes)
+        config.on_serializer_not_found.call(klass)


I'm not sure how I feel about ending with situation 4 (as described in the method comments) as the result of an empty block.

Can you explain why you're checking for serialized_attributes in the elsif here, and what you would do if on_serializer_not_found is nil?

NullVoxPopuli · 2016-10-20T15:18:36Z

lib/active_model/serializer/concerns/configuration.rb

@@ -10,6 +10,7 @@ module Configuration
        config = base.config
        config.collection_serializer = ActiveModel::Serializer::CollectionSerializer
        config.serializer_lookup_enabled = true
+        config.on_serializer_not_found = -> {}


what if this is nil?
should it start out as nil? does defaulting to an empty block feel weird?

See comments above about the effects (TLDR: none). When I set the default here, I thought that maybe someone will be looking at the code here to consider the values for on_serializer_not_found. I thought that an empty proc will communicate better that this was created to be (as opposed to nil which could be anything).

NullVoxPopuli · 2016-10-20T15:19:06Z

test/serializers/serializer_for_test.rb

+          ActiveModelSerializers.config.on_serializer_not_found = @previous_serializer_not_found_policy
+        end
+
+        def test_serializer_not_found_triggers_configured_policy


can you change these to the test 'serializer not found triggers configured policy' test format.

Thanks!

NullVoxPopuli · 2016-10-21T11:46:10Z

lib/active_model/serializer.rb

      end
    end

+    def self.excluded_base_class?(klass)


this method name doesn't make sense, it has a negative word, yet the condition is testing for the positive

NullVoxPopuli · 2016-10-21T11:50:09Z

lib/active_model/serializer.rb

+
+      if serializer_class
+        serializer_class
+      elsif klass.superclass && !excluded_base_class?(klass.superclass)


see, where I'm getting caught up on this, is that I'm reading this as:

"if the objec i'm trying to serialize has a superclass, and that superclass is neither ActiveRecord::Base, BasicObject, nor Object, then get the serializer for that ....."

And here is the problem (in my looking things over too quickly -- you're getting the serializer for the superclass, not klass -- this makes sense now -- sorry about the confusion).

So, I guess for clarity, the condition should be extracted to a method -- maybe

`is_superclass_allowed_to_be_serializable?(klass)

but then, why don't we want those superclasses serializable? I know it's weird otherwise, but what if someone wanted to try to write a general serializer for Object? would that be super weird? idk.

The problem here is that I only need this additional if condition to actually have a side-effect on the next if, which is really why we're coming up against this kind of reasoning issues. I'll try to come up with a better solution.

NullVoxPopuli · 2016-10-21T13:23:03Z

lib/active_model/serializer.rb

+
+      if serializer_class
+        serializer_class
+      elsif klass.superclass && find_serializer_class(klass.superclass)


this makes much more sense!

also, I was jus about to suggest extracting the serializer_lookup to a method. :-)

yeah - it was obvious after staring for 2 hours at it

What happens if A < B < C and there is a serializer for C but not for B?

Sounds like we need to check ancestors?

NullVoxPopuli · 2016-10-21T13:23:52Z

lib/active_model/serializer.rb

+      elsif klass.superclass && find_serializer_class(klass.superclass)
+        get_serializer_class(klass.superclass)
+      elsif config.on_serializer_not_found.respond_to?(:call) && klass.respond_to?(:serialized_attributes)
+        config.on_serializer_not_found.call(klass)
      end


what happens if all the conditions are false? what do we do then?

we return nil like we always did :)

NullVoxPopuli · 2016-10-21T13:30:32Z

I feel MUCH better about this now. so, you get the 👍 from me.

I'd like one more maintainer to review though -- @bf4, @beauby?

beauby · 2016-10-21T13:33:35Z

lib/active_model/serializer.rb

      end
    end

+    def self.find_serializer_class(klass)
+      serializer_lookup_chain_for(klass).map(&:safe_constantize).find { |x| x && x < ActiveModel::Serializer }


Can be lazified, i.e.:

serializer_lookup_chain_for(klass).lazy.map(&:safe_constantize).find { |x| x && x < ActiveModel::Serializer }

GregPK · 2016-10-30T10:29:57Z

@NullVoxPopuli @beauby I cleaned up the code and took care of the ancestor issues. Please take a look.

NullVoxPopuli · 2016-10-31T00:41:58Z

@GregPK can you fix the rubocop issue real quick?

bf4 · 2016-10-31T02:12:11Z

lib/active_model/serializer.rb

+
+      if serializer_class
+        serializer_class
+      elsif (ancestor_serializer_classes = ancestor_serializer_classes(klass)).present?


so this is change 1: s/klass.superclass/ancestor_serializer_classes(klass)

This is similar to code in either 0.8 or 0.9 IIRC

better Ruby to check for .any? as you know you have a collection and you're checking if there's anything in it. present? is one of those convinces Rails gives us that makes our code more less clear.

bf4 · 2016-10-31T02:16:16Z

lib/active_model/serializer.rb

+        serializer_class
+      elsif (ancestor_serializer_classes = ancestor_serializer_classes(klass)).present?
+        ancestor_serializer_classes.first
+      elsif config.on_serializer_not_found.respond_to?(:call) && klass.respond_to?(:serialized_attributes)


and this is change 2: a new condition

Since we own this config, we shouldn't need to check if it responds to call. We can either make a default strategy or just check if the config is set. I think I like having a default strategy that does nothing an returns :no_serializer_found

What is serialized_attributes that you are checking for?

The config setup you mentioned is probably the best way to go. However, if we setup a default strategy that returns a symbol, we will brake backwards compatibility for anyone relying on the historical behaviour of returning nil.

You're right about checking for callability. I think we can actually do elsif config.on_serializer_not_found - if it's an empty proc it will run, and when someone messes up the config value (ie setting it to :no_serializer_found instead of -> { :no_serializer_found } he will get an explicit error about what's wrong with the code. WDYT?

For :serialized_attributes, I'm checking whether this is actually something that represents a serializable AR resource. I'm aware that this excludes non AR-based objects. Maybe change this to klass.respond_to?(:serialized_attributes) || klass.respond_to?(:serialized_hash)? This will handle both cases. I would much rather not couple this to AR via serialized_attributes but I could not find any other way of detecting is the object is both AR-based and serializable.

NullVoxPopuli · 2016-11-13T03:15:11Z

needs rebase

mrnugget · 2017-04-06T13:12:43Z

Any updates on this? I'd love to see this get merged instead of having to add the monkey-patch (which is great btw.!) to our codebase.

bf4 · 2017-04-06T14:00:04Z

I suppose what I might like is a strict mode. i.e., if no serializer is passed explicitly, then raise an error. This means that it would error if you tried to pass in a hash. The problem with having a 'default serializer' is that if someone tries to serialize a hash, we have no way of knowing what 'type' it is. This is particularly problematic for JSON API.

so, it would be something like:

- return nil unless config.serializer_lookup_enabled
+ case config.serializer_lookup_enabled
+ when :strict_mode then fail StrictMode, "Won't lookup serializer for '#{klass}' In Strict mode serializers must be explicit"
+ when false then return nil
+ else nil # continue
+ end
serializers_cache.fetch_or_store(klass) do

bf4 · 2017-04-06T14:01:14Z

Possible regression in 0.10.1 - Object instance variable in parent serializer set to serializer in 0.10.1 #1813 (comment)
Allow users to globally opt out of automatic lookup #1295
- Explicit serializer_lookup_enabled in controller doesn't retain Adapter for entire render #1500

mrnugget · 2017-04-07T06:22:19Z

I thought about changing the approach to this problem, too. I tried by adding a last element to the lookup chain, that raises. The idea being: "if you reached this end of the lookup chain, there's no serializer for you". The only problem is that every element in the chain will be called for every lookup in one flat_map call, so it raises every time, even though the lookup may stop earlier.

beauby · 2017-04-07T06:24:47Z

Truth is, relying on serializer inference incurs a massive perf drop, for a few saved keystrokes.

mrnugget · 2017-04-07T06:36:42Z

What I have now running is a slightly modified version of the code in this pull request. Slightly modified in order to make it work with Rails 5.

-      elsif config.on_serializer_not_found.respond_to?(:call) && klass.respond_to?(:serialized_attributes)
+      elsif config.on_serializer_not_found.respond_to?(:call) && [ActiveRecord::Base, ApplicationRecord].include?(klass.superclass)
      config.on_serializer_not_found.call(klass)

The change was necessary, because serialized_attributes was removed in Rails 5. Now the code says: "Okay, by looking for a serializer and walking up the superclasses you've now reached ApplicationRecord/ActiveRecord::Base and so far we found no serializer, that means.. raise an exception". It works, without breaking the serialization of Hash and String for example.

NullVoxPopuli · 2017-04-07T13:03:47Z

Truth is, relying on serializer inference incurs a massive perf drop, for a few saved keystrokes.

@beauby do we have benchmarks on this?
Also, do we have a solution for explicit -- yet polymorphic -- serializers?

NullVoxPopuli reviewed Oct 20, 2016

View reviewed changes

NullVoxPopuli mentioned this pull request Oct 20, 2016

Inconsistent JSONAPI serialization when missing related serializer #1075

Open

NullVoxPopuli reviewed Oct 21, 2016

View reviewed changes

NullVoxPopuli added C: Feature C: Cleanup S: LGTM C: Config labels Oct 21, 2016

beauby reviewed Oct 21, 2016

View reviewed changes

GregPK force-pushed the issue_1853_serializer_not_found_policy branch 2 times, most recently from b09d925 to b3bf65a Compare October 30, 2016 10:28

GregPK closed this Oct 30, 2016

GregPK reopened this Oct 30, 2016

bf4 reviewed Oct 31, 2016

View reviewed changes

Add explicit serializer not found policy

eba8e17

GregPK force-pushed the issue_1853_serializer_not_found_policy branch from b3bf65a to eba8e17 Compare April 25, 2017 12:24

bf4 mentioned this pull request Nov 13, 2017

Note invalid JSON:API response expected in tests #2213

Closed

GregPK closed this Aug 26, 2019

GregPK deleted the issue_1853_serializer_not_found_policy branch August 26, 2019 17:02

benlangfeld mentioned this pull request Oct 14, 2021

Namespace serializers for consuming controller rubyforgood/Flaredown#538

Merged

Add explicit serializer not found policy #1949

Add explicit serializer not found policy #1949

Conversation

GregPK commented Oct 20, 2016 • edited Loading

Purpose

Changes

Related GitHub issues

NullVoxPopuli left a 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

GregPK Oct 21, 2016 • edited Loading

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 commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GregPK commented Oct 30, 2016

NullVoxPopuli commented Oct 31, 2016

Choose a reason for hiding this comment

bf4 Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Nov 13, 2016

mrnugget commented Apr 6, 2017

bf4 commented Apr 6, 2017

bf4 commented Apr 6, 2017 • edited Loading

mrnugget commented Apr 7, 2017 • edited Loading

beauby commented Apr 7, 2017

mrnugget commented Apr 7, 2017

NullVoxPopuli commented Apr 7, 2017

GregPK commented Oct 20, 2016 •

edited

Loading

GregPK Oct 21, 2016 •

edited

Loading

bf4 Oct 31, 2016 •

edited

Loading

bf4 commented Apr 6, 2017 •

edited

Loading

mrnugget commented Apr 7, 2017 •

edited

Loading