Opt Out of Virtual Attributes

[nrowegt]

I ran into some unexpected behavior, namely the existence of virtual attributes. I was operating under the assumption that, like Rails, if an attribute was not declared (exists in the DB for Rails, declared as a field for a Dynamoid::Document), that I'd receive an ActiveModel::UnknownAttributeError. However, this is not the case.

Would you be open to making virtual attributes be an opt-out behavior (per model, or global config), so that attempting to initialize an object with an undeclared attribute would act as expected? The one thing I've not checked is if there would be ramifications for load objects from Dynamo which have fields on the actual document which lives in AWS which are not declared in our models.

Looking for thoughts and input.

https://github.com/Dynamoid/dynamoid/blame/master/lib/dynamoid/document.rb#L210

[andrykonchin]

First of all, I am not against introducing a new configuration option if it helps. And compatibility with Rails is quite important.

On the other hand as far as I understand, Rails doesn't raise ActiveModel::UnknownAttributeError for a virtual attribute during mass-assignment. By "virtual attribute" I mean a public setter method.

I've checked this behavior in Rails 5.2 and Rails 6.1. Rails raises the exception only if there is no such a column in a table and no public setter in a model (e.g. foo=):

      def _assign_attribute(k, v)
        setter = :"#{k}="
        if respond_to?(setter)
          public_send(setter, v)
        else
          raise UnknownAttributeError.new(self, k.to_s)
        end
      end

https://github.com/rails/rails/blob/6-1-stable/activemodel/lib/active_model/attribute_assignment.rb#L46-L53

[nrowegt]

The primary behavior difference here is that Rails requires the setter to be explicitly defined or else it raises an error, whereas Dynamoid will continue working should no setter be defined.

    def load(attrs)
      attrs.each do |key, value|
        send("#{key}=", value) if respond_to?("#{key}=")
      end
    end

A patch like this would be the lowest touch option that comes to mind for me:

def load(attrs)
  attrs.each do |key, value|
    if respond_to?("#{key}=")
	 send("#{key}=", value) 
    else
    	raise UnknownAttributeError if Dynamoid::Config.raise_error_on_undefined_attribute # default false
  end
end

https://github.com/Dynamoid/dynamoid/blob/master/lib/dynamoid/loadable.rb#L9

I've also used Dynamoid for years and this has never been a blocker or impediment. I think it would provide a little added safety, but it's far from a blocker for usage.

[andrykonchin]

Ah, I see what you meant. So Dynamoid should raise an exception when a user tries to assign an attribute value (with a .new method call) when there is no setter method for it. I completely agree that it should be changed.

The #initialize method is probably the only way for a user to reproduce this issue (I consider the method #load as a private one). All the other places use the #write_attribute method that handles this situation properly.

So the proper and the simplest way to fix the issue would be just to use #write_attribute instead of #load in the #initialize method.

Regarding a configuration option. On the one hand, it's a breaking change that may lead to unexpected (for a user) exceptions. On the other hand, we can consider it as a bug fix - because it's an existing behavior to raise an exception, e.g. in #update_attributes, and introducing a new configuration option may look like overkill. Moreover, this new option should affect only #initialize method, and shouldn't affect #update_attributes/#update_attribute/#write_attribute. So it should be named something like this "raise_error_on_undefined_attribute_in_initialize".

So does it make sense not to introduce a new configuration option?

Nevertheless, thank you for pointing at this inconsistency. I am open to a PR with a fix. If you don't have a chance to look at it I will add this issue to the TODO list and fix it in the next release.