Skip to content
Permalink
Browse files

Fix `touch` option to behave consistently with `Persistence#touch` me…

…thod

`touch` option was added to `increment!` (#27660) and `update_counters`
(#26995). But that option behaves inconsistently with
`Persistence#touch` method.

If `touch` option is passed attribute names, it won't update
update_at/on attributes unlike `Persistence#touch` method.

Due to changed from `Persistence#touch` to `increment!` with `touch`
option, #31405 has a regression that `counter_cache` with `touch` option
which is passed attribute names won't update update_at/on attributes.

I think that the inconsistency is not intended. To get back consistency,
ensure that `touch` option updates update_at/on attributes.
  • Loading branch information...
kamipo committed Jun 9, 2018
1 parent a865f62 commit cad0b7d91dbc5abf4b0d7fdbcf2d0620557a3b0f
@@ -1,3 +1,7 @@
* Fix `touch` option to behave consistently with `Persistence#touch` method.

*Ryuta Kamizono*

* Migrations raise when duplicate column definition.

Fixes #33024.
@@ -47,8 +47,12 @@ def reset_counters(id, *counters, touch: nil)
reflection = child_class._reflections.values.find { |e| e.belongs_to? && e.foreign_key.to_s == foreign_key && e.options[:counter_cache].present? }
counter_name = reflection.counter_cache_column

updates = { counter_name.to_sym => object.send(counter_association).count(:all) }
updates.merge!(touch_updates(touch)) if touch
updates = { counter_name => object.send(counter_association).count(:all) }

if touch
names = touch if touch != true
updates.merge!(touch_attributes_with_time(*names))
end

unscoped.where(primary_key => object.id).update_all(updates)
end
@@ -68,8 +72,8 @@ def reset_counters(id, *counters, touch: nil)
# * +counters+ - A Hash containing the names of the fields
# to update as keys and the amount to update the field by as values.
# * <tt>:touch</tt> option - Touch timestamp columns when updating.
# Pass +true+ to touch +updated_at+ and/or +updated_on+. Pass a symbol to
# touch that column or an array of symbols to touch just those ones.
# If attribute names are passed, they are updated along with updated_at/on
# attributes.
#
# ==== Examples
#
@@ -107,7 +111,8 @@ def update_counters(id, counters)
end

if touch
touch_updates = touch_updates(touch)
names = touch if touch != true
touch_updates = touch_attributes_with_time(*names)
updates << sanitize_sql_for_assignment(touch_updates) unless touch_updates.empty?
end

@@ -171,13 +176,6 @@ def increment_counter(counter_name, id, touch: nil)
def decrement_counter(counter_name, id, touch: nil)
update_counters(id, counter_name => -1, touch: touch)
end

private
def touch_updates(touch)
touch = timestamp_attributes_for_update_in_model if touch == true
touch_time = current_time_from_proper_timezone
Array(touch).map { |column| [ column, touch_time ] }.to_h
end
end

private
@@ -393,10 +393,7 @@ def update_all(updates)
# Person.where(name: 'David').touch_all
# # => "UPDATE \"people\" SET \"updated_at\" = '2018-01-04 22:55:23.132670' WHERE \"people\".\"name\" = 'David'"
def touch_all(*names, time: nil)
attributes = Array(names) + klass.timestamp_attributes_for_update_in_model
time ||= klass.current_time_from_proper_timezone
updates = {}
attributes.each { |column| updates[column] = time }
updates = touch_attributes_with_time(*names, time: time)

if klass.locking_enabled?
quoted_locking_column = connection.quote_column_name(klass.locking_column)
@@ -53,19 +53,21 @@ def initialize_dup(other) # :nodoc:
end

module ClassMethods # :nodoc:
def timestamp_attributes_for_update_in_model
timestamp_attributes_for_update.select { |c| column_names.include?(c) }
end

def current_time_from_proper_timezone
default_timezone == :utc ? Time.now.utc : Time.now
def touch_attributes_with_time(*names, time: nil)
attribute_names = timestamp_attributes_for_update_in_model
attribute_names |= names.map(&:to_s)
attribute_names.index_with(time ||= current_time_from_proper_timezone)
end

private
def timestamp_attributes_for_create_in_model
timestamp_attributes_for_create.select { |c| column_names.include?(c) }
end

def timestamp_attributes_for_update_in_model
timestamp_attributes_for_update.select { |c| column_names.include?(c) }
end

def all_timestamp_attributes_in_model
timestamp_attributes_for_create_in_model + timestamp_attributes_for_update_in_model
end
@@ -77,6 +79,10 @@ def timestamp_attributes_for_create
def timestamp_attributes_for_update
["updated_at", "updated_on"]
end

def current_time_from_proper_timezone
default_timezone == :utc ? Time.now.utc : Time.now
end
end

private
@@ -116,15 +122,15 @@ def timestamp_attributes_for_create_in_model
end

def timestamp_attributes_for_update_in_model
self.class.timestamp_attributes_for_update_in_model
self.class.send(:timestamp_attributes_for_update_in_model)
end

def all_timestamp_attributes_in_model
self.class.send(:all_timestamp_attributes_in_model)
end

def current_time_from_proper_timezone
self.class.current_time_from_proper_timezone
self.class.send(:current_time_from_proper_timezone)
end

def max_updated_column_timestamp(timestamp_names = timestamp_attributes_for_update_in_model)
@@ -280,38 +280,38 @@ class ::SpecialReply < ::Reply
end

test "update counters with touch: :written_on" do
assert_touching @topic, :written_on do
assert_touching @topic, :updated_at, :written_on do
Topic.update_counters(@topic.id, replies_count: -1, touch: :written_on)
end
end

test "update multiple counters with touch: :written_on" do
assert_touching @topic, :written_on do
assert_touching @topic, :updated_at, :written_on do
Topic.update_counters(@topic.id, replies_count: 2, unique_replies_count: 2, touch: :written_on)
end
end

test "reset counters with touch: :written_on" do
assert_touching @topic, :written_on do
assert_touching @topic, :updated_at, :written_on do
Topic.reset_counters(@topic.id, :replies, touch: :written_on)
end
end

test "reset multiple counters with touch: :written_on" do
assert_touching @topic, :written_on do
assert_touching @topic, :updated_at, :written_on do
Topic.update_counters(@topic.id, replies_count: 1, unique_replies_count: 1)
Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: :written_on)
end
end

test "increment counters with touch: :written_on" do
assert_touching @topic, :written_on do
assert_touching @topic, :updated_at, :written_on do
Topic.increment_counter(:replies_count, @topic.id, touch: :written_on)
end
end

test "decrement counters with touch: :written_on" do
assert_touching @topic, :written_on do
assert_touching @topic, :updated_at, :written_on do
Topic.decrement_counter(:replies_count, @topic.id, touch: :written_on)
end
end
@@ -445,32 +445,38 @@ def test_counter_cache_with_touch_and_lock_version
assert_equal 0, car.wheels_count
assert_equal 0, car.lock_version

previously_car_updated_at = car.updated_at
travel(2.second) do
previously_updated_at = car.updated_at
previously_wheels_owned_at = car.wheels_owned_at
travel(1.second) do
Wheel.create!(wheelable: car)
end

assert_equal 1, car.reload.wheels_count
assert_not_equal previously_car_updated_at, car.updated_at
assert_equal 1, car.lock_version
assert_operator previously_updated_at, :<, car.updated_at
assert_operator previously_wheels_owned_at, :<, car.wheels_owned_at

previously_car_updated_at = car.updated_at
travel(1.day) do
previously_updated_at = car.updated_at
previously_wheels_owned_at = car.wheels_owned_at
travel(2.second) do
car.wheels.first.update(size: 42)
end

assert_equal 1, car.reload.wheels_count
assert_not_equal previously_car_updated_at, car.updated_at
assert_equal 2, car.lock_version
assert_operator previously_updated_at, :<, car.updated_at
assert_operator previously_wheels_owned_at, :<, car.wheels_owned_at

previously_car_updated_at = car.updated_at
travel(2.second) do
previously_updated_at = car.updated_at
previously_wheels_owned_at = car.wheels_owned_at
travel(3.second) do
car.wheels.first.destroy!
end

assert_equal 0, car.reload.wheels_count
assert_not_equal previously_car_updated_at, car.updated_at
assert_equal 3, car.lock_version
assert_operator previously_updated_at, :<, car.updated_at
assert_operator previously_wheels_owned_at, :<, car.wheels_owned_at
end

def test_polymorphic_destroy_with_dependencies_and_lock_version
@@ -206,12 +206,28 @@ def test_increment_updates_counter_in_db_using_offset
assert_equal initial_credit + 2, a1.reload.credit_limit
end

def test_increment_updates_timestamps
def test_increment_with_touch_updates_timestamps
topic = topics(:first)
topic.update_columns(updated_at: 5.minutes.ago)
previous_updated_at = topic.updated_at
topic.increment!(:replies_count, touch: true)
assert_operator previous_updated_at, :<, topic.reload.updated_at
assert_equal 1, topic.replies_count
previously_updated_at = topic.updated_at
travel(1.second) do
topic.increment!(:replies_count, touch: true)
end
assert_equal 2, topic.reload.replies_count
assert_operator previously_updated_at, :<, topic.updated_at
end

def test_increment_with_touch_an_attribute_updates_timestamps
topic = topics(:first)
assert_equal 1, topic.replies_count
previously_updated_at = topic.updated_at
previously_written_on = topic.written_on
travel(1.second) do
topic.increment!(:replies_count, touch: :written_on)
end
assert_equal 2, topic.reload.replies_count
assert_operator previously_updated_at, :<, topic.updated_at
assert_operator previously_written_on, :<, topic.written_on
end

def test_destroy_all
@@ -333,12 +349,28 @@ def test_decrement_attribute_by
assert_equal 41, accounts(:signals37, :reload).credit_limit
end

def test_decrement_updates_timestamps
def test_decrement_with_touch_updates_timestamps
topic = topics(:first)
topic.update_columns(updated_at: 5.minutes.ago)
previous_updated_at = topic.updated_at
topic.decrement!(:replies_count, touch: true)
assert_operator previous_updated_at, :<, topic.reload.updated_at
assert_equal 1, topic.replies_count
previously_updated_at = topic.updated_at
travel(1.second) do
topic.decrement!(:replies_count, touch: true)
end
assert_equal 0, topic.reload.replies_count
assert_operator previously_updated_at, :<, topic.updated_at
end

def test_decrement_with_touch_an_attribute_updates_timestamps
topic = topics(:first)
assert_equal 1, topic.replies_count
previously_updated_at = topic.updated_at
previously_written_on = topic.written_on
travel(1.second) do
topic.decrement!(:replies_count, touch: :written_on)
end
assert_equal 0, topic.reload.replies_count
assert_operator previously_updated_at, :<, topic.updated_at
assert_operator previously_written_on, :<, topic.written_on
end

def test_create
@@ -20,6 +20,8 @@ class Car < ActiveRecord::Base
scope :incl_engines, -> { includes(:engines) }

scope :order_using_new_style, -> { order("name asc") }

attribute :wheels_owned_at, :datetime, default: -> { Time.now }
end

class CoolCar < Car
@@ -1,5 +1,5 @@
# frozen_string_literal: true

class Wheel < ActiveRecord::Base
belongs_to :wheelable, polymorphic: true, counter_cache: true, touch: true
belongs_to :wheelable, polymorphic: true, counter_cache: true, touch: :wheels_owned_at
end
@@ -36,6 +36,7 @@
create_table :aircraft, force: true do |t|
t.string :name
t.integer :wheels_count, default: 0, null: false
t.datetime :wheels_owned_at
end

create_table :articles, force: true do |t|
@@ -126,7 +127,8 @@
create_table :cars, force: true do |t|
t.string :name
t.integer :engines_count
t.integer :wheels_count, default: 0
t.integer :wheels_count, default: 0, null: false
t.datetime :wheels_owned_at
t.column :lock_version, :integer, null: false, default: 0
t.timestamps null: false
end

0 comments on commit cad0b7d

Please sign in to comment.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.