Confusing behavior with attr_readonly

rubyonrails-core
1

I've done a little write-up of some confusing behavior around attr_readonly.

The upshot is that I think we should make attr_readonly act more like attr_protected. Failing that, I think we should consider change the behavior of update_attribute to make things a bit less confusing.

Here's the gist:

Thanks, - Trevor

2

Hi -

To your options at the bottom, I might add:

d) silently ignore update_attribute, in the same way that attr_protected silently ignores mass assignment. (to deal with the issue of the obj in memory appearing to be updated)

this seems to be most in keeping with the doc:

"Attributes listed as readonly can be set for a new record, but will be ignored in database updates afterwards."

So, allowing update_attribute to work on this might be somewhat surprising for people who are expecting to the behavior in the current doc. On the other hand, update_attribute not working might be more surprising...

3

So, allowing update_attribute to work on this might be somewhat surprising for people who are expecting to the behavior in the current doc. On the other hand, update_attribute not working might be more surprising...

I think I like option a. It's consistent with attr_protected and allows people who 'know what they're doing' to override that read only setting if they desperately need to, and avoids the horrible case of an object *appearing* updated but not being persisted with the updates applied.

4

Thanks for the replies, guys. I think I like option a, too. I expected update_attribute to work, and I can't imagine that anyone is counting on it not working.

If I don't hear any objections I'll try to work up a patch with doc fixes tomorrow and create a Lighthouse ticket.

Thanks, - Trevor

5

I've done some research and added a new file to the original gist:

http://gist.github.com/70955

You're probably best off viewing it here:

http://gist.github.com/raw/70955/9ccc058386921fefcc9f9d3e6a9ca0b658619e63/gistfile2.txt

The upshot is that I feel changing the behavior of update_attribute may have unexpected consequences, and I believe the short-term solution may be to 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.

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 the options I explored may end up being more problematic than are justified by the issue at hand.

Thanks, - Trevor

6

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 the options I explored may end up being more problematic than are justified by the issue at hand.

I completely agree with your assesment of the issue, create a new exception subclass along the lines of ReadOnlyAttributeError and make it raise. Just be sure that the exception tells you what the read only attribute was ;).

Thanks for spending the time on this.

7

I worked up a patch and created a ticket on Lighthouse here:

http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2132-confusing-behavior-with-attr_readonly

I also did a little more research and found some other methods that might be similarly confusing. Everything is detailed in the ticket.

Thanks, - Trevor