gistfile1.txt

# confusing behavior with attr_readonly

# setup - we're going to create a user with an attr_readonly attribute: "login"

~/working/$ rails -v
Rails 2.3.0
~/working $ rails rails
~/working $ cd rails
~/working/rails $ script/generate model user login:string
~/working/rails $ rake db:migrate

# app/models/user.rb
class User < ActiveRecord::Base
  attr_readonly :login
end

~/working/rails $ script/console

>> User.create!(:login => 'name')
=> #<User id: 1, login: "name", created_at: "2009-02-26 16:55:40", updated_at: "2009-02-26 16:55:40">
>> u = User.last
=> #<User id: 1, login: "name", created_at: "2009-02-26 16:55:40", updated_at: "2009-02-26 16:56:01">

# ok, we have a user with the login "name" - this attribute is read-only but maybe we can change it with update_attribute? 

>> u.update_attribute(:login, 'changed!')
=> true

# ok, so now the login should be "changed!" right? 

>> u
=> #<User id: 1, login: "changed!", created_at: "2009-02-26 16:55:40", updated_at: "2009-02-26 16:56:26">

# yup, it looks like it's "changed!" but...

>> u.reload
=> #<User id: 1, login: "name", created_at: "2009-02-26 16:55:40", updated_at: "2009-02-26 16:56:26">

# after a reload it's back to "name" - that's confusing!

# here's the difference when using attr_protected instead of attr_readonly for comparison

# app/models/user.rb
class User < ActiveRecord::Base
  attr_protected :login
end

~/working/rails $ script/console

User.create!(:login => 'name')
=> #<User id: 2, login: nil, created_at: "2009-02-26 17:24:09", updated_at: "2009-02-26 17:24:09">
>> u = User.last
=> #<User id: 2, login: nil, created_at: "2009-02-26 17:24:09", updated_at: "2009-02-26 17:24:09">

# ok, so the login is silently ignored...

>> u.update_attribute(:login, 'changed!')
=> true
>> u
=> #<User id: 2, login: "changed!", created_at: "2009-02-26 17:24:09", updated_at: "2009-02-26 17:26:31">

# ok, so it looks like it's been "changed!"

>> u.reload
=> #<User id: 2, login: "changed!", created_at: "2009-02-26 17:24:09", updated_at: "2009-02-26 17:26:31">

# yeah, even after a reload, it's been "changed!"

# EDIT I removed some stuff below this after talking to my coworkers... sounds like the best thing would be:

# (a) allow update_attribute to update an attr_readonly attribute (like attr_protected can)
# (b) have update_attribute return false if trying to update an attr_readonly attribute (and not change the in-memory object's attribute)
# (c) have update_attribute raise an exception if trying to update an attr_readonly attribute

# I think (a) is a good suggestion because it's doubtful that people are relying on update_attribute NOT being able to update an attribute. 

gistfile2.txt

Follow up... 

So, I did some research, and this turns out to be a bit complicated :)

I've come up with some detail of the options I have in mind, and I've tried to explain what the impacts of each might be:

(a) make save(false) not remove readonly attributes. update_attribute calls save(false), which will remove protected attributes by way of the update method, which calls attributes_with_quotes, which removes readonly attributes given the options it's passed. I'm not sure what impacts this might have (or how exactly to do it), so the idea concerns me. 

      # Updates a single attribute and saves the record without going through the normal validation procedure.
      # This is especially useful for boolean flags on existing records. The regular +update_attribute+ method
      # in Base is replaced with this when the validations module is mixed in, which it is by default.
      def update_attribute(name, value)
        send(name.to_s + '=', value)
        save(false)
      end

      # Updates the associated record with values matching those of the instance attributes.
      # Returns the number of affected rows.
      def update(attribute_names = @attributes.keys)
        quoted_attributes = attributes_with_quotes(false, false, attribute_names)
        return 0 if quoted_attributes.empty?
        connection.update(
          "UPDATE #{self.class.quoted_table_name} " +
          "SET #{quoted_comma_pair_list(connection, quoted_attributes)} " +
          "WHERE #{connection.quote_column_name(self.class.primary_key)} = #{quote_value(id)}",
          "#{self.class.name} Update"
        )
      end

      # Returns a copy of the attributes hash where all the values have been safely quoted for use in
      # an SQL statement.
      def attributes_with_quotes(include_primary_key = true, include_readonly_attributes = true, attribute_names = @attributes.keys)
        quoted = {}
        connection = self.class.connection
        attribute_names.each do |name|
          if (column = column_for_attribute(name)) && (include_primary_key || !column.primary)
            value = read_attribute(name)

            # We need explicit to_yaml because quote() does not properly convert Time/Date fields to YAML.
            if value && self.class.serialized_attributes.has_key?(name) && (value.acts_like?(:date) || value.acts_like?(:time))
              value = value.to_yaml
            end

            quoted[name] = connection.quote(value, column)
          end
        end
        include_readonly_attributes ? quoted : remove_readonly_attributes(quoted)
      end

(b) change update_attribute to use the update method directly, change the update method to optionally not remove readonly attributes, and leverage that update can be called in this way. I tried this change and it had some unpleasant test failures due to (I believe) the update method being overridden by validations and callbacks. Additionally, I noticed that the new nested attribute stuff works through update_attribute, so that caused a few test failures. The nested attribute tests can be changed to use update_attributes (with an s) but I'm not sure if we want to take away the ability of update_attribute to work with nested attributes. Some more research and discussion from people familiar with nested attributes might need to be done in order to go this route, but I think might feasible. 

(c) change update attribute as with (b) and also specify only the single attribute passed in as the one to update. This seems like the correct behavior to me, but it has even more potential implications. The thing I like about it is that calling save instead of directly updating the attribute means that any other instance variables that have been changed will be saved as well. This seems unexpected to me (i.e. shouldn't update_attribute simply only update the attribute given?), but perhaps this is the preferred behavior? Again, this would mean making the changes suggested in (b) and more, so maybe it's not a good idea.

(d) simply raise an exception if update_attribute is given a readonly attribute. This may be the easiest solution to implement, but it wouldn't have the 100% desired effect (e.g. allowing update_attribute to work on a readonly attribute). It would be low impact, though, and at least it would notify the user that the action they're trying to take won't work. Also, perhaps we could suggest using update_all for this case, which seems to be the only way to change readonly attributes without directly using sql. 

I'm tempted to work up a patch for (d) at this point. That way, at least the behavior would be clear. The fact that update_attribute appears to work but doesn't actually work is the "bug" in my mind. I'd prefer to make it actually work, but options (a), (b), and (c) may end up being more problematic than are justified by the issue at hand. 

Thanks,
- Trevor

gistfile3.txt

Follow up part 2...

Here are some other confusing points. I'm not sure if this would be a exhaustive list, 
but I could do some more research if we thought it worthwhile. Please take a look 
if you have time. The examples should be pretty self-explanatory. My apologies for 
not realizing these situations would be similarly affected. 

setup

# app/models/user.rb
class User < ActiveRecord::Base
  attr_readonly :login
end

>> User.create!(:login => 'name')
=> #<User id: 2, login: "name", created_at: "2009-03-02 23:01:00", updated_at: "2009-03-02 23:01:00">
>> u = User.last
=> #<User id: 2, login: "name", created_at: "2009-03-02 23:01:00", updated_at: "2009-03-02 23:01:00">

attribute= + save

>> u.login = 'changed'
=> "changed"
>> u
=> #<User id: 2, login: "changed", created_at: "2009-03-02 23:01:00", updated_at: "2009-03-02 23:01:00">
>> u.save
=> true
>> u
=> #<User id: 2, login: "changed", created_at: "2009-03-02 23:01:00", updated_at: "2009-03-02 23:01:14">
>> u.reload
=> #<User id: 2, login: "name", created_at: "2009-03-02 23:01:00", updated_at: "2009-03-02 23:01:14">

Perhaps we wouldn't want to change this behavior? Maybe we could log something if an attribute 
was dropped on a save? I'm not sure what impacts changing attribute= would have. 

update_attributes

>> u.update_attributes({:login => 'changed'}) # attr_readonly
=> true
>> u
=> #<User id: 2, login: "changed", created_at: "2009-03-02 23:01:00", updated_at: "2009-03-02 23:22:42">
>> u.reload
=> #<User id: 2, login: "name", created_at: "2009-03-02 23:01:00", updated_at: "2009-03-02 23:22:42">

I think update_attributes suffers form the same problems as update_attribute, and 
might be worth changing. I wouldn't suggest raising an exception, but note that the behavior 
with attr_protected is slightly different. Perhaps they should be changed to behave in the same way? 

update_attributes (attr_protected)

>> u.update_attributes(:login => 'changed')
=> true
>> u
=> #<User id: 3, login: "abc", created_at: "2009-03-02 23:28:16", updated_at: "2009-03-05 03:47:02">

I think attr_protected is a little better because it doesn't change the in-memory record. 

User.update

This one is similar to update_attributes. Here's with attr_readonly: 

>> User.update(u, :login => 'changed')
=> #<User id: 2, login: "changed", created_at: "2009-03-02 23:01:00", updated_at: "2009-03-02 23:11:00">
>> u
=> #<User id: 2, login: "name", created_at: "2009-03-02 23:01:00", updated_at: "2009-03-02 23:03:40">

...and attr_protected:

>> User.update(u, :login => 'changed')
=> #<User id: 3, login: "abc", created_at: "2009-03-02 23:28:16", updated_at: "2009-03-05 03:47:02">