[PATCH] ActiveRecord#touch without validations

Per Pratik's suggestion, I have made a patch JUST to change the .touch to use save(false)

+1

+1

This patch applies cleanly for me on edge. I also feel strongly that touch should not run the validations and there should be a touch! for that case.

+1

+1 for disregarding validations with touch. I also agree with Mike on providing a touch! to run validations and raise save errors.

My only thought on touch in general is that if we're mimicking the *NIX touch command, it seems odd that our touch has the potential of updating more than just the the updated_at (or other given column). By internally calling save at all on the model, aren't we opening ourselves up to:

Post = post.first
post.title = 'Foo'
post.touch
post.reload
post.title
# => 'Foo'

It's probably just me, but that seems quite counter-intuitive, as well. In my mind, touch should only affect the time stamp field(s) and not permanently save other dirty data in the model.

That is definitely an issue. Pratik suggested using update_attribute instead. In that case, it's possible that there will be two database writes if the model has both an updated_at and updated_on field (which is probably a VERY VERY edge case).

I vote for changing touch to use update_attribute to avoid the side-affects of calling #save on a dirty object.

Will saving a record twice mess with optimistic locking?

While your at it, how about the class-level touch, so there is a matching set.

class ActiveRecord::Base
  def self.touch(ids)
    update_all({:updated_at=>Time.now.utc}, :id=>ids)
  end
end

IMO any time save is called I want to know if my record is invalid so that I can do something about it. With this patch/idea it's not quite so obvious that touch doesn't call the validations, which can result in a propagation of invalid objects.

If you really want this functionality I'd suggest you alter #touch to accept false the same way #save does. Or define #touch! and change things accordingly.

+1

Also - touch(:other_column) always also updates :updated_at (Rails 2.3.4). Is it just me? This is not what it is supposed to do... but when I look at the code in "touch" it uses write_attribute, and I'm not deep into Rails' code (only using the API) but according to what I know and tried write_attribute doesn't cause a database write, there's a "save" somewhere else - which explains why :updated_at is updated. But this is completely the wrong behavior, even the current documentation says so!(?)

+1

I was thinking that updated_at field should change anytime the object is modified
successfully whether a touch happens or not. It seems that the issue is that updated_at/on
has a special meaning in Rails to begin with and is causing a level of confusion with how
touch should work. The code as implemented considers two code paths:

touch => updates updated_at and/or updated_on if they exist
touch(:attribute) => updates attribute and implicitly updates updated_at/updated_on if

                   they exist

I agree that there should be touch! running validations and generate appropriate exception.
Also, there should be way to not overwrite fields that are not of the appropriate type when
using touch(:attribute). Maybe, this type of checking is more appropriate for the touch!(:attribute)
form and not touch(:attribute).

Background - why updated_at should not be updated when "touch" has a column as parameter:

People like me use special "something_updated_at" columns for aiding in automatic cache expiration, using modelinstance.something_updated_at.to_s(:number) as one of the arguments in calls to "cache". That's what ActiveRecord does with a model instances cache_key method, but that method always uses updated_at. If I were to always use updated_at of an "Item" even though only its "comments" collection has been updated I'd needlessly expire certain cached fragments. I could just use the updated_at of the particular associated object - but my something_updated_at columns are the equivalent of counter_cache columns.

So I think there are (at least) two very different uses of timestamps:

a) for data consistency, as part of the business logic (when did you place this sorder?)
b) for the purely internal purpose of caching

We have to keep that in mind when we argue - those thinking about the business logic will always vote for validations and the current behavior, those concerned with much lower-level internal code issues are going to vote for what's been said here.

Maybe there even is a legitimate need for different kinds of timestamp columns then? Okay, maybe not yet I guess.

+1 Can this get merged in please? This just bit me in the butt and I think this makes the most sense.

By the way, the above patch, while applying cleanly, gave deprecation messages... so here's the updated patch.

New patch taking comment thread feedback into account. Didn't add a class-level touch method, cause not sure how useful it is.

Seems to me as though Obie's patch is pretty close to the conversation, but I believe it ultimately provides conflicting functionality. If one uses touch to update a dirty model, all that is stored is the touched column, the other dirty attributes remain dirty and unsaved. However, touch!, due to its use of save! would not only update the touched column, but any other dirty attribute in the model will be stored, as well.

So, the result of using the two methods, touch vs touch! in this implementation has a conflicting result. On one hand if you end up with a model.changed? #=> true (with touch) while on the other you'll get model.changed? #=> false.

Would it be more appropriate to implement touch! as:

    def touch!
      raise ActiveRecord::RecordInvalid unless valid?
      touch
    end

Or something very similar?

Patch integrating Obie's submission with a thinner implementation of touch!, utilizing his time stamp tests.

Thinking about this again, I don't like the the fact that touch/touch! aren't equivalent of save/save!, as save and save! both run the validations. So we have two options here :

1) I like Josh Sharpe's suggest to have touch accept false ( which is now :validate => false ). So touch() and touch!() would both run the validations, touch!() would raise an exception on validation failures, while one can use touch(:validate => false) to bypass the validations.

2) Just use update_attribute() for touch() and not add touch! at all.

Pratik, what's the next action on this one? Any ideas?

(from [68ade93cde3fcb3ac9fdfe0541d33d22c2c748d7]) Revert "Changed behavior of touch and added touch! Originally implemented by Obie Fernandez, updated touch! to act as a thin wrapper to touch. [#2520 [PATCH] ActiveRecord#touch without validations state:resolved]"

This reverts commit 3a875e618487a06a56f6cf912cf5440f294921cc.
http://github.com/rails/rails/commit/68ade93cde3fcb3ac9fdfe0541d33d...

I think touch and touch! should both run validations and touch! should raise an exception on validation failure. For bypassing validations, we need to add :validate => false argument to touch() - this way touch() and save() are more aligned.

Apart from validations, I would like to re-raise the issue I posted above once more.

I have several "updated_at" columns for different aspects, to aid in cache-key generation. Those special "SOMETHING_updated_at" columns track the state of associated objects, while pure updated_at tracks the state of the object itself.

When I "touch" one of those associated time-columns the last thing I want is that the object's updated_at is updated! When I say "touch(:items_updated_at)" I want to tell my cache-key generation code that the user's associated items have changed. The user item itself did not change. If the user's updated_at column is changed by "touch" it would indicate that the user object itself changed - which is not true.

See what I wrote above.

I consider my use of timestamp-columns wholly natural. As I wrote above, this is "business logic" vs. "implementation details" - but we HAVE TO do this. An AR object often contains columns for the real application logic, but also columns that are used internally only. If I don't cache associated timestamps with the user (example) I have to do a SQL JOIN each time to load all associated objects and find out if anything changed!

So PLEASE help me and provide a method that lets me update my timestamps WITHOUT also updating "updated_at" of the record in question, if I specify a different column to use by "touch". I would not ask for it if I wasn't convinced my "use case" is completely regular and not an "edge case".

Pratik, what's actionable here? If bugmashers can help, please feel free to tag this with 'bugmash'

-1

it should raise, because is not meant to act as save for other changes you made.

Best would be to call the class method suggested which only updates this single field and no others.

working on this one.

Shouldn't the last patch use

primary_key = self.class.primary_key
self.class.update_all({ name => value }, {primary_key => self[primary_key]})

instead of:

self.class.update_all({ name => value }, {:id => self.id})

?

I think :id automatically is converted to primary_key, but that needs to be checked.

did not work. and that is not good. I see :id at a few places. tonight I will spend some time looking at if I could fix the root cause. If not I'll implement the fix provided by Edgars. Edgars thanks for catching that.

Attached is modified patch based on feedback provided by Edgars.

Hey Neeraj, thanks for the patch! Initially, we agreed upon update_attribute not updating updated_at or updated_on, but there is already a lot of people relying on this behavior for important stuff like touch. So it would be interesting if we don't lose this behavior. Could you please update your patch? Thanks! :)

Attached is modified patch.

Please note that this patch depends on this commit http://github.com/neerajdotname/rails/commit/0db04b94b095b27f6715c0... .

Great Neeraj! Both patches were applied, now waiting for a fix for touch! :) Thanks!

Attached is patch with code change and tests.