Deprecate ActiveModelSerializers::Model

[richmolj]

Purpose

Deprecate ActiveModelSerializers::Model. We don't need it. We can easily support PORO serialization without this class, whether it be a superclass or rewritten as a mixin.

The docs make it seem like we're providing the ability to do PORO serialization. We're actually providing more functionality than we need to do this.

Minimum requirement we need:

alias :read_attribute_for_serialization, :send

But this ActiveModelSerializers::Model provides extra stuff like

post = Post.new(title: 'my title')
post.title # 'my title'
post.attributes # { title: 'my title' }
post.as_json # { title: 'my title' }

So these are really solving two problems:

• Ability to make an object serializable by AMS
• A class that provides ActiveModel objects (constructor params, #attributes, etc).

While the latter is nice to have in tests and such, it's not really required by this gem AFAIK. I'd say it should be out of scope, actually, and better handled by a separate library like Virtus.

In other words, we're linting that we quack like more of a duck than we really need to quack like.

This PR attempts to allow serializing POROs out of the box, with no extra superclass or mixin required.

Changes

This PR is a WIP but I think the meat of the change is altering read_attribute_for_serialization:

    # active_model/serializer.rb
   # obv refactor this
    def read_attribute_for_serialization(attr)
      # Start by favoring serializers method override
      if respond_to?(attr)
        send(attr)
      else
        # Otherwise, check read_attribute_for_serialization
        if object.respond_to?(:read_attribute_for_serialization)
          object.read_attribute_for_serialization(attr)
        else
          # Fall back to object property/method
          if object.respond_to?(attr)
            object.send(attr)
          else
            # Special logic for hashes
            if object.is_a?(Hash)
              object[attr] || object[attr.to_s]
            end
          end
        end
      end
    end

You can now serialize any PORO, including ruby hashes, with no extra code.

The other changes in this PR are mostly related to reorganizing ActiveModelSerializers::Model (this intended to convert it to a mixin). I suggest we end up not making these changes and instead write a deprecation any time ActiveModelSerializer::Model is extended or initialized.

Caveats

One issue is with the current tests, which currently use ActiveModelSerializer::Model for POROs. In particular heavy use of the constructor. We would probably want to rearrange the tests so they keep this behavior but inherit from some internal test class instead.

Related GitHub issues

#1877
#1283
#1873

Additional helpful information

This code is not ready to be merged. It's what I have working and available for discussion. If we can reach consensus I'll flesh it out.

[mention-bot]

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

[bf4]

I don't like putting anything in the gemspec not required for running the tests. I'd rather this be proposed to the Gemfile in a separate PR. Since this has come up a couple of times and I basically have the same thing in my Gemfile.local I think it's worth adding

[richmolj]

#1913

[bf4]

re: deprecation, we can easily extract all the behavior to a mixin... I'm not sure I want to deprecate, since it's just a convenience implementation of the interface.

[NullVoxPopuli]

👎

[richmolj]

For most of the comments: agreed, all good - this is mostly just scratchpad stuff I pushed so we could talk about the deprecation notion itself. If we decide to deprecate vs. turn into a module vs. just make the accessor tests pass, I'll submit a new PR.

[richmolj]

Regarding Hash: kinda disagree but sure, I can remove that. That was just bonus.

But after that, maybe someone could explain the point of the contract to me. I understand what it is, I just don't know why it's part of this gem...this gem doesn't use most of that functionality AFAIK. For instance, why do we care how the object to be serialized was initialized?

As far as I can tell the only required contract is read_attribute_for_serialization? My point in attempting a deprecation is a lot of work seems to be going into maintaining a contract that is not needed...

[richmolj]

By the way, the reason this started coming up is because our new ModelMixin (or Serializable) would have to override the constructor, which is pretty implicit and often not desirable. Given a class like:

# Assume everything else about child is ActiveModel compliant
class Child
  def initialize(parent, attributes: {})
    @parent = parent
    @attributes = attributes
  end
end

If we did:

class AnotherChild < Child
  include ActiveModelSerializers::Serializable
end

We've now

• Broken this user's code
• Not really provided them with a facility to make this object serializable (without telling the user to write a read_attribute_for_serialization method, which breaks the whole point of the contract stuff).

[NullVoxPopuli]

I don't think you'd have to override the constructor.

The only thing the constructor has done for AMS::Model, is that it sets default / starting attributes on the model. Since attribute tracking/storage is inevitably up to the implementation, we could just forgo that "feature" of the barebones module.

[richmolj]

@NullVoxPopuli yes, we both had the same thought 😄 The problem is the constructor sets @attributes which most of the rest of that module is based off of. read_attribute_for_serialization, for instance, is all about getting the value from attributes and falling back to a default:

    def read_attribute_for_serialization(key)
      if key == :id || key == 'id'
        attributes.fetch(key) { id }
      else
        attributes[key]
      end
    end

So if we remove the constructor the rest of the ActiveModelSerializers::Model falls apart. I started trying to make those methods conditional, falling back to send if attributes was blank...that's what I realized none of the class really seems to matter.

we could just forgo that "feature" of the barebones module.

So this is my thinking as well. What features would a barebones module provide? And as far as I can tell, the only required feature is:

alias :read_attribute_for_serialization, :send

If that's the only thing our barebones module would need, why not just handle that in a conditional within the code instead of requiring clients to implement it? Then our only contract is if you define attribute :foo your object needs to respond to send(:foo). Here's my (rough) test asserting I can serialize a PORO with nothing else needed. Shouldn't that be the minimum contract?

[NullVoxPopuli]

Even though I think time could better be spent elsewhere, I think this should be done more incrementally, rather than a whole bunch of controversial changes all at once.

[richmolj]

@NullVoxPopuli I agree a real PR should be done more incrementally, this was just a WIP to get a discussion going. I kind of regret doing a PR instead of an issue since I think that is a source of confusion.

AMS::Model is meant to serve is a guideline for others to implement their own way of doing things.
I think time and resources could be better spent elsewhere than to over-try to make AMS::Model flexible for all scenarios.

I can see why you would think that is what this PR is trying to do, since it really has one foot in 'make it a mixin' and one foot in 'deprecate it'. However, that's not my intent. I am not trying to make AMS::Model flexible. I am pointing out that it is not needed at all.

I think at a minimum, you could change read_attribute_for_serialization to use send (that way we don't need to worry about each frameworks implementation of the attributes idea.

My point is, if we did this, ActiveModelSerializers::Model is now useless and can be removed. I still haven't heard what the point of it is.

[NullVoxPopuli]

@richmolj do you think you have the time / desire to split everything up?

[richmolj]

@NullVoxPopuli I think so, it's actually mostly changing the test POROs more than anything else. But I want to make sure we all agree on the gameplan / that this makes sense before I take on that work. My suggested plan is here.

[NullVoxPopuli]

I think that's the right track, yeah.

Though, I don't think it's stated that ActiveModelSerializers::Model would be replaced by ActiveModelSerializers::Serializable

Though, I wonder if these should be namespaced further?

ActiveModelSerializers::Serializable
ActiveModelSerializers::Serializable::Errors
ActiveModelSerializers::Serializable::Cache

[richmolj]

@NullVoxPopuli agree on the namespacing but

Though, I don't think it's state that ActiveModelSerializers::Model would be replaced by ActiveModelSerializers::Serializable

I'm not sure this module is even needed at all. What do you foresee the problems being?

[NullVoxPopuli]

Well, something needs to implement read_attribute_for_serilaization.

[richmolj]

Not if we change serializer.rb to be:

if object.respond_to?(:read_attribute_for_serialization)
  object.read_attribute_for_serialization(key)
else
  object.send(key)
end

[NullVoxPopuli]

I'm conflicted on that. I think for now, to maintain existing functionality, we should have a small module that implements read_attribute_for_serialization via send. (which, I know is the same thing), but I don't think we should force everyone to use send by default (as would be the case if implemented in serializer.rb)

[richmolj]

but I don't think we should force everyone to use send by default

In the above code, read_attribute_for_serialization is still the default?

Anyway, I still prefer the serializer.rb change but I can get behind this 👍

[NullVoxPopuli]

we can make AMS::Model deprecated and split it in to 3 modules without controversial changes :-)

I just think forcing send usage by default (in serializer.rb) could cause unwanted consequences in some situtaions :-\

[richmolj]

But our module would make send the default (assuming they don't write their own read_attribute_for_serialization) anyway? Seems like 6 in one, half dozen in t'other.

[NullVoxPopuli]

not so. serializer.rb is used everywhere. the model is only used by non AR people. :-\

[richmolj]

everywhere == activerecord right? activerecord already implements read_attribute_for_serialization, so send would never be called

[richmolj]

In fact, it internally just calls send O_o

https://github.com/rails/rails/blob/master/activemodel/lib/active_model/serialization.rb#L168

[NullVoxPopuli]

lol. Well I've been defeated!

I guess go ahead and PR away. haha

[richmolj]

😆 @bf4 you agree with this plan? If so I can get cracking on PRs.

[bf4]

I haven't read everything so bear with me:

read_attribute_for_serialization was born in Rails before AMS was removed

Linting was added to clarify the api of reaources that AMS expects

Linting was based on ActiveModel::Lint

ActivrModel::Lint gave birth to ActiveModel::Model.

AMS::Model was born as PORO documentation and for convenience

I use AMS::Model myself

So, everything is need driven

I only never separated minimal linting from comprehensive

Initializers have nothing to do with the interface AMS expects of arguments to its serializers

AMS::Model is executable documentation, not a requirement

[bf4]

@richmolj can you clarify the plan? The tl;dr is that it is a huge change if AMS no longer expects objects to either include ActiveModel::Serializers::JSON or implement its interface. see #1911 (comment) for some other comments I wrote from my phone. Is the problem just that you think being able to require AMS::Model causes more problems than it solves, or something deeper?

[richmolj]

@bf4 sure, let me try and separate the why and the how here. I'm more than happy to be in the wrong here due to some missing context, I'm just not sure what that context is.

Why

• 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.

AMS::Model is executable documentation, not a requirement

• To end users, it seems like a requirement. It did to me.

How

I think five main steps:

• Make the change in the code diff above
• Create an ErrorSerializable mixin
• Create a CacheSerializable mixin
• Deprecate ActiveModelSerializers::Model
• Remove linting (or only run it for, say, cache-related tests).

Does this make sense?

[bf4]

Thanks for writing that all up. Do I understand that you'd prefer the only requirement for input serializer is respond_to? and send? Just for context, you've looked at why read_attribute_for_serialization was introduced, what it's for, how it's used in Rails, and that AMS uses activemodel because it came from activemodel and complements it?

Given our discussion, am I correct that better docs would explain PORO usage better?

More later when I'm not thumb typing an email while putting the kids to sleep :)

[richmolj]

@bf4 let me try a silly analogy. Let's say we had a dog, but we named it 'cat' and fed it cat food, gave it a litter box, everything. And this issue is akin to saying, "let's stop treating our dog like a cat." Let's get rid of the litter box, throw away the balls of yarn; they are not needed.

And sure you could view that as a major change...but fundamentally it's always been a dog; nothing is really changing.

That may have only made sense to me 😆 Let me answer your specific questions:

Do I understand that you'd prefer the only requirement for input serializer is respond_to? and send?

My point is that's currently the only requirement. We currently require users to alias read_attribute_for_serialization to send to support overriding the default behavior. But send is still the default, whether it's a module adding the alias or a code change that tries to call read_attribute_for_serialization and falls back to send.

Just for context, you've looked at why read_attribute_for_serialization was introduced, what it's for, how it's used in Rails, and that AMS uses activemodel because it came from activemodel and complements it?

As I said at the top of my last comment, I would love to get additional context showing the flaw of my logic. I could absolutely be missing something and would appreciate getting the additional puzzle pieces filled in. I've been going off of commits and comments, the comment for read_attribute_for_serialization is:

Hook method defining how an attribute value should be retrieved for
serialization. By default this is assumed to be an instance named after
the attribute. Override this method in subclasses should you need to
retrieve the value for a given attribute differently

OK, so read_attribute_for_serialization is the public interface over send so you can override it. Great, the proposed change would still honor read_attribute_for_serialization before falling back to send, so there should be no problem there.

and that AMS uses activemodel because it came from activemodel and complements it?

This is just speculation, but my guess is there used to be tighter coupling between this library and activemodel. But over time they have become decoupled. Let's stop treating them like they are still coupled (let's stop treating our dog like a cat).

Given our discussion, am I correct that better docs would explain PORO usage better?

To an extent, maybe? I think the better docs would be something along the lines of "ignore ActiveModelSerializers::Model and add this alias". And if those were the docs I'd ask why we don't just help our users by implementing send, and documenting that they can override this behavior by defining read_attribute_for_serialization.

Once again, I am totally open to getting some missing context. Barring that, all I can do is follow logical reasoning and make a proposal based on that reasoning.

[bf4]

@richmolj I've been thinking about this off and on. Basically, changing the serializable interface is a huge breaking change in the code and philosophy of AMS. It should really be an RFC before it is a PR.

In addition, I'd rather we spend time on critical issues within AMS before re-architecting it. As Kent Beck has said

for each desired change, make the change easy (warning: this may be hard),
then make the easy change

[richmolj]

It's not a breaking change at all, much less a huge breaking change. And per Mr. Beck, it's also already an easy change. If those statements are false, please let me know why, I would love to improve my understanding.

I really would love to get an actual response with some sort of counterargument or technical reasoning. I've said in this thread I regret making this a PR, I only wanted to share how I fixed the tests you wanted fixed and to share a solution on how to make this a mixin (since this was harder than I thought it would be). At this point I don't even have enough information to write better documentation, since no actual technical arguments are being made and no additional context is being supplied. I will move this to an RFC in hopes of getting a better discussion.

[NullVoxPopuli]

@richmolj looks like only a new PR will resolve all the concerns :-)

[richmolj]

New RFC! #1926