:remove attribute leads to resource deletion

[begor]

Hi!

When I have a resource with

attributes :a, :b, :remove

every time I fetch them, jsonapi ACTUALLY REMOVES them one by one from database with following trace:

D, [2017-07-20T14:35:16.578563 #25926] DEBUG -- :   SQL (0.2ms)  DELETE FROM `...` WHERE `...`.`id` = 42
I, [2017-07-20T14:35:16.578784 #25926]  INFO -- : #9:   .../vendor/bundle/ruby/2.1.0/gems/jsonapi-resources-0.8.1/lib/jsonapi/resource.rb:216:in `_remove'
#10:    .../vendor/bundle/ruby/2.1.0/gems/jsonapi-resources-0.8.1/lib/jsonapi/resource.rb:66:in `block in remove'
#14:    .../vendor/bundle/ruby/2.1.0/gems/jsonapi-resources-0.8.1/lib/jsonapi/resource.rb:65:in `remove'
#15:    .../vendor/bundle/ruby/2.1.0/gems/jsonapi-resources-0.8.1/lib/jsonapi/resource_serializer.rb:156:in `public_send'
#16:    .../vendor/bundle/ruby/2.1.0/gems/jsonapi-resources-0.8.1/lib/jsonapi/resource_serializer.rb:156:in `block in attributes_hash'

[senid231]

@lgebhardt maybe attribute :remove should raise an exception?

ActiveRecord::Base raises ActiveRecord::DangerousAttributeError: destroy is defined by Active Record. Check to make sure that you don't have an attribute or method with the same name. when a table has a column with name destroy

[lgebhardt]

I think we should add :remove and :destroy to check_reserved_attribute_name.

[lgebhardt]

Actually we should exclude any method names that exist on Resource or ActiveRecord::Base. Maybe like:

def check_reserved_attribute_name(attr)
  name = attr.to_sym
  # Allow :id since it can be used to specify the format. Since it is a method on the base Resource
  # an attribute method won't be created for it.
  if name != :id &&
      ([:type, :types].include?(name) ||
      JSONAPI::Resource.instance_methods.include?(name) ||
      ActiveRecord::Base.instance_methods.include?(name))
    warn "[NAME COLLISION] `#{name}` is a reserved key in #{_resource_name_from_type(_type)}."
  end
end

[senid231]

@lgebhardt good idea, but I think we should not generate warnings for collision with ActiveRecord::Base instance methods (or maybe do that only when _model_class is subclass of ActiveRecord::Base

collision with JSONAPI::Resource instance methods should raise exception instead of warning (and it should be run only on registration attributes/relationships)

[lgebhardt]

@senid231 check_reserved_attribute_name is only being called during registration of attributes. I agree changing from a warning to an exception would be better. Also agree that we should only check against ActiveRecord::Base when _model_class is subclass of ActiveRecord::Base.

[senid231]

@lgebhardt what about checking relationship name in the same way, that we will do for attributes?

because relationships registration adds instance methods too so we should check them before define_method call

[lgebhardt]

@senid231 Yes, we should update check_reserved_relationship_name in a similar manner.