Serializer.attributes fails with unclear message when an attribute is `:method`

[dongtong]

Expected behavior vs actual behavior

If attributes list has keyword, the serializer will be failed, the error is

Rendered ActiveModel::Serializer::CollectionSerializer with ActiveModelSerializers::Adapter::Attributes ArgumentError: wrong number of arguments (given 0, expected 1)

class ApiSerializer < ApplicationSerializer
  attributes :id,
             :group_name,
             :name,
             :endpoint,
             :method,
             :version,
             :created_at,
             :updated_at,
             :description
  #...
end

Solutions

class ApiSerializer < ApplicationSerializer
  attributes :id,
             :group_name,
             :name,
             :endpoint,
             :version,
             :created_at,
             :updated_at,
             :description
  attribute :method

  def method
     object.method
  end
end

I think active_model_serializers should tell the developer that attributes should not include keywords, otherwise, the developer will not know the error's detail information. If I have time, I will create PR, but I want the library creator could improve this issue.

Thanks a lot

[bf4]

@dongtong Thanks for the report. It would be great if you could submit a PR to improve the error message. We actually added a 'block format' for attributes for just this scenario:

class ApiSerializer < ApplicationSerializer
  attribute :method do
    object.method
  end

I'm not sure of a good way to warn people not to use words that might already be defined methods like select and method. We could check if the method is defined in the serializer, but it's probably easier to just fail early with a good message.

[9mm]

Another hour wasted with this gem before I find this issue. I really regret ever using it

[bf4]

@9mm not gem's fault. We can make it harder to have this problem in gem, but is ultimately devs needing to avoid names that are already defined: method, select, display etc. If you didn't experience this problem in ams you'd find it elsewhere. Same reason we always use self with setters.

[xlts]

@bf4 I'm not sure if using keywords or "names that are already defined" is the issue here; it's more about what these definitions are. Errors when calling method or select from the serializer are raised because methods require arguments and/or a block (ArgumentError). In case of methods with no (or default) arguments serialization works fine, even when using reserved words as attribute names.

The problem here is to distinguish which methods can be called and which cannot. I created a more-or-less naive fix (https://github.com/xlts/active_model_serializers/blob/invalid-attribute-name/lib/active_model/serializer.rb#L379) which works fine... until the model's read_attribute_for_serialization is called in when using attributes such as select. As it turns out, for this method ActiveModel::Serialization simply defines an alias to send (see https://github.com/rails/rails/blob/92703a9ea5d8b96f30e0b706b801c9185ef14f0e/activemodel/lib/active_model/serialization.rb#L168), pretty much repeating this gem's logic. So I guess the ultimate fix will not be that simple considering the dependency on ActiveModel. In any case, I'd happily provide a solution if we think of one.

[bf4]

@xlts In 0.10.6 I undef'd display and select. I considered undeffing method more dangerous. A lot of code could depend on that.

In my dev work I'm inheriting from BasicObject, but even BasicObject has method. The fix that I made in dev is that attribute declaration creates an instance method that delegates to the instance object, so it would create a method def method; object.method; end which would work as long as no one needs to use Kernel.method. Calling thing method, in general, is asking for trouble, from my own experience.

[xlts]

@bf4 allright, that makes sense and seems much less error-prone. So I guess this issue is more or less dealt with.

[bf4]

@xlts Thanks for your understanding. I wish code always just did what we intended :) Let us know what you end up doing.

[xlts]

@bf4 I suppose I'll wait for a new release as you tackled the issue already. Unless there is something else I can help with?

[bf4]

I'll wait for a new release

Or just define your attribute with a block attribute(:method) { object.method }.

Don't wait the the release :)

[xlts]

@bf4 Obviously that's what I'm doing :). I meant I'm waiting for the general solution, that's all.

[beauby]

IMO the reason for this behavior is poor design. A possible fix would be for AMS to automatically define a dummy method that just forwards to the object whenever an attribute is declared without a block. It would be dangerous as well, but much more consistent with the idea that "attributes are methods on the serializer", compared to the current concept of "attributes are something, unless a method is defined, in which case they are methods".

[bf4]

A possible fix would be for AMS to automatically define a dummy method that just forwards to the object whenever an attribute is declared without a block

So, now it defines a method unless one exists. You're suggesting that it always define an instance method. It could still be overridden, so I think that would work fine. It would mostly be a problem with inheritance if that same attribute were declared more than once.