Add Touch method to ActiveRelation #8343

Open
wants to merge 2 commits into
from

Projects

None yet
@duggiefresh
Contributor

This allows for more efficient touching of many records

@jeremy jeremy and 1 other commented on an outdated diff Nov 27, 2012
activerecord/lib/active_record/relation.rb
+ # Person.all.touch(:created_at)
+ # # => "UPDATE \"persons\" SET \"updated_at\" = '2012-11-27 23:12:25.745411', \"created_at\" = '2012-11-27 23:12:25.745411'"
+ #
+ # # Touch records with scope
+ # Person.where(:name => 'David').touch
+ # # => "UPDATE \"persons\" SET \"updated_at\" = '2012-11-27 23:12:30.087110' WHERE \"persons\".\"name\" = 'David'"
+ def touch(name = nil)
+ attributes = [:updated_at]
+ attributes << name if name
+ now = klass.default_timezone == :utc ? Time.now.utc : Time.now
+ value_hash = attributes.inject({}) do |values, column|
+ values[table[column]] = now
+ values
+ end
+ sql = compile_update(value_hash).to_sql
+ ActiveRecord::Base.connection.execute(sql)
@jeremy
jeremy Nov 27, 2012 Member

Possible to implement using update_all ?

def touch(name = nil)
  name ||= :updated_at
  now = klass.default_timezone == :utc ? Time.now.utc : Time.now
  update_all name => now
end
@duggiefresh
duggiefresh Nov 27, 2012 Contributor

ActiveRecord#touch always updates :updated_at in addition to any optional passed attribute.

My code above emulates this behavior on a batch scale.

@jeremy
jeremy Nov 28, 2012 Member

Do you need to reimplement the update using arel, or can you use update_all?

@duggiefresh
duggiefresh Nov 28, 2012 Contributor

@jeremy Thank you. That shortens the code a bit!

@jeremy jeremy commented on the diff Nov 27, 2012
activerecord/lib/active_record/relation.rb
+ # === Examples
+ #
+ # # Touch all records
+ # Person.all.touch
+ # # => "UPDATE \"persons\" SET \"updated_at\" = '2063-04-04 22:55:23.132670'"
+ #
+ # # Touch multiple records with a custom attribute
+ # Person.all.touch(:created_at)
+ # # => "UPDATE \"persons\" SET \"updated_at\" = '2012-11-27 23:12:25.745411', \"created_at\" = '2012-11-27 23:12:25.745411'"
+ #
+ # # Touch records with scope
+ # Person.where(:name => 'David').touch
+ # # => "UPDATE \"persons\" SET \"updated_at\" = '2012-11-27 23:12:30.087110' WHERE \"persons\".\"name\" = 'David'"
+ def touch(name = nil)
+ attributes = [:updated_at]
+ attributes << name if name
@jeremy
jeremy Nov 27, 2012 Member

This always sets updated_at, so you can't touch(:other_timestamp) without changing updated_at. Not sure if that's a good idea. It's not demonstrated in the test coverage, in any case.

@bcardarella
bcardarella Nov 27, 2012 Contributor

The behavior of this method closely follows the behavior of ActiveRecord#touch as shown here: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/persistence.rb#L338-L357

The only caveat is that the :udpated_at attribute is something that is being derived form an instance method in ActiveRecord which of course is inaccessible in this case.

@jeremy
jeremy Nov 28, 2012 Member

Good point. That feels weird to me too :)

@jeremy
Member
jeremy commented Nov 28, 2012

@duggieawesome got an example of when you've needed this & why? It does feel natural to me but, upon review, I don't have a use in my apps.

@bcardarella
Contributor

@jeremy there are use cases with admin sections when touching a bunch of records have been necessary for our client apps. @btucker has also expressed an interest in this behavior: https://twitter.com/btucker/status/264108386717667328

@jeremy
Member
jeremy commented Nov 28, 2012

Thoughts on optimistic locking?

@bcardarella
Contributor

@jeremy shouldn't that be the responsibility of update_all?

@jeremy
Member
jeremy commented Nov 28, 2012

#touch increments the lock on the record manually. update_all isn't operating on a collection so it can't optimistically lock en masse.

@bcardarella
Contributor

@jeremy I see your point. What is your suggestion, we believe this method makes sense but obviously want to have it implemented in the most responsible way

@bcardarella
Contributor

@jeremy according to: http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html OptimisticLocking requires a lock_version column on the record that ActiveRecord then updates to ensure proper locking between multiple instances. Is the concern to ensure the optimistic locking is still occurring and increment the lock_version if present? If that is the case can we just add a incr on that column if it is present? This would require something like:

update users
set updated_at = #{Time.now},
lock_version = lock_version + 1

But again, I feel this would be the responsibility of update_all

@btucker
Contributor
btucker commented Nov 28, 2012

Thanks for working on this @duggieawesome & @bcardarella!

#touch increments the lock on the record manually

My opinion, actually, is that having #touch in any context increment the lock version is unexpected.

this is the situations I would want to avoid:

class Person < AR::B
  belongs_to :group, touch: true
end

g = Group.find(1)
p = g.persons.first

p.name = "new name"
p.save

g.name = "new group name"
g.save # raises ActiveRecord::StaleObjectError

I haven't actually used Optimistic Locking in Rails, but am I correct in my code reading that the above currently happens?

@duggiefresh
Contributor

Hey @jeremy any updates to this issue?

Thank you.

@lukaszx0
Member

@jeremy any updates?

@arunagw
Member
arunagw commented Jun 21, 2013

@duggiefresh Hey, Rebase is also required with master for this Pull Request.

@dgalarza
Contributor

Any updates on this feature? I recently started trying to put something similar together because I hadn't noticed a pull request was opened for it already. It's very useful when using key-based caching to expire a large number of records at once.

@bcardarella
Contributor

@duggiefresh rebase por favor

duggiefresh added some commits Nov 27, 2012
@duggiefresh duggiefresh Add Touch method to ActiveRelation
This will allow for a more efficient touch on multiple records, handled
by the database rather than iterating objects in Ruby
fd9853e
@duggiefresh duggiefresh Create an update_on field for developers.yml
This is a required addition for activerecord/test/cases/integration_test.rb
32ae453
@dgalarza
Contributor
dgalarza commented Oct 4, 2013

I'm also curious if it might follow convention more closely if the method was named touch_all rather than touch. It utilizes update_all as it is and would more closely represent the relationship between them. It also follows the convention of update_all, destroy_all, etc..

@leafac
Contributor
leafac commented Dec 23, 2013

I see this PR has been stale for a while. Can I help making any progress?

@duggiefresh
Contributor

Thanks. @leafac I've got time during this holiday to work on this. Feel free to pull it down and check it out.

@exviva exviva commented on the diff Dec 23, 2013
activerecord/lib/active_record/relation.rb
+ # Touches multiple records.
+ #
+ # === Examples
+ #
+ # # Touch all records
+ # Person.all.touch
+ # # => "UPDATE \"persons\" SET \"updated_at\" = '2063-04-04 22:55:23.132670'"
+ #
+ # # Touch multiple records with a custom attribute
+ # Person.all.touch(:created_at)
+ # # => "UPDATE \"persons\" SET \"updated_at\" = '2012-11-27 23:12:25.745411', \"created_at\" = '2012-11-27 23:12:25.745411'"
+ #
+ # # Touch records with scope
+ # Person.where(:name => 'David').touch
+ # # => "UPDATE \"persons\" SET \"updated_at\" = '2012-11-27 23:12:30.087110' WHERE \"persons\".\"name\" = 'David'"
+ def touch(name = nil)
@exviva
exviva Dec 23, 2013 Contributor

Shouldn't it be called touch_all, for consistency with update_all, destroy_all, etc.?

@rafaelfranca
Member

I think we should address the optimistic locking issue on this method. touch at instance level increment it, so lets keep the same behaviour at the class method.

@repinel
Contributor
repinel commented Jun 13, 2015

@duggiefresh Anything I can help you with on this PR?

@oesgalha
Contributor

Any updates on this PR? Can I help on anything? This feature would be handy. :)

@duggiefresh
Contributor

@oesgalha please feel free to own the PR :)

Thanks for checking in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment