read_attribute(:attr) and self[:attr] not equivalent

[amar47shah]

My team is using RuboCop with Rails cops enabled as a first step in cleaning up a legacy codebase. We found some code like this:

  ...
  after_initialize :set_variations

  def set_variations
    if read_attribute(:page_id) && !self.page_id.nil?
      ...

Among many others, we got this warning from RuboCop:

C: Prefer self[:attr] over read_attribute(:attr).
    if read_attribute(:page_id) && !self.page_id.nil?
       ^^^^^^^^^^^^^^

which refers to this recommendation in the style guide.

And our specs blew up. :( We got several of these:

     ActiveModel::MissingAttributeError:
       missing attribute: page_id
     # activerecord-4.0.13/lib/active_record/attribute_methods/read.rb:95:in `block (2 levels) in read_attribute'
     # activerecord-4.0.13/lib/active_record/attribute_methods/read.rb:94:in `fetch'
     # activerecord-4.0.13/lib/active_record/attribute_methods/read.rb:94:in `block in read_attribute'
     # activerecord-4.0.13/lib/active_record/attribute_methods/read.rb:84:in `fetch'
     # activerecord-4.0.13/lib/active_record/attribute_methods/read.rb:84:in `read_attribute'
     # activerecord-4.0.13/lib/active_record/attribute_methods.rb:345:in `[]'

It turns out that ActiveRecord::AttributeMethods::Read#read_attribute and ActiveRecord::AttributeMethods#[] are not exact synonyms: the latter will raise an ActiveModel::MissingAttributeError if the attribute is not found, whereas the former (I think) just returns nil and fails silently.

It's true that our example is especially gnarly code (read_attribute in an after_initialize callback), but I thought I should at least leave a note here that the two methods can't always be swapped safely. Would it be helpful to reword the style recommendation to make that more clear?

[bquorning]

I agree, #[] used to be an alias of #read_attribute, but that changed in rails/rails#8056.

[kevinelliott]

This issue is starting to age, but it remains a valid concern still. Now that #[] is no longer an alias of #read_attribute (and same with #write_attribute) what is the official recommendation of Ruby Style Guide on this?

[KevinBongart]

A few thoughts:

• the style guide recommendation doesn't make a great job at explaining why one should prefer #[] over #read_attribute and #write_attribute
• at least to me, #read_attribute and #write_attribute are more expressive and readable than brackets
• the #[] method raises an error on missing attributes, which leads to safer code

I'd recommend:

1. adding that information to the style guide recommendation so developers can make an informed decision (don't tell me what to do, tell me why I should do it)
2. keeping the current behavior of Rubocop enforcing #[]: it's a safe default, and developers can either disable the cop globally, locally or rescue ActiveModel::MissingAttributeError errors if the code really has to expect a missing attribute

Happy to make the change to the style guide, just let me know if that's a decision we can agree on!

[filabreu]

Sorry to revisit this, but I recently went into the same problem. I agree we should we remove this from the styleguide. This rule seems kind of random and arbitrary in my point of view.

I always thought that read_attribute is actually better and it is extensively used (specially on Rails).

[johana-star]

@bbatsov Do you support a change like this? It sounds like @KevinBongart would be up to draft new language.

[filabreu]

@strand this is actually very Rails specific, not language... I always prefered the usage of read_attribute. I have solved this issue by disabling this cop.

Even [] uses read_attribute. https://github.com/rails/rails/blob/057ba1280b1a5a33446387b286adb4a75acdebe4/activerecord/lib/active_record/attribute_methods.rb#L384

[johana-star]

@filabreu Apologies for my lack of clarity. I mean that Kevin has indicated interest in rewriting the language used in this section, not anything to do with the core language or its libraries.

[pirj]

@KevinBongart There is no doubt a consensus regarding the necessity of the change.
Would you like to send a PR?

A PR would be sufficient if you refer to the ticket there. I'll close the ticket as the discussion has come to an end.

[vichuge]

Question: Why this issue is open again? I'm here because I would like to know more about this.

[MarcPer]

Since there seems to be an agreement on simply adding a clarification for this guideline, I created a PR for it.
I only modified the read_attribute part, not write_attribute. For the first one, the recommendation was made clear in this discussion.

I don't see a reason to recommend []= over write_attribute though, since it's just an alias. I'd guess it's a matter of consistency ([] is recommended for reading, then []= is recommended for writing).

[MarcPer]

I'd just point out something that wasn't immediately obvious to me, regarding when ActiveModel::MissingAttributeError is raised.

ActiveRecord's documentation has this example:

person = Person.new(name: 'Francesco', age: '22')
person[:name] # => "Francesco"
person[:age]  # => 22

person = Person.select('id').first
person[:name]            # => ActiveModel::MissingAttributeError: missing attribute: name

It's important to note what happens if a random attribute is used:

person[:wrong_attr]                # => nil
person.read_attribute(:wrong_attr) # => nil

So, the exception is raised when the model does have the attribute, but it's not part of the instantiated object, as when select is used. I think even the way the docs phrase it is misleading:

It raises ActiveModel::MissingAttributeError if the identified attribute is missing.

Again, "missing" has a specific meaning here.

[Drenmi]

FWIW, using #[] and #[]= makes this polymorphic with hashes, which is relevant in at least one of the codebases I'm working on. So that might be a reason for.

Notably it's not 100% on par with a hash, since hashes don't raise errors on missing keys.