Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

Confusing behavior with attr_readonly

#2132

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!

Reported by Trevor Turk · March 5th, 2009 @ 05:22 AM

State: stale
Milestone: 3.x
Assigned to: Michael Koziarski Michael Koziarski
Importance: none

Activity

  1. Trevor Turk
    Trevor Turk

    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!

    March 6th, 2009 @ 05:37 PM

  2. Mike Breen
    Mike Breen
    • Tag changed from activerecord, patch to activerecord, bugmash, patch

    verified that the patch applies cleanly to 2-3-stable

    August 10th, 2009 @ 12:59 AM

  3. Elad Meidar
    Elad Meidar

    +1 Verified, i got my hands wrapped with this bug for a long time...

    Patch failed on master so i attach a clean one.

    August 10th, 2009 @ 01:13 AM

  4. Elad Meidar
    Elad Meidar

    Arg.

    here's the patch

    August 10th, 2009 @ 01:13 AM

  5. Mike Breen
    Mike Breen

    +1 verified Elad's patch applies cleanly to master. all tests are green

    August 10th, 2009 @ 01:18 AM

  6. dira
    dira

    +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.

    September 27th, 2009 @ 11:55 AM

  7. Elomar França
    Elomar França

    +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

    September 27th, 2009 @ 02:26 PM

  8. Matías Flores
    Matías Flores

    +1 for the feature, it's less confusing than the current behavior

    September 27th, 2009 @ 04:43 PM

  9. Rizwan Reza
    Rizwan Reza
    • Tag changed from activerecord, bugmash, patch to activerecord, patch

    February 12th, 2010 @ 12:46 PM

  10. Jeremy Kemper
    Jeremy Kemper
    • Milestone changed from 2.x to 3.x

    May 4th, 2010 @ 06:48 PM

  11. Santiago Pastorino
    Santiago Pastorino
    • 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.

    February 2nd, 2011 @ 04:53 PM

  12. Santiago Pastorino
    Santiago Pastorino
    • State changed from open to stale

    February 2nd, 2011 @ 04:53 PM

  13. Michael
    Michael

    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

    April 17th, 2011 @ 06:01 PM