Permalink
Browse files

Ensure transactions around save and destroy are rolled back when a be…

…fore hook returns false

Three cases:

1) raise_on_save_failure true: Existing behavior was correct;
save_failure raises an exception that rolls back the transaction
and is reraised.
2) raise_on_save_failure false and use_transaction true: save_failure
raises Rollback, which the transaction code will take care of.
3) raise_on_save_failure false and use_transaction false: should just
return false, as raising Rollback and catching it makes it impossible
to differentiate between the a before hook returning false and a user
raising Rollback in a before hook (which they expect to bubble up to
their own transaction code).

While here, support the :transaction option in destroy
  • Loading branch information...
1 parent 3d364a6 commit c89a4e47a22d7741745b625d2f0488c9d3d8755e John Firebaugh committed with Nov 17, 2009
Showing with 96 additions and 12 deletions.
  1. +25 −12 lib/sequel/model/base.rb
  2. +24 −0 spec/model/model_spec.rb
  3. +47 −0 spec/model/record_spec.rb
View
37 lib/sequel/model/base.rb
@@ -506,8 +506,20 @@ def self.class_attr_reader(*meths) # :nodoc:
private_class_method :class_attr_overridable, :class_attr_reader
class_attr_reader :columns, :db, :primary_key, :db_schema
- class_attr_overridable :raise_on_save_failure, :raise_on_typecast_failure, :strict_param_setting, :typecast_empty_string_to_nil, :typecast_on_assignment, :use_transactions
-
+ class_attr_overridable :raise_on_save_failure, :raise_on_typecast_failure, :strict_param_setting, :typecast_empty_string_to_nil, :typecast_on_assignment
+
+ attr_writer :use_transactions
+ def use_transactions(opts = {})
+ case
+ when opts.include?(:transaction)
+ opts[:transaction]
+ when defined?(@use_transactions)
+ @use_transactions
+ else
+ self.class.use_transactions
+ end
+ end
+
# The hash of attribute values. Keys are symbols with the names of the
# underlying database columns.
attr_reader :values
@@ -608,8 +620,8 @@ def delete
# deleting the object the the database. Otherwise, deletes
# the item from the database and returns self. Uses a transaction
# if use_transactions is true.
- def destroy
- use_transactions ? db.transaction{_destroy} : _destroy
+ def destroy(opts = {})
+ use_transactions(opts) ? db.transaction{_destroy(opts)} : _destroy(opts)
end
# Iterates through all of the current values using each.
@@ -722,8 +734,7 @@ def reload
def save(*columns)
opts = columns.last.is_a?(Hash) ? columns.pop : {}
return save_failure(:invalid) if opts[:validate] != false and !valid?
- use_transaction = opts.include?(:transaction) ? opts[:transaction] : use_transactions
- use_transaction ? db.transaction(opts){_save(columns, opts)} : _save(columns, opts)
+ use_transactions(opts) ? db.transaction(opts){_save(columns, opts)} : _save(columns, opts)
end
# Saves only changed columns if the object has been modified.
@@ -809,8 +820,8 @@ def valid?
# Internal destroy method, separted from destroy to
# allow running inside a transaction
- def _destroy
- return save_failure(:destroy) if before_destroy == false
+ def _destroy(opts)
+ return save_failure(:destroy, opts) if before_destroy == false
delete
after_destroy
self
@@ -844,9 +855,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) if before_save == false
+ return save_failure(:save, opts) if before_save == false
if new?
- return save_failure(:create) if before_create == false
+ return save_failure(:create, opts) if before_create == false
pk = _insert
@this = nil if pk
@new = false
@@ -862,7 +873,7 @@ def _save(columns, opts)
changed_columns.clear
end
else
- return save_failure(:update) if before_update == false
+ return save_failure(:update, opts) if before_update == false
if columns.empty?
@columns_updated = opts[:changed] ? @values.reject{|k,v| !changed_columns.include?(k)} : @values.dup
changed_columns.clear
@@ -890,13 +901,15 @@ def inspect_values
end
# Raise an error if raise_on_save_failure is true, return nil otherwise.
- def save_failure(type)
+ 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_transactions(opts)
+ raise Rollback
end
end
View
24 spec/model/model_spec.rb
@@ -565,3 +565,27 @@ def @c.columns; [:x]; end
@c.primary_key.should == :id
end
end
+
+context "Model#use_transactions" do
+ before do
+ @c = Class.new(Sequel::Model(:items))
+ @instance = @c.new
+ end
+
+ specify "should return class value by default" do
+ @instance.use_transactions.should == Sequel::Model.use_transactions
+ end
+
+ specify "should return set value if manually set" do
+ @instance.use_transactions = false
+ @instance.use_transactions.should == false
+ @instance.use_transactions = true
+ @instance.use_transactions.should == true
+ end
+
+ specify "should return :transaction option if given" do
+ @instance.use_transactions(:transaction => false).should == false
+ @instance.use_transactions = false
+ @instance.use_transactions(:transaction => true).should == true
+ end
+end
View
47 spec/model/record_spec.rb
@@ -207,6 +207,41 @@ def ds.insert_select(hash)
MODEL_DB.sqls.should == ["BEGIN", "UPDATE items SET y = NULL WHERE (id = 3)", "COMMIT"]
MODEL_DB.reset
end
+
+ it "should rollback if before_save returns false and raise_on_save_failure = true" do
+ o = @c.load(:id => 3, :x => 1, :y => nil)
+ o.use_transactions = true
+ o.raise_on_save_failure = true
+ def o.before_save
+ false
+ end
+ proc { o.save(:y) }.should raise_error(Sequel::BeforeHookFailed)
+ MODEL_DB.sqls.should == ["BEGIN", "ROLLBACK"]
+ 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
+ o.raise_on_save_failure = false
+ def o.before_save
+ false
+ end
+ o.save(:y).should == nil
+ MODEL_DB.sqls.should == ["BEGIN", "ROLLBACK"]
+ MODEL_DB.reset
+ end
+
+ it "should not rollback if before_save throws Rollback and use_transactions = false" do
+ o = @c.load(:id => 3, :x => 1, :y => nil)
+ o.use_transactions = false
+ def o.before_save
+ raise Sequel::Error::Rollback
+ end
+ proc { o.save(:y) }.should raise_error(Sequel::Error::Rollback)
+ MODEL_DB.sqls.should == []
+ MODEL_DB.reset
+ end
end
describe "Model#marshallable" do
@@ -760,6 +795,18 @@ def o.modified?; false; end
@instance.destroy
end
+ it "should run within a transaction if :transaction option is true" do
+ @instance.use_transactions = false
+ @model.db.should_receive(:transaction)
+ @instance.destroy(:transaction => true)
+ end
+
+ it "should not run within a transaction if :transaction option is false" do
+ @instance.use_transactions = true
+ @model.db.should_not_receive(:transaction)
+ @instance.destroy(:transaction => false)
+ end
+
it "should run before_destroy and after_destroy hooks" do
@model.send(:define_method, :before_destroy){MODEL_DB.execute('before blah')}
@model.send(:define_method, :after_destroy){MODEL_DB.execute('after blah')}

0 comments on commit c89a4e4

Please sign in to comment.