#2520 [PATCH] ActiveRecord#touch without validations - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2520 ✓resolved

[PATCH] ActiveRecord#touch without validations

Reported by Drew | April 19th, 2009 @ 02:53 PM | in 3.0.2

The functionality for the new ActiveRecord#touch is not what I would expect: "If the save fails because of validation errors, an ActiveRecord::RecordInvalid exception is raised"

I feel that least-surprise would be the following:

.touch should not run the validations, but instead make the change no matter what ( just as if you called model.update_attribute(:updated_at, Time.now) )

.touch! should run the validations and raise an error if it fails ( just like .save! )

Comments and changes to this ticket

You flagged this item as spam.
Drew April 19th, 2009 @ 02:54 PM
Per Pratik's suggestion, I have made a patch JUST to change the .touch to use save(false)
- 2520-change_ar_touch.diff 1.7 KB
You flagged this item as spam.
rob April 19th, 2009 @ 04:54 PM
+1
You flagged this item as spam.
Mike Breen April 19th, 2009 @ 08:42 PM
+1
You flagged this item as spam.
Mike Breen April 19th, 2009 @ 09:06 PM
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.
You flagged this item as spam.
Jason Dew April 20th, 2009 @ 01:47 PM
+1
You flagged this item as spam.
Nathaniel Bibler April 20th, 2009 @ 02:53 PM
+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.
You flagged this item as spam.
Drew Blas April 20th, 2009 @ 04:21 PM
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.
You flagged this item as spam.
matt (at pearce) June 5th, 2009 @ 09:16 AM
Will saving a record twice mess with optimistic locking?
You flagged this item as spam.
grosser September 2nd, 2009 @ 05:47 AM
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
```
You flagged this item as spam.
Josh Sharpe September 2nd, 2009 @ 04:40 PM
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.
You flagged this item as spam.
Michael Hasenstein October 16th, 2009 @ 01:31 PM
+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!(?)
You flagged this item as spam.
Conrad Taylor October 16th, 2009 @ 03:01 PM
+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).
You flagged this item as spam.
Michael Hasenstein October 16th, 2009 @ 03:15 PM
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.
You flagged this item as spam.
Mitchell Hashimoto November 3rd, 2009 @ 05:29 PM
+1 Can this get merged in please? This just bit me in the butt and I think this makes the most sense.
Rizwan Reza January 23rd, 2010 @ 11:19 PM
- State changed from “new” to “verified”
- Tag set to “3.0, activerecord, feature, patch, touch, verified”
- Assigned user set to “Pratik”
- Milestone cleared.
I have verified this patch and it applies cleanly to master.

+1 on the feature addition, this makes much more sense.

I talked to Obie over on IRC, he also thinks this should be in for Rails 3.0.
Rizwan Reza January 23rd, 2010 @ 11:21 PM
- Title changed from “Patch - ActiveRecord#touch without validations” to “[PATCH] ActiveRecord#touch without validations”
Rizwan Reza January 23rd, 2010 @ 11:50 PM
By the way, the above patch, while applying cleanly, gave deprecation messages... so here's the updated patch.
- 0001-touch-is-less-surprising-now.patch 744 Bytes
You flagged this item as spam.
Obie January 24th, 2010 @ 12:29 AM
New patch taking comment thread feedback into account. Didn't add a class-level touch method, cause not sure how useful it is.
- 2520-fix-touch-and-add-touch-bang.diff 4 KB
Pratik January 24th, 2010 @ 09:59 AM
- Assigned user changed from “Pratik” to “DHH”
You flagged this item as spam.
Nathaniel Bibler January 24th, 2010 @ 04:00 PM
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?
You flagged this item as spam.
Nathaniel Bibler January 24th, 2010 @ 06:09 PM
Patch integrating Obie's submission with a thinner implementation of touch!, utilizing his time stamp tests.
- 2520-update-touch-bang-as-thin-wrapper.diff 4.1 KB
Pratik January 24th, 2010 @ 11:02 PM
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.
Rizwan Reza March 26th, 2010 @ 08:43 AM
Pratik, what's the next action on this one? Any ideas?
Repository March 27th, 2010 @ 07:35 AM
- State changed from “verified” to “resolved”
(from [3a875e618487a06a56f6cf912cf5440f294921cc]) Changed behavior of touch and added touch! Originally implemented by Obie Fernandez, updated touch! to act as a thin wrapper to touch. [#2520 state:resolved]

Signed-off-by: wycats wycats@gmail.com
http://github.com/rails/rails/commit/3a875e618487a06a56f6cf912cf544...
Repository March 27th, 2010 @ 08:22 AM
(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 state:resolved]"

This reverts commit 3a875e618487a06a56f6cf912cf5440f294921cc.
http://github.com/rails/rails/commit/68ade93cde3fcb3ac9fdfe0541d33d...
Yehuda Katz (wycats) March 27th, 2010 @ 08:25 AM
- State changed from “resolved” to “incomplete”
Pratik March 27th, 2010 @ 08:33 AM
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.
Pratik March 27th, 2010 @ 08:33 AM
- Assigned user changed from “DHH” to “Pratik”
You flagged this item as spam.
Michael Hasenstein March 27th, 2010 @ 09:21 AM
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".
Dan Pickett May 15th, 2010 @ 02:11 AM
Pratik, what's actionable here? If bugmashers can help, please feel free to tag this with 'bugmash'
Jeremy Kemper May 15th, 2010 @ 05:59 PM
-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.
Rizwan Reza May 15th, 2010 @ 06:01 PM
- State changed from “incomplete” to “wontfix”
Jeremy Kemper May 15th, 2010 @ 06:08 PM
- State changed from “wontfix” to “incomplete”
To be clear, +1 on grosser's suggestion of a class method that touches the record without saving other attributes. And revert the touch! method.
Rizwan Reza May 15th, 2010 @ 06:17 PM
- Tag changed from “3.0, activerecord, feature, patch, touch, verified” to “3.0, activerecord, bugmash, feature, patch, touch, verified”
José Valim June 29th, 2010 @ 05:49 PM
- Importance changed from “” to “Medium”
Hey guys, anyone in mood to work on a patch?
Neeraj Singh June 29th, 2010 @ 09:28 PM
working on this one.
Neeraj Singh June 30th, 2010 @ 12:24 PM
- Assigned user changed from “Pratik” to “José Valim”
- State changed from “incomplete” to “open”
attaching a patch which fixes update_attribute. This patch does not fix the issue with this ticket but getting there.
- update_attribute_change.diff 8.8 KB

Edgars Beigarts June 30th, 2010 @ 07:42 PM

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})

José Valim June 30th, 2010 @ 07:49 PM
I think :id automatically is converted to primary_key, but that needs to be checked.
Neeraj Singh June 30th, 2010 @ 07:53 PM
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.
Neeraj Singh July 1st, 2010 @ 03:48 AM
Attached is modified patch based on feedback provided by Edgars.
- 2520_fix.diff 8.9 KB
José Valim July 2nd, 2010 @ 06:49 AM
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! :)
Neeraj Singh July 4th, 2010 @ 04:37 AM
Attached is modified patch.

Please note that this patch depends on this commit http://github.com/neerajdotname/rails/commit/0db04b94b095b27f6715c0... .
- 2520_fix.diff 10.4 KB
José Valim July 8th, 2010 @ 10:01 PM
Great Neeraj! Both patches were applied, now waiting for a fix for touch! :) Thanks!
Neeraj Singh July 9th, 2010 @ 07:15 PM
Attached is patch with code change and tests.
- 2520_fix.diff 4.1 KB
Repository July 13th, 2010 @ 07:37 AM
- State changed from “open” to “resolved”
(from [1d45ea081493cd4eca95cd75cce7be7b8d9cb07c]) with this fix touch method - does not call validations - doest not call callbacks - updates updated_at/on along with attribute if attribute is provided - marks udpated_at/on and attribute as NOT changed

[#2520 state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/1d45ea081493cd4eca95cd75cce7be...
Ryan Bigg October 9th, 2010 @ 09:55 PM
- Tag cleared.
Automatic cleanup of spam.
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
[bulk edit]