update_attribute, update_column and update_columns should not update readonly attributes #8464

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@VadimPushtaev

This is a new version of broken #7688

It was decided two year ago in c96a505 that "update_attribute should not update readonly attributes". I believe it's missleading and should be changed.

I wrote an issue asking whether it's normal or not: rails/rails#7679. As you can see, it was recommended to me to try sending pull request.

Reasons I believe update_attribute, update_column and update_columns should update readonly attributes:

  1. All validations are skipped in this methods. Readonly check really looks like validation.

  2. update_attribute is kind of more 'raw' version of = operator, but in this case I can do

    minivan.color = 'new_color'
    minivan.save

    without getting exceptions, but can't do

    minivan.update_attribute :color, 'new_color'

    It's pretty weird, in this case = behaves more raw than update_attribute.

  3. Readonly control is relatively low-level:

    # Filters the primary keys and readonly attributes from the attribute names.
    def attributes_for_update(attribute_names)
     attribute_names.select do |name|
       column_for_attribute(name) && !pk_attribute?(name) && !readonly_attribute?(name)
     end
    end

    And here is one more oddity: we don't raise exception for pk_attribute, but do for readonly_attribute in update_attribute, update_column and update_columns.

I'm open for discussion. Please, let me know where I'm wrong :)

@VadimPushtaev VadimPushtaev Update readonly attributes
update_attribute, update_column and update_columns
should update readonly attributes
32c4420
@carlosantoniodasilva

Well, removing the check for update_attribute might make sense, because at the end this attribute is not going to be updated right? Since the read only check would skip it.

For the others, though, I'm not sure we should remove the check, because a readonly attribute is not meant to be updated at any time.

@VadimPushtaev

I agree, the check may be reasonable, but it's also reasonable in

 minivan.color = 'new_color'
 minivan.save

case.

And it's also reasonable to check update of id attribute.

Now it behaves different ways in similar situations.

@rafaelfranca
Member

I don't think readonly attributes should be updated in any of these cases.

Also I don't see update_attribute as a raw version of =, it is more like a alternative version that raises exceptions when you are trying to update a readonly attribute.

@VadimPushtaev

@rafaelfranca

I don't think readonly attributes should be updated in any of these cases.

They are not due to

def attributes_for_update(attribute_names)
  attribute_names.select do |name|
    column_for_attribute(name) && !pk_attribute?(name) && !readonly_attribute?(name)
  end
end

I just think updating should not raise exception in all cases. Or it should raise exception in all cases. Current behaviour makes no sense to me.

@neerajdotname
Member

This is no longer an issue in both master and 3-2-stable.

Following test passes in both master and in 3-2-stable .

https://gist.github.com/neerajdotname/5403321

update_column, update_columns and update_attribute raise exception.

save and update_attributes silently ignore readonly attributes.

None of these methods actually update the attribute in the db.

@neerajdotname
Member

@carlosantoniodasilva @rafaelfranca Can this PR be closed ?

@rafaelfranca
Member

Yes. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment