Inheriting from ActiveModelSerializers::Model to support serialization of a PORO is a bad practice

[vasilakisfil]

Expected behavior vs actual behavior

Expected:

class SimpleObject
  include ActiveModelSerializers::Model #includes neccesary methods

end

Actual

class SimpleObject < ActiveModelSerializers::Model

end

What would happen if my SimpleObject was inheriting from another class?

Additonal helpful information

#1272

[NullVoxPopuli]

that's a good point.

if you want to make a quick lil PR that extracts the body of AMS::Model into a module and update the docs for using it, that would be hugely appreciated.

Maybe call it ModelMixin or something? idk. that way AMS::Model could include it, and people's existing code wouldn't break.

[vasilakisfil]

alright!

[bf4]

Why AMS::Model is an executable documentation of the API. Why is inheritance a problem?

[bf4]

Oh, you'd rather just be able to include a mixin. So, here's the issue: you might not want the precise implementation of AMS::Model. In fact, there's a bug in it.

[bf4]

Also related

• Document ActiveModelSerializers::Model #1283
• Document ams model #1873 (comment)
• Source of truth: ActiveModel::Serializer::Lint

[bf4]

@vasilakisfil Is there anything else you'd like to discuss here? FWIW, I use AMS::Model, but there is a known bug in it around the attributes hash and read attribute for serialization not returning the same thing, which is due to different implementation decisions in AM::Model and AM::Serializers::JSON.. I haven't resolved it yet

[vasilakisfil]

No I think I am fine although I haven't seen the bug you are talking about.

[richmolj]

I rather agree with @vasilakisfil a mixin would be preferable. My understanding from this comment on #1873 is that the bug is:

class Post < ActiveModelSerializers::Model
 attr_accessor :title
end

# works
Post.new(title: 'my title').as_json # { title: 'my title' }

# does not work
post = Post.new
post.title = 'my title'
post.as_json # {}

Quick guess on why this doesn't come up more often and maybe why @vasilakisfil hasn't seen it: Many of us use Virtus for POROs which has its own attributes, overriding the one from AMS and solving the problem.

However, this seems a pretty simply fix. ActiveModelSerializers::Model gets its attributes from a @attributes attr_reader. This could be changed to something like:

def read_attribute_for_serialization(key)
  send(key) if respond_to?(key)
end

@bf4 would you support something like this?

[bf4]

@richmolj If you can solve this, you will be my hero! I've tried!

In terms of being a mixin, we'd have to discuss what should be included and what not and what defaults.

At this point, it might be best to first drop activemodel::model as a dependency and instead use the useful parts of it, get AMS::Model all fixed up, then move to a mixin.

[bf4]

ref: #1903

[richmolj]

@bf4 so I was able to add the additional accessor tests, get them all passing, and convert to a mixin. However, as I was finishing I thought a better solution would be to deprecate this class altogether and support serializing POROs and hashes with no extra work, subclass or mixin required. #1911.

If I'm missing something the code to convert to a mixin is still mostly there. This was actually a lot trickier than I thought. One issue is changing to a mixin changes the load order. In the current code, we include ActiveModel::Serializers::JSON which includes [ActiveModel::Serialization](https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model_serializers/model.rb#L7` which defines `read_attribute_for_serialization) - but our method 'wins'.

When converting to a mixin using the included hook to include ActiveModel::Serializers::JSON, their method 'wins' and we don't properly override this. This is fixed by using class_eval within the included hook instead of a normal mixin.

The fix for the accessor bug I believe was a combination of this and this.