Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Add require_modification to Sequel::Model, for checking that model in…
…stance updating and deleting affects a single row

Previously, Sequel would not check that model instance update and
delete queries actually modified a single row, which led to the
following behavior:

  DB.create_table(:as){primary_key :id}
  class A < Sequel::Model; end
  a = A.create
  a.delete
  a.save # no error!
  a.delete # no error!

This commit adds a new global, class, and instance flag to
Sequel::Model that makes the model instance update and delete
methods check that a single row was modified:

  Sequel::Model.require_modification = false # global
  Album.require_modification = true # class
  album.require_modification = false # instance

This appears to work fine on all tested database adapters except
ADO, native MySQL, and the mysql do subadapter.  Currently, the
global default is set to true, but I will be changing it in the
future to check with the database or dataset object to see if
it is supported.
  • Loading branch information
jeremyevans committed Apr 14, 2010
1 parent a4eaba2 commit 21ef8d4
Show file tree
Hide file tree
Showing 11 changed files with 81 additions and 24 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG
@@ -1,5 +1,7 @@
=== HEAD

* Add require_modification to Sequel::Model, for checking that model instance updating and deleting affects a single row (jeremyevans)

* Fix leak of ResultSets when getting metadata in the jdbc adapter (jrun)

* Make Dataset#filter and related methods just clone receiver if given an empty argument, such as {}, [], or '' (jeremyevans)
Expand Down
4 changes: 3 additions & 1 deletion lib/sequel/model.rb
Expand Up @@ -72,7 +72,8 @@ class Model
# If the value is nil, the superclass's instance variable is used directly in the subclass.
INHERITED_INSTANCE_VARIABLES = {:@allowed_columns=>:dup, :@dataset_methods=>:dup,
:@dataset_method_modules=>:dup, :@primary_key=>nil, :@use_transactions=>nil,
:@raise_on_save_failure=>nil, :@restricted_columns=>:dup, :@restrict_primary_key=>nil,
:@raise_on_save_failure=>nil, :@require_modification=>nil,
:@restricted_columns=>:dup, :@restrict_primary_key=>nil,
:@simple_pk=>nil, :@simple_table=>nil, :@strict_param_setting=>nil,
:@typecast_empty_string_to_nil=>nil, :@typecast_on_assignment=>nil,
:@raise_on_typecast_failure=>nil, :@plugins=>:dup}
Expand Down Expand Up @@ -100,6 +101,7 @@ class Model
@primary_key = :id
@raise_on_save_failure = true
@raise_on_typecast_failure = true
@require_modification = true
@restrict_primary_key = true
@restricted_columns = nil
@simple_pk = nil
Expand Down
17 changes: 13 additions & 4 deletions lib/sequel/model/base.rb
Expand Up @@ -42,6 +42,11 @@ module ClassMethods
# plugins) in connection with option to check for typecast failures for
# columns that aren't blobs or strings.
attr_accessor :raise_on_typecast_failure

# Whether to raise an error if an UPDATE or DELETE query related to
# a model instance does not modify exactly 1 row. If set to false,
# Sequel will not check the number of rows modified (default: true).
attr_accessor :require_modification

# Which columns are specifically restricted in a call to set/update/new/etc.
# (default: not set). Some columns are restricted regardless of
Expand Down Expand Up @@ -493,7 +498,7 @@ def set_columns(new_columns)
# * The following instance_methods all call the class method of the same
# name: columns, dataset, db, primary_key, db_schema.
# * The following instance methods allow boolean flags to be set on a per-object
# basis: raise_on_save_failure, raise_on_typecast_failure, strict_param_setting,
# basis: raise_on_save_failure, raise_on_typecast_failure, require_modification, strict_param_setting,
# typecast_empty_string_to_nil, typecast_on_assignment, use_transactions.
# If they are not used, the object will default to whatever the model setting is.
module InstanceMethods
Expand All @@ -518,7 +523,7 @@ 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, :require_modification, :strict_param_setting, :typecast_empty_string_to_nil, :typecast_on_assignment, :use_transactions

# The hash of attribute values. Keys are symbols with the names of the
# underlying database columns.
Expand Down Expand Up @@ -822,7 +827,9 @@ def valid?

# Actually do the deletion of the object's dataset.
def _delete
_delete_dataset.delete
n = _delete_dataset.delete
raise(NoExistingObject, "Attempt to delete object did not result in a single row modification (Rows Deleted: #{n}, SQL: #{_delete_dataset.delete_sql})") if require_modification && n != 1
n
end

# The dataset to use when deleting the object. The same as the object's
Expand Down Expand Up @@ -912,7 +919,9 @@ def _save(columns, opts)

# Update this instance's dataset with the supplied column hash.
def _update(columns)
_update_dataset.update(columns)
n = _update_dataset.update(columns)
raise(NoExistingObject, "Attempt to update object did not result in a single row modification (SQL: #{_update_dataset.update_sql(columns)})") if require_modification && n != 1
n
end

# The dataset to use when updating an object. The same as the object's
Expand Down
12 changes: 8 additions & 4 deletions lib/sequel/model/exceptions.rb
@@ -1,5 +1,12 @@
module Sequel
# This exception will be raised when raise_on_save_failure is set and validation fails
# Exception class raised when raise_on_save_failure is set and a before hook returns false
class BeforeHookFailed < Error; end

# Exception class raised when require_modification is set and an UPDATE or DELETE statement to modify the dataset doesn't
# modify a single row.
class NoExistingObject < Error; end

# Exception class raised when raise_on_save_failure is set and validation fails
class ValidationFailed < Error
def initialize(errors)
if errors.respond_to?(:full_messages)
Expand All @@ -11,7 +18,4 @@ def initialize(errors)
end
attr_reader :errors
end

# This exception will be raised when raise_on_save_failure is set and a before hook returns false
class BeforeHookFailed < Error; end
end
18 changes: 4 additions & 14 deletions lib/sequel/plugins/instance_filters.rb
Expand Up @@ -32,11 +32,13 @@ module Plugins
# # database row has been updated to allow deleting,
# # delete now works.
# i1.delete
#
# You must have require_modification on the class and instances
# that use this plugin.
module InstanceFilters
# Exception class raised when updating or deleting an object does
# not affect exactly one row.
class Error < Sequel::Error
end
Error = Sequel::NoExistingObject

module InstanceMethods
# Clear the instance filters after successfully destroying the object.
Expand Down Expand Up @@ -84,18 +86,6 @@ def _delete_dataset
def _update_dataset
apply_instance_filters(super)
end

# Raise an Error if calling deleting doesn't
# indicate that a single row was deleted.
def _delete
raise(Error, "No matching object for instance filtered dataset (SQL: #{_delete_dataset.delete_sql})") if super != 1
end

# Raise an Error if updating doesn't indicate that a single
# row was updated.
def _update(columns)
raise(Error, "No matching object for instance filtered dataset (SQL: #{_update_dataset.update_sql(columns)})") if super != 1
end
end
end
end
Expand Down
2 changes: 2 additions & 0 deletions lib/sequel/plugins/optimistic_locking.rb
Expand Up @@ -19,6 +19,8 @@ module Plugins
# class level accessor) that defaults to 0.
#
# This plugin relies on the instance_filters plugin.
# You must have require_modification on the class and instances
# that use this plugin.
module OptimisticLocking
# Exception class raised when trying to update or destroy a stale object.
Error = InstanceFilters::Error
Expand Down
2 changes: 2 additions & 0 deletions spec/extensions/caching_spec.rb
Expand Up @@ -57,10 +57,12 @@ def fetch_rows(sql)
def update(values)
$sqls << update_sql(values)
$cache_dataset_row.merge!(values)
1
end

def delete
$sqls << delete_sql
1
end
})

Expand Down
2 changes: 2 additions & 0 deletions spec/extensions/spec_helper.rb
Expand Up @@ -18,10 +18,12 @@ def insert(*args)

def update(*args)
@db.execute update_sql(*args)
1
end

def delete(*args)
@db.execute delete_sql(*args)
1
end

def fetch_rows(sql)
Expand Down
11 changes: 11 additions & 0 deletions spec/integration/model_test.rb
Expand Up @@ -75,6 +75,17 @@ class ::Item::Thing < Sequel::Model(@db[:items].select(:name))
i.save(:num)
Item.all.should == [Item.load(:id=>1, :name=>'K', :num=>2)]
end

specify "#save should check that the only a single row is modified, unless require_modification is false" do
i = Item.create(:name=>'a')
i.delete
proc{i.save}.should raise_error(Sequel::NoExistingObject)
proc{i.delete}.should raise_error(Sequel::NoExistingObject)

i.require_modification = false
i.save
i.delete
end

specify ".to_hash should return a hash keyed on primary key if no argument provided" do
i = Item.create(:name=>'J')
Expand Down
33 changes: 32 additions & 1 deletion spec/model/record_spec.rb
Expand Up @@ -97,6 +97,22 @@ def ds.insert_select(hash)
MODEL_DB.sqls.should == ["UPDATE items SET x = 1 WHERE (id = 3)"]
end

it "should raise a NoExistingObject exception if the dataset update call doesn't return 1, unless require_modification is false" do
o = @c.load(:id => 3, :x => 1)
o.this.meta_def(:update){|*a| 0}
proc{o.save}.should raise_error(Sequel::NoExistingObject)
o.this.meta_def(:update){|*a| 2}
proc{o.save}.should raise_error(Sequel::NoExistingObject)
o.this.meta_def(:update){|*a| 1}
proc{o.save}.should_not raise_error

o.require_modification = false
o.this.meta_def(:update){|*a| 0}
proc{o.save}.should_not raise_error
o.this.meta_def(:update){|*a| 2}
proc{o.save}.should_not raise_error
end

it "should update only the given columns if given" do
o = @c.load(:id => 3, :x => 1, :y => nil)
o.save(:y)
Expand Down Expand Up @@ -788,7 +804,7 @@ def o.modified?; false; end
MODEL_DB.reset
@model = Class.new(Sequel::Model(:items))
@model.columns :id
@model.dataset.meta_def(:delete) {MODEL_DB.execute delete_sql}
@model.dataset.meta_def(:delete){MODEL_DB.execute delete_sql;1}

@instance = @model.load(:id => 1234)
end
Expand All @@ -797,6 +813,21 @@ def o.modified?; false; end
@model.send(:define_method, :after_destroy){3}
@instance.destroy.should == @instance
end

it "should raise a NoExistingObject exception if the dataset delete call doesn't return 1" do
@instance.this.meta_def(:delete){|*a| 0}
proc{@instance.delete}.should raise_error(Sequel::NoExistingObject)
@instance.this.meta_def(:delete){|*a| 2}
proc{@instance.delete}.should raise_error(Sequel::NoExistingObject)
@instance.this.meta_def(:delete){|*a| 1}
proc{@instance.delete}.should_not raise_error

@instance.require_modification = false
@instance.this.meta_def(:delete){|*a| 0}
proc{@instance.delete}.should_not raise_error
@instance.this.meta_def(:delete){|*a| 2}
proc{@instance.delete}.should_not raise_error
end

it "should run within a transaction if use_transactions is true" do
@instance.use_transactions = true
Expand Down
2 changes: 2 additions & 0 deletions spec/model/spec_helper.rb
Expand Up @@ -15,10 +15,12 @@ def insert(*args)

def update(*args)
@db.execute update_sql(*args)
1
end

def delete(*args)
@db.execute delete_sql(*args)
1
end

def fetch_rows(sql)
Expand Down

0 comments on commit 21ef8d4

Please sign in to comment.