Support a :raise_on_failure option for save and destroy that override…

…s the raise_on_save_failure setting

This is useful in library contexts where it is necessary to enforce or
prohibit raising without perturbing the existing raise_on_save_failure
setting (for example, to implement an ActiveRecord-like save! method).
Before, this would have required a begin/set/ensure/reset dance.

While here, update and clarify the documentation for save, and rename
save_failure and update its usage to reflect its new behavior.

lib/sequel/model/base.rb

@@ -766,7 +766,7 @@ def delete
       #   Artist[1].destroy # BEGIN; DELETE FROM artists WHERE (id = 1); COMMIT;
       #   # => #<Artist {:id=>1, ...}>
       def destroy(opts = {})
-        checked_save_failure{checked_transaction(opts){_destroy(opts)}}
+        checked_save_failure(opts){checked_transaction(opts){_destroy(opts)}}
       end
 
       # Iterates through all of the current values using each.
@@ -928,34 +928,37 @@ def reload
       end
 
       # Creates or updates the record, after making sure the record
-      # is valid.  If the record is not valid, or before_save,
-      # before_create (if new?), or before_update (if !new?) return
-      # false, returns nil unless +raise_on_save_failure+ is true (if it
-      # is true, it raises an error).
-      # Otherwise, returns self. You can provide an optional list of
-      # columns to update, in which case it only updates those columns.
+      # is valid and before hooks execute successfully. Fails if:
       #
-      # Takes the following options:
+      # * the record is not valid, or
+      # * before_save returns false, or
+      # * the record is new and before_create returns false, or
+      # * the record is not new and before_update returns false.
       #
-      # :changed :: save all changed columns, instead of all columns or the columns given
-      # :transaction :: set to false not to use a transaction
-      # :validate :: set to false not to validate the model before saving
+      # If +save+ fails and either raise_on_save_failure or the
+      # :raise_on_failure option is true, it raises ValidationFailed
+      # or BeforeHookFailed. Otherwise it returns nil.
       #
-      #   a = Artist.new(:name=>'Bob')
-      #   a.save # INSERT INTO artists (name) VALUES ('Bob')
+      # If it succeeds, it returns self.
       #
-      #   a = Artist[1]
-      #   a.save # UPDATE artists SET name = 'Bob', hometown = 'Sac' WHERE (id = 1)
-      #   a.save(:name) # UPDATE artists SET name = 'Bob' WHERE (id = 1)
-      #   a.hometown = 'LA'
-      #   a.save(:changes=>true) # UPDATE artists SET hometown = 'Sac' WHERE (id = 1)
+      # You can provide an optional list of columns to update, in which
+      # case it only updates those columns.
+      #
+      # Takes the following options:
+      #
+      # * :changed - save all changed columns, instead of all columns or the columns given
+      # * :transaction - set to true or false to override the current
+      #   use_transactions setting
+      # * :validate - set to false to skip validation
+      # * :raise_on_failure - set to true or false to override the current
+      #   raise_on_save_failure setting
       def save(*columns)
         opts = columns.last.is_a?(Hash) ? columns.pop : {}
-        if opts[:validate] != false and !valid?
-          raise(ValidationFailed.new(errors)) if raise_on_save_failure
+        if opts[:validate] != false and !valid?(opts)
+          raise(ValidationFailed.new(errors)) if raise_on_failure?(opts)
           return
         end
-        checked_save_failure{checked_transaction(opts){_save(columns, opts)}}
+        checked_save_failure(opts){checked_transaction(opts){_save(columns, opts)}}
       end
 
       # Saves only changed columns if the object has been modified.
@@ -1096,10 +1099,10 @@ def validate
       #   artist(:name=>'Valid').valid? # => true
       #   artist(:name=>'Invalid').valid? # => false
       #   artist.errors.full_messages # => ['name cannot be Invalid']
-      def valid?
+      def valid?(opts = {})
         errors.clear
         if before_validation == false
-          save_failure(:validation) if raise_on_save_failure
+          raise_hook_failure(:validation) if raise_on_failure?(opts)
           return false
         end
         validate
@@ -1125,7 +1128,7 @@ def _delete_dataset
       # Internal destroy method, separted from destroy to
       # allow running inside a transaction
       def _destroy(opts)
-        return save_failure(:destroy) if before_destroy == false
+        raise_hook_failure(:destroy) if before_destroy == false
         _destroy_delete
         after_destroy
         self
@@ -1173,9 +1176,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
+        raise_hook_failure(:save) if before_save == false
         if new?
-          return save_failure(:create) if before_create == false
+          raise_hook_failure(:create) if before_create == false
           pk = _insert
           @this = nil
           @new = false
@@ -1185,7 +1188,7 @@ def _save(columns, opts)
           @was_new = nil
           pk ? _save_refresh : changed_columns.clear
         else
-          return save_failure(:update) if before_update == false
+          raise_hook_failure(:update) if before_update == false
           if columns.empty?
             @columns_updated = if opts[:changed]
               @values.reject{|k,v| !changed_columns.include?(k)}
@@ -1239,10 +1242,10 @@ def _update_dataset
         this
       end
 
-      # If raise_on_save_failure is false, check for BeforeHookFailed
+      # If not raising on failure, check for BeforeHookFailed
       # being raised by yielding and swallow it.
-      def checked_save_failure
-        if raise_on_save_failure
+      def checked_save_failure(opts)
+        if raise_on_failure?(opts)
           yield
         else
           begin
@@ -1262,9 +1265,18 @@ def checked_transaction(opts={})
       def inspect_values
         @values.inspect
       end
-
-      # Raise a +BeforeHookFailed+ exception.
-      def save_failure(type)
+
+      # Whether to raise or return false if this action fails. If the
+      # :raise_on_failure option is present in the hash, use that, otherwise,
+      # fallback to the object's raise_on_save_failure (if set), or
+      # class's default (if not).
+      def raise_on_failure?(opts)
+        opts.fetch(:raise_on_failure, raise_on_save_failure)
+      end
+
+      # Raise an error appropriate to the hook type. May be swallowed by
+      # checked_save_failure depending on the raise_on_failure? setting.
+      def raise_hook_failure(type)
         raise BeforeHookFailed, "one of the before_#{type} hooks returned false"
       end
 

spec/model/associations_spec.rb

@@ -1316,7 +1316,7 @@ def ds.fetch_rows(sql)
     @c2.one_to_many :attributes, :class => @c1
     n = @c2.new(:id => 1234)
     a = @c1.new(:id => 2345)
-    def a.valid?; false; end
+    def a.valid?(opts); false; end
     proc{n.add_attribute(a)}.should raise_error(Sequel::Error)
     proc{n.remove_attribute(a)}.should raise_error(Sequel::Error)
   end
@@ -1325,7 +1325,7 @@ def a.valid?; false; end
     @c2.one_to_many :attributes, :class => @c1, :validate=>false
     n = @c2.new(:id => 1234)
     a = @c1.new(:id => 2345)
-    def a.valid?; false; end
+    def a.valid?(opts); false; end
     n.add_attribute(a).should == a
     n.remove_attribute(a).should == a
   end
@@ -2199,7 +2199,7 @@ def ds.fetch_rows(sql)
     @c2.many_to_many :attributes, :class => @c1 
     n = @c1.new
     a = @c2.load(:id=>123)
-    def n.valid?; false; end
+    def n.valid?(opts); false; end
     proc{a.add_attribute(n)}.should raise_error(Sequel::ValidationFailed)
   end
 
@@ -2208,15 +2208,15 @@ def n.valid?; false; end
     n = @c1.new
     n.raise_on_save_failure = false
     a = @c2.load(:id=>123)
-    def n.valid?; false; end
+    def n.valid?(opts); false; end
     proc{a.add_attribute(n)}.should raise_error(Sequel::Error)
   end
 
   it "should not attempt to validate the associated object in add_ if the :validate=>false option is used" do
     @c2.many_to_many :attributes, :class => @c1, :validate=>false
     n = @c1.new
     a = @c2.load(:id=>123)
-    def n.valid?; false; end
+    def n.valid?(opts); false; end
     a.add_attribute(n)
     n.new?.should == false
   end

spec/model/hooks_spec.rb

@@ -89,6 +89,14 @@ def after_update; MODEL_DB << "BLAH after" end
     MODEL_DB.sqls.should == []
   end
 
+  specify "#save should cancel the save and raise an error if before_update returns false and raise_on_failure option is true" do
+    @c.send(:define_method, :before_update){false}
+    @c.raise_on_save_failure = false
+    proc{@c.load(:id => 2233).save(:raise_on_failure => true)}.should_not raise_error(Sequel::ValidationFailed)
+    proc{@c.load(:id => 2233).save(:raise_on_failure => true)}.should raise_error(Sequel::BeforeHookFailed)
+    MODEL_DB.sqls.should == []
+  end
+
   specify "#save should cancel the save and return nil if before_update returns false and raise_on_save_failure is false" do
     @c.send(:define_method, :before_update){false}
     @c.raise_on_save_failure = false
@@ -137,6 +145,14 @@ def after_save; MODEL_DB << "BLAH after" end
     MODEL_DB.sqls.should == []
   end
 
+  specify "#save should cancel the save and raise an error if before_save returns false and raise_on_failure option is true" do
+    @c.send(:define_method, :before_save){false}
+    @c.raise_on_save_failure = false
+    proc{@c.load(:id => 2233).save(:raise_on_failure => true)}.should_not raise_error(Sequel::ValidationFailed)
+    proc{@c.load(:id => 2233).save(:raise_on_failure => true)}.should raise_error(Sequel::BeforeHookFailed)
+    MODEL_DB.sqls.should == []
+  end
+
   specify "#save should cancel the save and return nil if before_save returns false and raise_on_save_failure is false" do
     @c.send(:define_method, :before_save){false}
     @c.raise_on_save_failure = false
@@ -176,6 +192,13 @@ def delete
     MODEL_DB.sqls.should == []
   end
 
+  specify "#destroy should cancel the destroy and raise an error if before_destroy returns false and raise_on_failure option is true" do
+    @c.send(:define_method, :before_destroy){false}
+    @c.raise_on_save_failure = false
+    proc{@c.load(:id => 2233).destroy(:raise_on_failure => true)}.should raise_error(Sequel::BeforeHookFailed)
+    MODEL_DB.sqls.should == []
+  end
+
   specify "#destroy should cancel the destroy and return nil if before_destroy returns false and raise_on_save_failure is false" do
     @c.send(:define_method, :before_destroy){false}
     @c.raise_on_save_failure = false
@@ -231,6 +254,14 @@ def validate
     MODEL_DB.sqls.should == []
   end
 
+  specify "#save should cancel the save and raise an error if before_validation returns false and raise_on_failure option is true" do
+    @c.send(:define_method, :before_validation){false}
+    @c.raise_on_save_failure = false
+    proc{@c.load(:id => 2233).save(:raise_on_failure => true)}.should_not raise_error(Sequel::ValidationFailed)
+    proc{@c.load(:id => 2233).save(:raise_on_failure => true)}.should raise_error(Sequel::BeforeHookFailed)
+    MODEL_DB.sqls.should == []
+  end
+
   specify "#save should cancel the save and return nil if before_validation returns false and raise_on_save_failure is false" do
     @c.send(:define_method, :before_validation){false}
     @c.raise_on_save_failure = false

spec/model/record_spec.rb

@@ -243,6 +243,18 @@ def o.before_save
     MODEL_DB.reset
   end
 
+  it "should rollback if before_save returns false and :raise_on_failure option is true" 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
+    proc { o.save(:y, :raise_on_failure => true) }.should raise_error(Sequel::BeforeHookFailed)
+    MODEL_DB.sqls.should == ["BEGIN", "ROLLBACK"]
+    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
@@ -386,7 +398,7 @@ class ::Album < Sequel::Model
 
   it "should take options passed to save" do
     o = @c.new(:x => 1)
-    def o.valid?; false; end
+    def o.before_validation; false; end
     proc{o.save_changes}.should raise_error(Sequel::Error)
     MODEL_DB.sqls.should == []
     o.save_changes(:validate=>false)

spec/model/validations_spec.rb

@@ -128,7 +128,7 @@ def validate
       columns :id, :x
 
       def validate
-        errors[:id] << 'blah' unless x == 7
+        errors.add(:id, 'blah') unless x == 7
       end
     end
     @m = @c.load(:id => 4, :x=>6)
@@ -153,11 +153,16 @@ def validate
     @m.save(:validate=>false)
     MODEL_DB.sqls.should == ['UPDATE people SET x = 6 WHERE (id = 4)']
   end
-    
+
   specify "should raise error if validations fail and raise_on_save_faiure is true" do
-    proc{@m.save}.should raise_error(Sequel::ValidationFailed){ |e| e.errors.should == @m.errors }
+    proc{@m.save}.should raise_error(Sequel::ValidationFailed){|e| e.errors.should == @m.errors }
   end
-
+
+  specify "should raise error if validations fail and :raise_on_failure option is true" do
+    @m.raise_on_save_failure = false
+    proc{@m.save(:raise_on_failure => true)}.should raise_error(Sequel::ValidationFailed)
+  end
+
   specify "should return nil if validations fail and raise_on_save_faiure is false" do
     @m.raise_on_save_failure = false
     @m.save.should == nil