Confusing behavior with attr_readonly

[Trevor Turk]

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:

http://gist.github.com/70955

Thanks,
- Trevor

[Mischa]

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

[Michael Koziarski]

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

--
Cheers

Koz

[Trevor Turk]

On Feb 27, 12:48 pm, Mischa <f.mis...@gmail.com> wrote:
> 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...

On Mar 1, 11:23 pm, Michael Koziarski <mich...@koziarski.com> wrote:
> 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.

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

[Trevor Turk]

On Mar 1, 11:32 pm, Trevor Turk <trevort...@gmail.com> wrote:
> If I don't hear any objections I'll try to work up a patch with doc
> fixes tomorrow and create a Lighthouse ticket.

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

[Michael Koziarski]

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

Koz

[Trevor Turk]

On Mar 2, 3:36 pm, Michael Koziarski <mich...@koziarski.com> wrote:
> 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 ;).

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