#2132 Confusing behavior with attr_readonly - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2132 ✓stale

Confusing behavior with attr_readonly

Reported by Trevor Turk | March 5th, 2009 @ 05:22 AM | in 3.x

As discussed here:

http://groups.google.com/group/r...

I've created a patch to raise an exception if update_attribute is given a readonly attribute. Currently, this action appears to work until the model is reloaded, which is confusing.

The patch:

(a) introduces a ReadOnlyAttributeError exception (b) suggests using the update_all class method to update a readonly attribute in the docs (c) changes the remove_readonly_attributes method to leverage a newly created readonly_attributes_include? class method.

I'm more than happy to rework this patch if there are any issues. I think it's all good stuff, but I thought it worth highlighting the changes for sure. In particular, the changes to remove_readonly_attributes may have a slight performance impact, but I think it reads nicer :)

I've also added a 3rd file to this gist, where I've been writing up some of this stuff:

http://gist.github.com/70955

It describes some other areas that are potentially confusing in the same way. Please have a look if you've got the time. I'd be happy to do some more research and write-up stuff, and I think I could put together patches if we agreed that it was worthwhile. The things discussed are:

(a) attribute= + save (b) update_attributes (c) update

I didn't do a full audit, so there may be more, but these methods stood out as worth discussing.

Thank you!

attr_readonly_patch_1.diff 5.2 KB

Comments and changes to this ticket

You flagged this item as spam.
Trevor Turk March 6th, 2009 @ 05:37 PM
I've updated this patch to be less intrusive in terms of changing the remove_readonly_attributes method. I also changed the ReadOnlyAttributeError so that it descends from ActiveRecordError instead of NoMethodError.

I left in the readonly_attributes_include? class method and improved its testing. Perhaps there's a better approach to this, though. Maybe it should be made private? Maybe we don't need it?

Lastly, I realized that ReadOnlyAttributeError probably wouldn't be raised when attempting to update a nested attribute. I didn't test this, and I'm not sure we need to worry about for now, but I'm happy to check into it if we think it's worthwhile.

Thanks!
- attr_readonly_patch_2.diff 6.1 KB
You flagged this item as spam.
Mike Breen August 10th, 2009 @ 12:59 AM
- Tag changed from “activerecord, patch” to “activerecord, bugmash, patch”
verified that the patch applies cleanly to 2-3-stable
You flagged this item as spam.
Elad Meidar August 10th, 2009 @ 01:13 AM
+1 Verified, i got my hands wrapped with this bug for a long time...

Patch failed on master so i attach a clean one.
You flagged this item as spam.
Elad Meidar August 10th, 2009 @ 01:13 AM
Arg.

here's the patch
- 2132-fixed_attr_readonly_for_master.diff 5.8 KB
You flagged this item as spam.
Mike Breen August 10th, 2009 @ 01:18 AM
+1 verified Elad's patch applies cleanly to master. all tests are green
You flagged this item as spam.
dira September 27th, 2009 @ 11:55 AM
+1 - this behavior is a lot less confusing as the previous one

I attached a patch. The previous patch did not apply on master anymore; the attached one does.
- 2132-master-tests-and-patch.diff 5.7 KB
You flagged this item as spam.
Elomar França September 27th, 2009 @ 02:26 PM
+1 for the feature, but the patches does not seem to work on SQLite3:

/test/cases/base_test.rb:937:in test_update_attribute_raises_an_exception_if_given_a_readonly_attribute'

test_update_attribute_raises_an_exception_if_given_a_readonly_attribute(BasicsTest):
ActiveRecord::StatementInvalid: SQLite3::SQLException: posts.body may not be NULL: INSERT INTO "posts" ("title", "body", "taggings_count", "type", "author_id", "comments_count") VALUES('cannot change this', NULL, 0, 'ReadonlyTitlePost', NULL, 0)

Tested in 1.8.7, 1.8.6 and 1.9.1
You flagged this item as spam.
Matías Flores September 27th, 2009 @ 04:43 PM
+1 for the feature, it's less confusing than the current behavior
Rizwan Reza February 12th, 2010 @ 12:46 PM
- Tag changed from “activerecord, bugmash, patch” to “activerecord, patch”
[bulk edit]
Jeremy Kemper May 4th, 2010 @ 06:48 PM
- Milestone changed from “2.x” to “3.x”
[bulk edit]
Santiago Pastorino February 2nd, 2011 @ 04:53 PM
- State changed from “new” to “open”
- Importance changed from “” to “”
This issue has been automatically marked as stale because it has not been commented on for at least three months.

The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.
Santiago Pastorino February 2nd, 2011 @ 04:53 PM
- State changed from “open” to “stale”
You flagged this item as spam.
Michael April 17th, 2011 @ 06:01 PM
I came across this same confusing behavior and eventually ended up here.

I'm getting an error when trying to view the attached patch:
"NoSuchKeyThe specified key does not exist"

Can anyone re-attach?

M