Only use subclasses of ActiveModel::Serializer during lookup.

[beauby]

Fixes #1293.

[trek]

I don't think this would fully address the problem. For example, if you're migrating from nothing to AMS and have a Tweet class and TweetSerializer, everywhere you render :json tweet_instance will use the TweetSerializer whether that is your intent or not (making it really painful to use AMS on newer versions of your API but not affect previous versions responses).

I think basically you need an option to switch off automatic lookup: https://github.com/rails-api/active_model_serializers/compare/master...trek:config-autolookup?expand=1

[beauby]

The whole purpose of this PR is to avoid what you just described. Have a look at the provided test to convince yourself. Or maybe I misunderstood something?

[bf4]

kill ducks!

[NullVoxPopuli]

LGTM.
Solves the problem of any serializer named with the AMS naming pattern accidentally getting used and ensures that only AM::S classes are used for 'serialization'

[bf4]

Wait, why was this merged? OP said it wouldn't solve the problem and limiting serializers to subclasses doesn't make sense

[bf4]

cc @rails-api/ams re considering reverting this. OP said it wouldn't help #1294 (comment) and made his own PR #1295

[beauby]

@bf4 The reasons OP stated are not valid, unless I didn't understand.
Would you care to elaborate on how it does not make sense to limit to AM::S subclasses?

[bf4]

The reasons OP stated are not valid, unless I didn't understand.

I just think that if OP @trek says this doesn't look like the fix he needs it shouldn't be merged until he says it works or it just seems like a good thing for the library to have

Would you care to elaborate on how it does not make sense to limit to AM::S subclasses?

two reasons

1. It prevents people from using the library the way they want. If someone wants to write their own adapter, they can, as long as they use the adapter interface. Until this PR was merged, someone could do that with serializers too. (Though I'd like to make it easier, in general)
2. Specific to this PR: it uses really esoteric code, at least in my experience. I'd love to see counter examples, though :)

Also, looping three times instead of one just seems unnecessary. (And yes, I understand that lazy evaluation wouldn't loop three times, but we're not using lazy evaluation).

[beauby]

For 2) we already agreed on dropping support for 1.9.3, and lazy evaluation will come right when we do. As for the alleged 'esotericness', I suggested replacing find { |x| x } with find { |x| !x.nil? } when that PR was pending but nobody followed up. Apart from that bit, I do not think this line has anything special. Could you point to what you find esoteric?
For 1) I cannot think of a use case where it would really make sense, but I'm open to suggestions.

[beauby]

Moreover, 2) is still possible by explicitly providing the serializer.

[NullVoxPopuli]

Sorry about that, I thought it was good to go, and something beneficial :-/

[trek]

The reasons OP stated are not valid, unless I didn't understand.

How are they not valid? This PR did fix a problematic situation ("I have serializers but they are not AMS") but doesn't address what I suspect will be the larger problem for existing applications: You were not using Serializers (of any flavor) in existing controllers but want to use them for new controllers. AMS will lookup serializers and change render output when introduced:

Here's an example of where this would cause breaking changes into an existing API:

Imagine an application like this

# models/tweet.rb
class Tweet < ActiveRecord::Base
end

# controllers/v1/tweets_controller.rb
class V1::TweetsController < ApplicationController
  def show
     render json: Tweet.find(params[:id])
  end
end

V1::TweetsController#show is rendering with a bare .to_json call on a model

For v2, the developers decide to adopt JSON API via AMS:

# models/tweet.rb
class Tweet < ActiveRecord::Base
end

+ # serializers/tweet_serializer.rb
+ TweetSerializer < ActiveModel::Serializer
+   attribute :body
+ end

# controllers/v1/tweets_controller.rb
class V1::TweetsController < ApplicationController
  def show
     render json: Tweet.find(params[:id])
  end
end

+ # controllers/v2/tweets_controller.rb
+ class V2::TweetsController < ApplicationController
+   def show
+      render json: Tweet.find(params[:id])
+   end
+ end

Now, the newly introduced V2 controller is OK. The augmentation to render looks up TweetSerializer and serializes using that. However, this also occurs for the v1 controller, mischievously changing its render behavior even though nobody changed the existing code.

There are two solutions to this:

1. Turn off AMS per action by passing adapter: false

   def show
      render json: Tweet.find(params[:id]), adapter: false
   end
   

2. Allow AMS to be opt-out globally, but default that option to opt-in Only use subclasses of ActiveModel::Serializer during lookup. #1294.

   ActiveModel::Serializer.config.automatic_lookup = false
   
   # V1, not affected by AMS
   def show
      render json: Tweet.find(params[:id])
   end
   
   # V2 specifically opting-in to AMS
   def show
      render json: Tweet.find(params[:id]), serializer: TweetSerializer
   end
   

Option 1 makes it easy to opt out locally (nicer for smaller or greefield apps), Option 2 makes it easy to opt in locally (nicer for larger legacy apps).

In this small example, each option feels like roughly the same amount of work. But in real world examples there will be – initially – many hundreds of renders that don't want AMS, and several dozens that do.

Over time, this balance will shift to the other side: all non-AMS controllers will end up removed from the application as those versions of the API are discontinued.

Controllers that manually opted into AMS will continue to function, new controllers can begin using the automatic lookup behavior:

# use default behavior
- ActiveModel::Serializer.config.automatic_lookup = false

- # V2, deprecated and removed
- def show
-    render json: Tweet.find(params[:id])
- end

# V8, manually opted into AMS
def show
   render json: Tweet.find(params[:id]), serializer: TweetSerializer
end

+ # V9 using automatic lookuop
+ def show
+    render json: Tweet.find(params[:id])
+ end