prevent ActiveModel::Serializer::Fieldset.fields_for crashing in case…

[aovertus]

Purpose

Current AMS implementation allows things like this:

render json: users, each_serializer: UserSerializer, root: :users

but the new fieldset class don't accept anything else than string for #fields_for

Changes

Allow #fields_for to receive a symbol and parse it to string before any manipulation

value is coming from #json_key method

Caveats

Related GitHub issues

Additional helpful information

[wasifhossain]

Hi @aovertus thanks for your PR 👍

btw it has already been merged as part of #2344. closing it anyways

[aovertus]

@wasifhossain should not it be part of a new tag ? fetching the v0.10.10 will not provide the fix 🤔

We're using dependabot which try to retrieve the newest tag.

[wasifhossain]

@bf4 can you please suggest the best step that we can take here. (#2343 (comment) as a reference)

thanks

[bf4]

@wasifhossain 0.10.11 is fine as is 0.10.10.1, which is a thing. I don't have a strong feeling

[aovertus]

@bf4 @wasifhossain can we settle on a tag so the update is available ? Since its a fix I would go for 0.10.10.1

[wasifhossain]

I would also prefer to tag it as 0.10.10.1, but the fact is, unless we create a new branch from 0-10-stable, it does not really matter whether we label it as any of those; the next tags released after this proposed tag will make way to all projects having this gem version defined as '~> 0.10.10'.

another point that I would like to address before we make the release is #2350 (comment). otherwise we will continue to receive relevant issues

I agree that the attributes adapter has no need to call root

also we received a good number of issues most recently resulted from lib/active_model/serializer.rb#L387, which I think is due to lack of guide that tells about the requirement on how to define the PORO.

docs/howto/serialize_poro.md explains it nicely, but the linter only mentions that its required if caching is enabled.

I think we should highlight the point for the PORO that it should respond to model_name and be an instance of ActiveModel::Name. In that case, :attributes adapter can work fine without the need to have root set.

@bf4 any thoughts, please?

[bf4]

• 0.10.10.1 is fine
• Should make an issue that the attributes adapter shouldn't invoke json_key
• Shouldn't require the poro to include ActiveModel::Name as the linter intended. I'd be in favor of rescuing the NoMethodError in json_key and re-raising it with a message with the gist "no root specified and none could be inferred"

[wasifhossain]

regarding item 2 above:

https://github.com/rails-api/active_model_serializers/blob/0-10-stable/lib/active_model_serializers/adapter/attributes.rb#L8

instance_options[:fieldset] ||= ActiveModel::Serializer::Fieldset.new(fields_to_fieldset(instance_options.delete(:fields)))

right now, instance_options[:fieldset] is always assigned an instance of ActiveModel::Serializer::Fieldset whether instance_options[:fields] is present or not, causing if (fieldset = adapter_options[:fieldset]) to always pass and fieldset.fields_for(json_key) to be called every time.

rather we can set an instance of ActiveModel::Serializer::Fieldset to instance_options[:fieldset] only when instance_options[:fields] is present. this will make the json_key called only when the fields option is present.

still that would make the attributes adapter to depend on either of root, _type or a class that responds to model_name in order to take the benefit of the Fieldset class.

[wasifhossain]

i agree that we can work on a PR to make the attributes adapter not invoke json_key at all. should I give it a try?

[aovertus]

Nice! Let me know if I can be of anyway help.

[wasifhossain]

@aovertus thanks for your hands!

btw I need to take a dive into the internals and lets see what I can come up with in next few days.

[bf4]

I think a first-level fix could be

-        instance_options[:fieldset] ||= ActiveModel::Serializer::Fieldset.new(fields_to_fieldset(instance_options.delete(:fields)))
+        if (fields_options = instance_options.delete(:fields)))
+          instance_options[:fieldset] ||= ActiveModel::Serializer::Fieldset.new(fields_to_fieldset(fields_options))
+        end

so that it's only even possible to get a fieldset when there are field options present

secondly, we need to think through how a fieldset in should even work without a json_key. My feeling is, you can't have your cake and eat it too. If you want to use a fieldset, you need a 'json_key'.

I think if we make the above change, then we can raise a runtime error

changing

      if (fieldset = adapter_options[:fieldset])
        options[:fields] = fieldset.fields_for(json_key)
      end

to

      if (fieldset = adapter_options[:fieldset])
        options[:fields] = fields_for!(fieldset)
      end

.....

def fields_for!(fieldset)
  fieldset.fields_for(fieldset_type)
end

def fieldset_type
  json_key
rescue NoMethodError
  raise NoMethodError, "Cannot determine fieldset type for serializer instance class=#{self.class.name} object=#{object.class.name}"
end

[wasifhossain]

you can't have your cake and eat it too

😄

[kddnewton]

We were burned by this as well when we tried to upgrade. In case anyone ends up here because there's no new version out yet, here's the fix I'm using locally in an initializer:

ActiveModel::Serializer::Fieldset.prepend(
  Module.new do
    def fields_for(type)
      super(type.to_s)
    end
  end
)