Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

In Model#save and destroy, don't rollback transactions you didn't ini…

…tiate

This fixes the issue where an outer transaction is present, and
use_transactions is set for the model instance, and one of the
before hooks returns false.  With the recent commits, that would
rollback the outer transaction.

This required refactoring so that save_failure now always raises,
it doesn't check raise_on_save_failure.  Model#save and destroy
now use checked_save_failure, which ignore the error raised by
save_failure if raise_on_save_failure is true.

While refactoring, I decided it was no longer worth using
save_failure to handle validation failures, so the validation
failures don't call it.  before_validation hook callers only call
it if raise_on_save_failure is true, since they aren't wrapped
by check_save_failure.

This breaks backwards compatibility for callers of save_failure,
which is a private method.

While here, add checked_transaction private method that uses
transactions if necessary.
  • Loading branch information...
commit 13d2a4a2a6a6bbc920d2b6289f60d8dd07311ef9 1 parent f333791
@jeremyevans authored
Showing with 51 additions and 21 deletions.
  1. +33 −18 lib/sequel/model/base.rb
  2. +18 −3 spec/model/record_spec.rb
View
51 lib/sequel/model/base.rb
@@ -610,7 +610,7 @@ def delete
# if use_transactions is true or if the :transaction option is given and
# true.
def destroy(opts = {})
- use_transaction?(opts) ? db.transaction{_destroy(opts)} : _destroy(opts)
+ checked_save_failure{checked_transaction(opts){_destroy(opts)}}
end
# Iterates through all of the current values using each.
@@ -722,8 +722,11 @@ def reload
# * :validate - set to false not to validate the model before saving
def save(*columns)
opts = columns.last.is_a?(Hash) ? columns.pop : {}
- return save_failure(:invalid) if opts[:validate] != false and !valid?
- use_transaction?(opts) ? db.transaction(opts){_save(columns, opts)} : _save(columns, opts)
+ if opts[:validate] != false and !valid?
+ raise(ValidationFailed, errors.full_messages.join(', ')) if raise_on_save_failure
+ return
+ end
+ checked_save_failure{checked_transaction(opts){_save(columns, opts)}}
end
# Saves only changed columns if the object has been modified.
@@ -797,7 +800,7 @@ def validate
def valid?
errors.clear
if before_validation == false
- save_failure(:validation)
+ save_failure(:validation) if raise_on_save_failure
return false
end
validate
@@ -810,7 +813,7 @@ def valid?
# Internal destroy method, separted from destroy to
# allow running inside a transaction
def _destroy(opts)
- return save_failure(:destroy, opts) if before_destroy == false
+ return save_failure(:destroy) if before_destroy == false
delete
after_destroy
self
@@ -844,9 +847,9 @@ def _refresh(dataset)
# Internal version of save, split from save to allow running inside
# it's own transaction.
def _save(columns, opts)
- return save_failure(:save, opts) if before_save == false
+ return save_failure(:save) if before_save == false
if new?
- return save_failure(:create, opts) if before_create == false
+ return save_failure(:create) if before_create == false
pk = _insert
@this = nil if pk
@new = false
@@ -862,7 +865,7 @@ def _save(columns, opts)
changed_columns.clear
end
else
- return save_failure(:update, opts) if before_update == false
+ return save_failure(:update) if before_update == false
if columns.empty?
@columns_updated = opts[:changed] ? @values.reject{|k,v| !changed_columns.include?(k)} : @values.dup
changed_columns.clear
@@ -880,26 +883,38 @@ def _save(columns, opts)
self
end
+ # Update this instance's dataset with the supplied column hash.
def _update(columns)
this.update(columns)
end
+
+ # If raise_on_save_failure is false, check for BeforeHookFailed
+ # beind raised by yielding and swallow it.
+ def checked_save_failure
+ if raise_on_save_failure
+ yield
+ else
+ begin
+ yield
+ rescue BeforeHookFailed
+ nil
+ end
+ end
+ end
+ # If transactions should be used, wrap the yield in a transaction block.
+ def checked_transaction(opts)
+ use_transaction?(opts)? db.transaction(opts){yield} : yield
+ end
+
# Default inspection output for the values hash, overwrite to change what #inspect displays.
def inspect_values
@values.inspect
end
# Raise an error if raise_on_save_failure is true, return nil otherwise.
- def save_failure(type, opts = {})
- if raise_on_save_failure
- if type == :invalid
- raise ValidationFailed, errors.full_messages.join(', ')
- else
- raise BeforeHookFailed, "one of the before_#{type} hooks returned false"
- end
- elsif type != :invalid && use_transaction?(opts)
- raise Rollback
- end
+ def save_failure(type)
+ raise BeforeHookFailed, "one of the before_#{type} hooks returned false"
end
# Set the columns, filtered by the only and except arrays.
View
21 spec/model/record_spec.rb
@@ -158,7 +158,7 @@ def ds.insert_select(hash)
res.should == [nil, nil]
end
- it "should use Model's save_in_transaction setting by default" do
+ it "should use Model's use_transactions setting by default" do
@c.use_transactions = true
@c.load(:id => 3, :x => 1, :y => nil).save(:y)
MODEL_DB.sqls.should == ["BEGIN", "UPDATE items SET y = NULL WHERE (id = 3)", "COMMIT"]
@@ -169,7 +169,7 @@ def ds.insert_select(hash)
MODEL_DB.reset
end
- it "should inherit Model's save_in_transaction setting" do
+ it "should inherit Model's use_transactions setting" do
@c.use_transactions = true
Class.new(@c).load(:id => 3, :x => 1, :y => nil).save(:y)
MODEL_DB.sqls.should == ["BEGIN", "UPDATE items SET y = NULL WHERE (id = 3)", "COMMIT"]
@@ -180,7 +180,7 @@ def ds.insert_select(hash)
MODEL_DB.reset
end
- it "should use object's save_in_transaction setting" do
+ it "should use object's use_transactions setting" do
o = @c.load(:id => 3, :x => 1, :y => nil)
o.use_transactions = false
@c.use_transactions = true
@@ -220,6 +220,21 @@ def o.before_save
MODEL_DB.reset
end
+ it "should not rollback outer transactions if before_save returns false and raise_on_save_failure = false" do
+ o = @c.load(:id => 3, :x => 1, :y => nil)
+ o.use_transactions = true
+ o.raise_on_save_failure = false
+ def o.before_save
+ false
+ end
+ MODEL_DB.transaction do
+ o.save(:y).should == nil
+ MODEL_DB.run "BLAH"
+ end
+ MODEL_DB.sqls.should == ["BEGIN", "BLAH", "COMMIT"]
+ MODEL_DB.reset
+ end
+
it "should rollback if before_save returns false and raise_on_save_failure = false" do
o = @c.load(:id => 3, :x => 1, :y => nil)
o.use_transactions = true
Please sign in to comment.
Something went wrong with that request. Please try again.