New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make AR#increment! AR#decrement! concurency safe #11410
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -332,24 +332,26 @@ def increment(attribute, by = 1) | |
# Saving is not subjected to validation checks. Returns +true+ if the | ||
# record could be saved. | ||
def increment!(attribute, by = 1) | ||
increment(attribute, by).update_attribute(attribute, self[attribute]) | ||
increment(attribute, by) | ||
change = public_send(attribute) - (attribute_was(attribute.to_s) || 0) | ||
self.class.update_counters(id, attribute => change) | ||
clear_attribute_change(attribute) # eww | ||
self | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this changes the behavior from the documentation, which says that these methods return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. /cc @bogdan |
||
end | ||
|
||
# Initializes +attribute+ to zero if +nil+ and subtracts the value passed as +by+ (default is 1). | ||
# The decrement is performed directly on the underlying attribute, no setter is invoked. | ||
# Only makes sense for number-based attributes. Returns +self+. | ||
def decrement(attribute, by = 1) | ||
self[attribute] ||= 0 | ||
self[attribute] -= by | ||
self | ||
increment(attribute, -by) | ||
end | ||
|
||
# Wrapper around +decrement+ that saves the record. This method differs from | ||
# its non-bang version in that it passes through the attribute setter. | ||
# Saving is not subjected to validation checks. Returns +true+ if the | ||
# record could be saved. | ||
def decrement!(attribute, by = 1) | ||
decrement(attribute, by).update_attribute(attribute, self[attribute]) | ||
increment!(attribute, -by) | ||
end | ||
|
||
# Assigns to +attribute+ the boolean opposite of <tt>attribute?</tt>. So | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,6 +120,15 @@ def test_increment_attribute_by | |
assert_equal 59, accounts(:signals37, :reload).credit_limit | ||
end | ||
|
||
def test_increment_updates_counter_in_db_using_offset | ||
a1 = accounts(:signals37) | ||
initial_credit = a1.credit_limit | ||
a2 = Account.find(accounts(:signals37).id) | ||
a1.increment!(:credit_limit) | ||
a2.increment!(:credit_limit) | ||
assert_equal initial_credit + 2, a1.reload.credit_limit | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we also verify that the in-memory objects have the right value (without a reload) after updating? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This patch doesn't fix that. We can not make everything perfect from one patch. I would prefer to apply it afterwards. |
||
end | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "test increment concurrency" needs to be more precise so future readers of this code understand the guarantee we're making. This is testing the I in ACID, that Does this single test really cover all the code changes above? It's difficult to determine but seems unlikely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jeremy Maybe the changes fix some bugs when counter cache is only updated in DB but not on instance, but it is hard to assume where it happens and if it really happens. We have no identity map and we can not be sure we updated all instances in memory with a proper counter value. So I would consider all these changes a refactoring that just reuses code. Renamed test name to be more precise about use case. |
||
def test_destroy_all | ||
conditions = "author_name = 'Mary'" | ||
topics_by_mary = Topic.all.merge!(:where => conditions, :order => 'id').to_a | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come this method is removed while the equivalent in
has_many_through_association
was kept?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply moved method to the place where it is used: only in
has_many_through_association
but nothas_many_association
.