Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Raise on persisting invalid properties

This improves cases where Resource#{save,update} simply returns false
with no hint what was wrong. This mostly happens when developing custom
properties.
  • Loading branch information...
commit 8785fbe01a36f6b2bd5e32317bdd085e0147b433 1 parent 5362d49
Markus Schirp authored solnic committed
View
1  lib/dm-core.rb
@@ -69,6 +69,7 @@ def self.to_string(value)
require 'dm-core/resource/persistence_state/dirty'
require 'dm-core/property'
+require 'dm-core/property/invalid_value_error'
require 'dm-core/property/object'
require 'dm-core/property/string'
require 'dm-core/property/binary'
View
7 lib/dm-core/collection.rb
@@ -844,9 +844,10 @@ def update!(attributes)
if dirty_attributes.empty?
true
- elsif dirty_attributes.any? { |property, value| !property.valid?(value) }
- false
- else
+ else
+ dirty_attributes.each do |property, value|
+ property.assert_valid_value(value)
+ end
unless _update(dirty_attributes)
return false
end
View
17 lib/dm-core/property.rb
@@ -695,6 +695,23 @@ def valid?(value, negated = false)
end
end
+ # Asserts value is valid
+ #
+ # @param [Object] loaded_value
+ # the value to be tested
+ #
+ # @return [Boolean]
+ # true if the value is valid
+ #
+ # @raise [Property::InvalidValueError]
+ # if value is not valid
+ def assert_valid_value(value)
+ unless valid?(value)
+ raise Property::InvalidValueError.new(self,value)
+ end
+ true
+ end
+
# Returns a concise string representation of the property instance.
#
# @return [String]
View
20 lib/dm-core/property/invalid_value_error.rb
@@ -0,0 +1,20 @@
+module DataMapper
+ class Property
+ # Exception raised when DataMapper is about to work with
+ # invalid property values.
+ class InvalidValueError < StandardError
+ attr_reader :property, :value
+ def initialize(property,value)
+ msg = "Invalid value %s for property %s (%s) on model %s" %
+ [ value.inspect,
+ property.name.inspect,
+ property.class.name,
+ property.model.name
+ ]
+ super(msg)
+ @property = property
+ @value = value
+ end
+ end
+ end
+end
View
1  lib/dm-core/property/serial.rb
@@ -8,6 +8,7 @@ class Serial < Integer
def to_child_key
Property::Integer
end
+
end # class Text
end # module Property
end # module DataMapper
View
10 lib/dm-core/resource/persistence_state/dirty.rb
@@ -18,7 +18,7 @@ def delete
def commit
remove_from_identity_map
set_child_keys
- return self unless valid_attributes?
+ assert_valid_attributes
update_resource
reset_original_attributes
reset_resource_key
@@ -83,11 +83,11 @@ def reset_original_attributes
original_attributes.clear
end
- def valid_attributes?
- original_attributes.each_key do |property|
- return false if property.kind_of?(Property) && !property.valid?(property.get!(resource))
+ def assert_valid_attributes
+ properties.each do |property|
+ value = property.get! resource
+ property.assert_valid_value(value)
end
- true
end
end # class Dirty
View
10 lib/dm-core/resource/persistence_state/transient.rb
@@ -21,7 +21,7 @@ def delete
def commit
set_child_keys
set_default_values
- return self unless valid_attributes?
+ assert_valid_attributes
create_resource
set_repository
add_to_identity_map
@@ -65,10 +65,12 @@ def set_repository
resource.instance_variable_set(:@_repository, repository)
end
- def valid_attributes?
- properties.all? do |property|
+ def assert_valid_attributes
+ properties.each do |property|
value = get(property)
- property.serial? && value.nil? || property.valid?(value)
+ unless property.serial? && value.nil?
+ property.assert_valid_value(value)
+ end
end
end
View
24 lib/dm-core/spec/shared/resource_spec.rb
@@ -785,11 +785,9 @@
describe 'on a new, invalid resource' do
before :all do
@user = @user_model.new(:name => nil)
- @return = @user.__send__(method)
- end
-
- it 'should return false' do
- @return.should be(false)
+ expect { @user.__send__(method) }.to(raise_error(DataMapper::Property::InvalidValueError) do |error|
+ error.property.should == @user_model.properties[:name]
+ end)
end
it 'should call save hook expected number of times' do
@@ -805,7 +803,9 @@
end
it 'should not save an invalid resource' do
- @user.__send__(method).should be(false)
+ expect { @user.__send__(method) }.to(raise_error(DataMapper::Property::InvalidValueError) do |error|
+ error.property.should == @user_model.properties[:name]
+ end)
end
it 'should call save hook expected number of times' do
@@ -1161,14 +1161,10 @@
end
describe 'with attributes where a value is nil for a property that does not allow nil' do
- before :all do
- rescue_if @skip do
- @return = @user.__send__(method, :name => nil)
- end
- end
-
- it 'should return false' do
- @return.should be(false)
+ before do
+ expect { @user.__send__(method, :name => nil) }.to(raise_error(DataMapper::Property::InvalidValueError) do |error|
+ error.property.should == @user_model.properties[:name]
+ end)
end
it 'should not persist the changes' do
View
50 lib/dm-core/spec/shared/semipublic/property_spec.rb
@@ -123,4 +123,54 @@ class Article
end
end
end
+
+ describe '#assert_valid_value' do
+ subject do
+ @property.assert_valid_value(value)
+ end
+
+ shared_examples_for 'assert_valid_value on invalid value' do
+ it 'should raise DataMapper::Property::InvalidValueError' do
+ expect { subject }.to(raise_error(DataMapper::Property::InvalidValueError) do |error|
+ error.property.should == @property
+ end)
+ end
+ end
+
+ describe 'when provided a valid value' do
+ let(:value) { @value }
+
+ it 'should return true' do
+ subject.should be(true)
+ end
+ end
+
+ describe 'when provide an invalid value' do
+ let(:value) { @invalid_value }
+
+ it_should_behave_like 'assert_valid_value on invalid value'
+ end
+
+ describe 'when provide a nil value when required' do
+ before do
+ @property = @type.new(@model, @name, @options.merge(:required => true))
+ end
+
+ let(:value) { nil }
+
+ it_should_behave_like 'assert_valid_value on invalid value'
+ end
+
+ describe 'when provide a nil value when not required' do
+ before do
+ @property = @type.new(@model, @name, @options.merge(:required => false))
+ end
+
+ let(:value) { nil }
+
+ it 'should return true' do
+ subject.should be(true)
+ end
+ end
+ end
end
View
6 spec/public/model_spec.rb
@@ -297,7 +297,11 @@ def model?; true end
let(:attributes) { { :title => nil } }
let(:args) { [ attributes ] }
- it { should be(false) }
+ it 'should raise InvalidValueError' do
+ expect { subject }.to(raise_error(DataMapper::Property::InvalidValueError) do |error|
+ error.property.should == model.title
+ end)
+ end
end
end
end
View
6 spec/public/resource_spec.rb
@@ -166,9 +166,13 @@ class ::Default
it { should be(true) }
end
+ # FIXME: We cannot trigger a failing save with invalid properties anymore.
+ # Invalid properties will result in their own exception.
+ # So Im mocking here, but a better approach is needed.
+
describe 'and it is an invalid resource' do
before do
- @user.name = nil # name is required
+ @user.should_receive(:save_self).and_return(false)
end
it 'should raise an exception' do
View
8 spec/public/shared/collection_shared_spec.rb
@@ -1587,16 +1587,14 @@
describe 'with attributes where a required property is nil' do
before :all do
- @return = @articles.send(method, :title => nil)
+ expect { @articles.send(method, :title => nil) }.to(raise_error(DataMapper::Property::InvalidValueError) do |error|
+ error.property.should == @articles.model.title
+ end)
end
if method == :update!
should_not_be_a_kicker
end
-
- it 'should return false' do
- @return.should be(false)
- end
end
describe 'on a limited collection' do
View
12 spec/semipublic/resource/state/dirty_spec.rb
@@ -71,10 +71,16 @@ class ::Author
@resource.coding = 'yes'
end
- it { should equal(@state) }
+ it 'should raise InvalidValueError' do
+ expect { subject }.to(raise_error(DataMapper::Property::InvalidValueError) do |error|
+ error.property.should == Author.coding
+ end)
+ end
- it 'should update the resource to the identity map if the key changed' do
- method(:subject).should_not change { @resource.repository.identity_map(@model).dup }
+ it 'should not change the identity map' do
+ identity_map = @resource.repository.identity_map(@model).dup
+ expect { subject }.to raise_error
+ identity_map.should == @resource.repository.identity_map(@model)
end
end
end
Please sign in to comment.
Something went wrong with that request. Please try again.