root key for :attributes adapter is pointless

[LeKristapino]

Before upgrading from 0.10.5 to 0.10.10 this line worked without any errors

render json: {first_key: "some string",  second_key: "another string" } , serializer: SomeCustomSerializer

Now it produces an error undefined method 'model_name' for Hash:Class.

Adding a root parameter (for example ..., serializer: SomeCustomSerializer, root: '' fixes the issue.

BUT from the documentation here I figure that root is not even used for :attributes adapter.

My question is, shouldn't it be ignored in the Attribute and JSON API adapters, thus avoiding breaking the example serialization. Or have I missed some extra functionality that root provides?

[wasifhossain]

Hi @LeKristapino that was unfortunate. but upgrading to the latest HEAD of the 0-10-stable branch should resolve the issue.

gem 'active_model_serializers', :git => 'https://github.com/rails-api/active_model_serializers.git', :branch => '0-10-stable'

This PR fixes the case: #2344

[wasifhossain]

@LeKristapino I think I am wrong here. that PR does not fix the case yet! I see it now :(

Actually that was kind of like a side-effect while implementing the fields option for the attributes adapter: docs/general/fields.md

can you please write a test for the scenario that you described, so that we can figure out an effective solution to avoid further side-effects related to this feature fix

[LeKristapino]

@wasifhossain I think it would take me some time to understand the differences in mechanics for calling SomeSerializer.new( {name: "John"}).to_json and the differences with render json: {name: "John"}, serializer: SomeSerializer but in first case there is no problem (and wasn't before). The second case produces an error in 0.10.10 but was working fine in 0.5.10.

[wasifhossain]

@LeKristapino can you share some code to reproduce the fail case. Specifically how SomeCustomSerializer looks like when you pass {first_key: "some string", second_key: "another string" } in render json: call

[bf4]

@LeKristapino If you're trying to serialize a primitive like a Hash, AMS will get confused. AMS is for models. There's no way AMS can know what to do with a Hash. I know this is documented both because I wrote it and because serializing Hashes comes up in issues periodically. When I've need to ensure that the thing I'm rendering doesn't use a serializer, I usually dump it to a string and pass that into the render arguments.

• I'm not positive this broke in the latest release. Have you tried other versions?
• I agree that the attributes adapter has no need to call root

[wasifhossain]

@LeKristapino as bf4 mentioned, can you try your issue on other AMS versions later than 0.10.5 but earlier than 0.10.10 and let us know how you found it?

[LeKristapino]

@wasifhossain The breaking change came after 0.10.9.
0.10.9 Works as expected, 0.10.10 requires me to add root parameter.

As for the reason why I use AMS on a Hash - My DB JSON column has different key names as the API Response I am providing, but mostly it is so that I can create correctly formatted dates in the response instead of the plain format I have in my database JSON.

Sometimes I need to improve my API response formatting with more accurate key names and such, and then it is far easier to do it through a serializer than to modify json in my DB to have the name corrected or improved. So I wouldn't say that there are no use-cases for Serializers being used on Hashes.

Hope that helps

[wasifhossain]

@LeKristapino thanks for the insight.

btw AMS should have already covered the case, like: formatting the attribute name or the value as needed by the api client, and that is exactly the role of the serializer instead of doing anywhere else for an api response; which you got right - to do it in the serializer.

the only gotcha here is, if you need to change the name/value of a DB-backed field, then you can do it through your model instance, instead of passing it as a hash.

docs/general/serializers.md#attributes lists down how to customize the behavior, in case you need it.

Still if you find it hard to figure out, we would appreciate if you can provide some mock example of how the field(s) looks like in the DB and how do you like to serialize it.

[LeKristapino]

I guess a good example would be something like:

DB has JSONB column details.

a persisted Teacher instance would have:

teacher.details = {
"first_name": "John",
"last_name":  "Doe",
"middle_name": "Richard",
"start_date": "01-01-2000",
"end_date": "01-10-2010",
}

then teacher_serializer.rb would have something like:

class TeacherSerializer < ActiveModel::Serializer
  attributes :details

  def details
    TeacherDetailsSerialzer.new(object.details)
  end
end

and

class TeacherDetailsSerializer < ActiveModel::Serializer
  attributes :name, :started_work_on, :finished_work_on

  def name
    if object["middle_name]
      "#{object['first_name']} #{object['middle_name']} #{object['last_name']}"
    else
      "#{object['first_name']} #{object['last_name']}"
  end

  def started_work_on
    object["start_date"].rfc2822 if object["start_date"].present?
  end

  def finished_work_on
    object["end_date"].rfc2822 if object["end_date"].present?
  end
end

Maybe the particular example doesn't make sense with JSON column being used, but I hope you get my point.

Of course I could make a separate model for the JSON column where I essentially would be doing the same things as in the serializer, but since all of these actions are pretty much for the API it makes more sense to do it straight in the serializer as putting this stuff in a model and then in a serializer just creates overhead.

[bf4]

@LeKristapino Thanks

The problem is that TeacherDetailsSerialzer.new(object.details) shouldn't work. AMS stands for ActiveModelSerializer. object.details is a hash, a primitive, not a model. You'd need to do something like TeacherDetailsSerialzer.new(TeacherDetails.new(object.details)) to process the details via AMS.

[LeKristapino]

@bf4 Right now my example works just fine (minus the root). As I mentioned, of course, you could make a model for the json first, but then it makes the serializer redundant since all the logic that is currently in the serializer would be in the model.

Wouldn't it be smarter to just support this use-case as well since it does not require any extra coding, just some tests which I could provide a PR with so that this functionality does not break with future impeovements

[bf4]

@LeKristapino I'd be interested in see a PR with a Hash serializer, even if just as a test implementation for example, and not included in lib. It's been brought up before. I don't follow the case myself.

[bmelton-mdsol]

I believe I am having the same issue. Definitely works with 0.10.9, does not work with 0.10.10. I am passing a hash, root is false, attributes adapter. I get NoMethodError (described in the conversation for #2223).
In 0.10.9 (and previous versions for some years prior) it sailed through fine, but now because of the fieldset addition to #serializable_hash, the error is thrown.
This is my first time delving into debugging a gem, so please pardon if this comment is rough around the edges, let me know what more info I can provide.

[bf4]

I'm honestly surprised that a Hash works at all as an input or that it would ever be the intended input object.

[Loschcode]

I've got the same problem going from 0.10.9 to 0.10.10 is there any workaround you guys figured already?

[bmelton-mdsol]

We just took it back down to 0.10.9

[bf4]

@Loschcode @bmelton-mdsol I don't have a failing test to fix. Based on the discussion, looks like y'all were relying on some behavior that should never have existed in the first place, and I'm not even sure what that behavior is. Seems to me that the test case would be something like the below.

Though why I'm trying to figure this out and not any of y'all is beyond me

class AbstractHashSerializer < ActiveModel::Serializer
  def self.attributes(*attrs)
     super.tap do
       attrs.each do |hash_key|
         define_method(hash_key.to_sym) do
           object[hash_key]
         end
       end
     end   
  end
end
class MyHashSerializer < AbstractHashSerializer
  attributes :name
end
object = { name: "Benjamin" }
serializer_instance = MyHashSerializer.new(object)
# 0.10.9
serializer_instance.as_json #=> { name: "benjamin" }
# 0.10.10
serializer_instance.as_json #=> NoMethodError

class MyRootlessHashSerializer < MyHashSerializer
    def initialize(object, options = {})
       super
       self.root = ''
    end
  # or maybe `def json_key; nil; end` dunno
end
object = { name: "Benjamin" }
serializer_instance = MyHashSerializer.new(object)
# 0.10.9
serializer_instance.as_json #=> { name: "benjamin" }
# 0.10.10
serializer_instance.as_json #=> { name: "benjamin" }

[blackst0ne]

@Loschcode

You can use this workaround:

# config/initializers/fix_active_model_serializers.rb

# Remove `root` key for `attributes` adapter.
# Remove this monkey patch once the upstream codebase gets fixed.
# https://github.com/rails-api/active_model_serializers/issues/2350
module ActiveModel
  class Serializer
    def json_key
      root || _type ||
        begin
          object.class.model_name.to_s.underscore
        rescue ArgumentError, NoMethodError
          'anonymous_object'
        end
    end
  end
end

[bf4]

@blackst0ne that is a dangerous fix for anything that might ever use another adapter as discussed above.

you mention

Remove this monkey patch once the upstream codebase gets fixed.

has anyone proposed something? I'm just trying to garden here.

[werleo]

as a workaround, we added

class BaseSerializer < ActiveModel::Serializer
  type ''
end

and all other serializers just inherit from it.
It works for us as we don't have root objects in our json responses.
It was also a small change as we already had BaseSerializer implemented.
I hope it will help someone and I personally would like to see a fix for it.

[JoeWoodward]

I've just updated our app to rails 6 which meant also updating active_model_serializers and also came across this issue. In our case we have some collection serializers that serialize ActiveRecord::ActiveRecord_Relation objects and include custom fields e.g.

class CustomCollectionSerializer < ActiveModel::Serializer
    attributes :active_count, :entries

    def active_count
      object.sum(&:active_count)
    end

    def entries
      object.map do |entry|
        ActiveModelSerializers::SerializableResource.new(entry)
      end
    end
end

The change introduced in #998 changed the way that the root was defined which now prevents non-AR models from working. If root isn't required for the attributes adapter this method should probably just check if root is even required and return early

[bf4]

@JoeWoodward firstly, I agree with this issue which you're commenting on

it's unclear to me what version of AMS you're upgrading since the commit you point to implies you're changing 'major' versions from 0.8 or 0.9, but your example demonstrates some work done in the 0.10 way, but your comment about preventing non-AR models from working ignores that most of the test suite runs non-AR models and support all-over-the-place for non-AR models, including a linter...

so, again, I'm 👍 on someone fixing the Attributes adapter, but I'm not confident that's your precise issue. Hopefully my reply here helps pushing someone who isn't me to propose a PR. otherwise, we're all just agreeing.

[kmsheng]

json_key should not be called when attributes adapter is used.

object.class.model_name.to_s is not likely to get model_name correctly.
For example, instances of ActiveRecord_Relation do not have the method model_name
and it would raise NoMethodError, not ArgumentError

    def json_key
      root || _type ||
        begin
          object.class.model_name.to_s.underscore
        rescue ArgumentError
          'anonymous_object'
        end
    end

https://github.com/rails-api/active_model_serializers/blob/v0.10.12/lib/active_model/serializer.rb#L384