[PATCH] counter_cache value gets overwritten

attr_readonly.diff adds an attr_readonly method to prevent the counter_cache from being updated - tests included.

Confirmed that the second patch works and is consider a 1.2 regression bug.

This has always been the case. It's definitely worth investigating, but it's not a 1.2 regression.

I also don't think adding an "attr_readonly" is the best solution.

It would be nice to have readonly attributes for other reasons as well. They are needed for nested set implementations, and I have seen others asking about making attributes readonly on the rails talk list.

I would really like to see something like this patch committed.

Yes, in some application I have a clustering of users that is maintained in bulk updates as a certain graph of contacts evolves. Each user has a cluster_id where he belongs to. I had to be super-defensive to avoid those race conditions in client code. This patch would have been the right solution.

Replying to josh:

I also don't think adding an "attr_readonly" is the best solution.

What do you thing is the best solution? How would you approach this problem?

Replying to josh:

I also don't think adding an "attr_readonly" is the best solution.

It might be true that there are possibly better solutions. However this would be an improvment - what we are talking about here is a bug! And I think the severity of a bug should be definitly higher!

If you might ask why to call this a serious bug: A value that is readonly should never be included in the update-statement - that's why it's set to readonly. And this might be for a good reason. Imagine that this column is updated by the database using PLpgSQL, probably because the particular column has a datatype that is not supported by the ruby database driver like all types of arrays for instance (at least for postgres-pr). In this case this very bug is a NO-GO for Rails. You would either need a hack/workaround or to choose a different framework like STRUTS. I don't think this is where Rails wanno go!

Please don't reject this bugfix just because you think there a better solutions that might still be applied later on.

Thank you!

This patch would be huge for me, and would go a long ways towards enforcing proper encapsulation at the AR level. Any attribute created to cache a computed value (average_score on an Article over many Ratings, etc), for example, should not have a publicly available mutator. Best solution I can think of and vastly better than what I was working on. I'd love to see this committed.

For those of you who need a quick fix, it is worth noting that ActiveRecord already provides perfect read-only behavior, but only for the primary key column!

The crude but (so far) effective hack is to monkey-patch attributes_with_quotes, adding your columns to the line containing 'column.primary', like so:

quoted[name] = quote_value(value, column) unless !include_primary_key && (column.primary || ["protected1", "protected2"].include?(column.name))

I found I also needed to cut-n-paste the 'quote_value' method in to make this work.

I will clean up this patch, but before I do:

• Is there a better solution than attr_readonly ?
• What should be the behavior for the writer?
  • I think the writer should work as normal if the model is a new_record.
  • If not a new record, using the writer could raise an exception or fail validation...

Replying to dcmanges:

* What should be the behavior for the writer? * I think the writer should work as normal if the model is a new_record. * If not a new record, using the writer could raise an exception or fail validation...

This seems like sound behavior to me, though it kind of conflicts with the name "attr_readonly". Maybe "attr_final" or "attr_constant" would be slightly better?

Replying to bn:

Replying to dcmanges:

* What should be the behavior for the writer? * I think the writer should work as normal if the model is a new_record. * If not a new record, using the writer could raise an exception or fail validation...

This seems like sound behavior to me, though it kind of conflicts with the name "attr_readonly". Maybe "attr_final" or "attr_constant" would be slightly better?

Or perhaps "attr_frozen" to go along with the "freeze" method in Ruby?

I believe the best solution is for the automatic callbacks that are generated when counter_cache is used to keep the counter attributes on the instance in synch with the db. Right now all they do is cause an UPDATE to happen, which means that the db is now a step ahead of the instance in play. If you happen to modify that instance after doing the increment and save it, the counter_cache values will get clobbered back to their original value.

Replying to obie:

I believe the best solution is for the automatic callbacks that are generated when counter_cache is used to keep the counter attributes on the instance in synch with the db. Right now all they do is cause an UPDATE to happen, which means that the db is now a step ahead of the instance in play. If you happen to modify that instance after doing the increment and save it, the counter_cache values will get clobbered back to their original value.

Unless locking is involved, you can't keep all instances in sync with the database. I think the best solution is to have a way to exclude an attribute from the update queries (not the counter_cache update queries, but the save/update_attributes queries).

Dan, I know where you're coming from and your solution might be the best, but enhancing that automatic callback the way I described does solve the number one problem caused by this bug and is less controversial:

parent.children.create(blah...)
parent.foo = bar
parent.save! # d'oh, the counter_cache is reset to what it was originally

I think this ticket should be refreshed a bit. If the current patch is not in a state to be applied what needs to be done to get this fixed. There are a couple of good suggestions here. It be nice if core devs gave their opinion on the matter.

+1 on this patch. been using it for a while and does the trick.

(In [7675]) Failing counter cache test. References #6896.

+1: This patch totally saved our app which uses plenty of counter caches to save boatloads of db counting queries. No adverse side-effects at all after a few months live. Can't believe this bug hasn't sent more people screaming to include the patch since it was discovered 9 months ago. Definitely a must for rails2 imho.

+1 on this patch, works perfectly!, changed to verified

(In [7693]) Add attr_readonly to specify columns that are skipped during a normal ActiveRecord #save operation. Closes #6896 [dcmanges]