[RFC] Deprecate ActiveModelSerializers::Model

[richmolj]

• Start Date: (2016-09-14)
• RFC PR: N/A
• ActiveModelSerializers Issue: Inheriting from ActiveModelSerializers::Model to support serialization of a PORO is a bad practice #1877

Summary

The documentation on serializing POROs leads one to believe the PORO should subclass ActiveModelSerializers::Model. There are three issues with this:

• Subclassing is a bad practice when a mixin will do. What if I already have a superclass?
• ActiveModelSerializers::Model provides a bunch of extra functionality that is not needed by this library. So POROs are polluted for no reason.
• This gives the false impression that ActiveModelSerializers::Model or the contract that it provides is somehow relevant to the rest of this library.

Motivation

• Because as far as I can tell AMS does not need objects to implement this interface. Maybe at one point it did, but this test shows POROs can be serialized without any special mixin, interface, etc.

  The only code change to make that work is:

  def read_attribute_for_serialization(attr)
    if respond_to?(attr)
      send(attr)
    else
  -     object.read_attribute_for_serialization(attr)
  +     if object.respond_to?(:read_attribute_for_serialization)
  +       object.read_attribute_for_serialization(attr)
  +     else
  +       object.send(attr)
  +     end
    end
  end

  So to me it's like we might as well lint that these objects respond to foo; it's arbitrary.

• This provides a better experience to end users. No special steps required to serialize POROs.

• Currently the documentation is misleading / point of ActiveModelSerializers::Model is misleading. It says in order to serialize POROs I should subclass this, but it actually adds a lot of stuff to my object I may not want (polluting my PORO). For instance these objects now respond to updated_at...I would never expect that as an end user. In addition, most of my objects are Virtus objects, which has its own @attributes implementation that just happens to work right now but is ripe for future conflicts.

• The logic for constructors/attributes/errors etc might be nice to have, but it's not relevant to a serialization library. I would rather this move to something like an extension.

• The fact that we're having this discussion is one of the problems. Yeah, we lint these objects quack like a certain kind of duck, but nowhere do we assert why they have to quack that way. If I implemented a feature that needed objects to respond to as_json, but a few months later that feature gets removed, nobody knows if we can remove as_json from the lint or not.

• Begins to remove Rails dependencies, so this library could be used in Sinatra et al without requiring ActiveModel and friends.

• I prefer a more composable approach so I pollute my POROs the minimal amount possible. I'll start with a PORO that can be serialized by default, but I'll add ActiveModelSerializers::CacheSerializable if my serializer needs the caching feature. Or (this would be my preference) we can implement caching so that it doesn't have this requirement at all. For instance it seems easy to put cache_key as a method on a serializer, instead of on the object we are serializing.

Detailed design

Once again, make this code change:

  def read_attribute_for_serialization(attr)
    if respond_to?(attr)
      send(attr)
    else
-     object.read_attribute_for_serialization(attr)
+     if object.respond_to?(:read_attribute_for_serialization)
+       object.read_attribute_for_serialization(attr)
+     else
+       object.send(attr)
+     end
      end
    end

Then, create ActiveModelSerializers::CacheSerializable and ActiveModelSerializers::ErrorSerializable. These will implement only the bits of ActiveModelSerializers::Model that are needed for those purposes.

If possible, both those mixins should be immediately deprecated as well. For instance it seems like an easy change to use the serializer's updated_at instead of requiring the object respond to updated_at. This would be better code in any case. Suggest these conversations happen in separate, targeted PRs.

Finally, remove the lint tests since these are both unnecessary and misleading.

Drawbacks

We'd probably add something to our test POROs so they could keep the otherwise-irrelevant constructor functionality ActiveModelSerializers::Model provides. So you could make a case our tests wouldn't be testing on true POROs.

Suggest handling this by adding a separate test(s) specific to vanilla POROs.

Alternatives

We could avoid the code change mentioned above, and instead implement a mixin that does the same:

module ActiveModelSerializers
  module Serializable
    def self.included(klass)
      klass.class_eval do
        alias :read_attribute_for_serialization, :send
      end
    end
  end
end

I would support this alternative, though I think it's kind of silly and puts extra work on our users without justification.

Unresolved questions

In the 0.8x series, my_model.as_json would use AMS to serialize. This functionality is not currently in 0.10.x, though there are many comments about doing so.

I suggest this is the true purpose of a future ActiveModelSerializers::Serializable mixin, that should be independent of this issue (and completely optional to users). If that's not the case, I imagine we would double down on ActiveModelSerializers::Model and monkey-patch ActiveRecord...so the unresolved question would be "is that really a good idea"...

Additional Information

Prior discussion here: #1911