This project is archived and is in readonly mode.

#2132 ✓stale
Trevor Turk

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!

Comments and changes to this ticket

  • Trevor Turk

    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!

  • Mike Breen

    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

  • Elad Meidar

    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.

  • Elad Meidar
  • Mike Breen

    Mike Breen August 10th, 2009 @ 01:18 AM

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

  • dira

    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.

  • Elomar França

    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

  • Matías Flores

    Matías Flores September 27th, 2009 @ 04:43 PM

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

  • Rizwan Reza

    Rizwan Reza February 12th, 2010 @ 12:46 PM

    • Tag changed from activerecord, bugmash, patch to activerecord, patch
  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • Santiago Pastorino

    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

    Santiago Pastorino February 2nd, 2011 @ 04:53 PM

    • State changed from “open” to “stale”
  • Michael

    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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages