Tighten CouldNotSetAttributeError restriction

Why:

* Previously, `allow_value` would raise a CouldNotSetAttributeError
  if the value being set didn't match the value the attribute had after
  being set, but only if the attribute was being changed from nil to
  non-nil or non-nil to nil.
* It turns out it doesn't matter which value you're trying to set the
  attribute to -- if the attribute rejects that change it's confusing
  either way. (In fact, I was recently bit by a case in which I was
  trying to validate numericality of an attribute, where the writer
  method for that attribute was overridden to ensure that the attribute
  always stored a number and never contained non-number characters.
  This ended up making the numericality validation useless, of
  course -- but it caused confusion because the test acted in a way
  I didn't expect.)

To satisfy the above:

* `allow_value` now raises a CouldNotSetAttributeError if the attribute
  rejects the value being set in *any* way.
* However, add a `ignoring_interference_by_writer` qualifier so that
  it is possible to manually override this behavior.
* Fix tests that are failing now because of this new change:
  * Fix tests for allow_value matcher
  * Fix tests for numericality matcher
  * Remove tests for numericality matcher + integer column
    * An integer column will typecast any non-integer value to an
      integer.
    * Because of the typecasting, our tests for the numericality matcher
      against an integer column don't quite work, because we can't
      really test what happens when the attribute is set to a
      non-integer value. Now that `allow_value` is more strict, we're
      getting a CouldNotSetAttributeError when attempting to do so.
    * The tests mentioned were originally added to ensure that we are
      handling RangeErrors that ActiveRecord used to emit. This doesn't
      happen anymore, so the tests aren't necessary anymore either.
  * Fix tests for acceptance matcher
  * Fix tests for absence matcher

NEWS.md

@@ -53,24 +53,102 @@
 
 * There are two changes to `allow_value`:
 
-  * The negative form of the matcher has been changed so that instead of
+  * The negative form of `allow_value` has been changed so that instead of
     asserting that any of the given values is an invalid value (allowing good
     values to pass through), assert that *all* values are invalid values
     (allowing good values not to pass through). This means that this test which
     formerly passed will now fail:
 
-        expect(record).not_to allow_value('good value', *bad_values)
+    ``` ruby
+    expect(record).not_to allow_value('good value', *bad_values)
+    ```
 
     ([19ce8a6])
 
-  * The matcher may raise an error if the attribute in question contains
-    custom logic to ignore certain values, resulting in a discrepancy between
-    the value you provide and the value that the attribute is actually set to.
-    Specifically, if the attribute cannot be changed from a non-nil value to a
-    nil value, or vice versa, then you'll get a CouldNotSetAttributeError. The
-    current behavior (which is to permit this) is misleading, as the test that
-    you're writing under the hood by using `allow_value` could be different from
-    the test that actually ends up getting run. ([eaaa2d8])
+  * `allow_value` now raises a CouldNotSetAttributeError if in setting the
+    attribute, the value of the attribute from reading the attribute back is
+    different from the one used to set it.
+
+    This would happen if the writer method for that attribute has custom logic
+    to ignore certain incoming values or change them in any way. Here are three
+    examples we've seen:
+
+    * You're attempting to assert that an attribute should not allow nil, yet
+      the attribute's writer method contains a conditional to do nothing if
+      the attribute is set to nil:
+
+      ``` ruby
+      class Foo
+        include ActiveModel::Model
+
+        attr_reader :bar
+
+        def bar=(value)
+          return if value.nil?
+          @bar = value
+        end
+      end
+
+      describe Foo do
+        it do
+          foo = Foo.new
+          foo.bar = "baz"
+          # This will raise a CouldNotSetAttributeError since `foo.bar` is now "123"
+          expect(foo).not_to allow_value(nil).for(:bar)
+        end
+      end
+      ```
+
+    * You're attempting to assert that an numeric attribute should not allow a
+      string that contains non-numeric characters, yet the writer method for
+      that attribute strips out non-numeric characters:
+
+      ``` ruby
+      class Foo
+        include ActiveModel::Model
+
+        attr_reader :bar
+
+        def bar=(value)
+          @bar = value.gsub(/\D+/, '')
+        end
+      end
+
+      describe Foo do
+        it do
+          foo = Foo.new
+          # This will raise a CouldNotSetAttributeError since `foo.bar` is now "123"
+          expect(foo).not_to allow_value("abc123").for(:bar)
+        end
+      end
+      ```
+
+    * You're passing a value to `allow_value` that the model typecasts into
+      another value:
+
+      ``` ruby
+      describe Foo do
+        # Assume that `attr` is a string
+        # This will raise a CouldNotSetAttributeError since `attr` typecasts `[]` to `"[]"`
+        it { should_not allow_value([]).for(:attr) }
+      end
+      ```
+
+    With all of these failing examples, why are we making this change? We want
+    to guard you (as the developer) from writing a test that you think acts one
+    way but actually acts a different way, as this could lead to a confusing
+    false positive or negative.
+
+    If you understand the problem and wish to override this behavior so that
+    you do not get a CouldNotSetAttributeError, you can add the
+    `ignoring_interference_by_writer` qualifier like so. Note that this will not
+    always cause the test to pass.
+
+    ``` ruby
+    it { should_not allow_value([]).for(:attr).ignoring_interference_by_writer }
+    ```
+
+    ([eaaa2d8])
 
 * `validate_uniqueness_of` is now properly case-insensitive by default, to match
   the default behavior of the validation itself. This is a backward-incompatible

lib/shoulda/matchers/active_model/allow_value_matcher.rb

@@ -226,6 +226,7 @@ def initialize(*values)
           self.options = {}
           self.after_setting_value_callback = -> {}
           self.validator = Validator.new
+          @ignoring_interference_by_writer = false
         end
 
         def for(attribute)
@@ -255,6 +256,11 @@ def strict
           self
         end
 
+        def ignoring_interference_by_writer
+          @ignoring_interference_by_writer = true
+          self
+        end
+
         def _after_setting_value(&callback)
           self.after_setting_value_callback = callback
         end
@@ -297,6 +303,10 @@ def attribute_to_check_message_against=(attribute)
           validator.attribute = attribute
         end
 
+        def ignoring_interference_by_writer?
+          @ignoring_interference_by_writer
+        end
+
         def value_matches?(value)
           self.value = value
           set_attribute(value)
@@ -305,14 +315,14 @@ def value_matches?(value)
 
         def set_attribute(value)
           instance.__send__("#{attribute_to_set}=", value)
-          ensure_that_attribute_has_been_changed_to_or_from_nil!(value)
+          ensure_that_attribute_was_set!(value)
           after_setting_value_callback.call
         end
 
-        def ensure_that_attribute_has_been_changed_to_or_from_nil!(expected_value)
+        def ensure_that_attribute_was_set!(expected_value)
           actual_value = instance.__send__(attribute_to_set)
 
-          if expected_value.nil? != actual_value.nil?
+          if expected_value != actual_value && !ignoring_interference_by_writer?
             raise CouldNotSetAttributeError.create(
               instance.class,
               attribute_to_set,

lib/shoulda/matchers/active_model/disallow_value_matcher.rb

@@ -31,6 +31,11 @@ def with_message(message, options={})
           self
         end
 
+        def ignoring_interference_by_writer
+          @allow_matcher.ignoring_interference_by_writer
+          self
+        end
+
         def failure_message
           @allow_matcher.failure_message_when_negated
         end

lib/shoulda/matchers/active_model/numericality_matchers/comparison_matcher.rb

@@ -80,7 +80,11 @@ def last_failing_submatcher
           def submatchers
             @_submatchers ||=
               comparison_combos.map do |diff, submatcher_method_name|
-                matcher = __send__(submatcher_method_name, @value + diff, nil)
+                matcher = __send__(
+                  submatcher_method_name,
+                  (@value + diff).to_s,
+                  nil
+                )
                 matcher.with_message(@message, values: { count: @value })
                 matcher
               end

lib/shoulda/matchers/active_model/numericality_matchers/even_number_matcher.rb

@@ -8,9 +8,10 @@ class EvenNumberMatcher < NumericTypeMatcher
 
           def initialize(attribute, options = {})
             @attribute = attribute
-            @disallow_value_matcher = DisallowValueMatcher.new(NON_EVEN_NUMBER_VALUE).
-                for(@attribute).
-                with_message(:even)
+            @disallow_value_matcher =
+              DisallowValueMatcher.new(NON_EVEN_NUMBER_VALUE.to_s).
+              for(@attribute).
+              with_message(:even)
           end
 
           def allowed_type

lib/shoulda/matchers/active_model/numericality_matchers/odd_number_matcher.rb

@@ -8,9 +8,10 @@ class OddNumberMatcher < NumericTypeMatcher
 
           def initialize(attribute, options = {})
             @attribute = attribute
-            @disallow_value_matcher = DisallowValueMatcher.new(NON_ODD_NUMBER_VALUE).
-                for(@attribute).
-                with_message(:odd)
+            @disallow_value_matcher =
+              DisallowValueMatcher.new(NON_ODD_NUMBER_VALUE.to_s).
+              for(@attribute).
+              with_message(:odd)
           end
 
           def allowed_type

lib/shoulda/matchers/active_model/numericality_matchers/only_integer_matcher.rb

@@ -7,7 +7,8 @@ class OnlyIntegerMatcher < NumericTypeMatcher
           NON_INTEGER_VALUE = 0.1
           def initialize(attribute)
             @attribute = attribute
-            @disallow_value_matcher = DisallowValueMatcher.new(NON_INTEGER_VALUE).
+            @disallow_value_matcher =
+              DisallowValueMatcher.new(NON_INTEGER_VALUE.to_s).
               for(attribute).
               with_message(:not_an_integer)
           end

lib/shoulda/matchers/active_model/validate_absence_of_matcher.rb

@@ -86,6 +86,7 @@ def with_message(message)
         def matches?(subject)
           super(subject)
           @expected_message ||= :present
+
           disallows_value_of(value, @expected_message)
         end
 
@@ -103,21 +104,22 @@ def value
             else
               obj
             end
-          elsif [Fixnum, Float].include?(attribute_class)
-            1
-          elsif attribute_class == BigDecimal
-            BigDecimal.new(1, 0)
-          elsif !attribute_class || attribute_class == String
-            'an arbitrary value'
           else
-            attribute_class.new
+            case column_type
+            when :integer, :float then 1
+            when :decimal then BigDecimal.new(1, 0)
+            when :datetime, :time, :timestamp then Time.now
+            when :date then Date.new
+            when :binary then "0"
+            else 'an arbitrary value'
+            end
           end
         end
 
-        def attribute_class
+        def column_type
           @subject.class.respond_to?(:columns_hash) &&
-            @subject.class.columns_hash[@attribute.to_s].respond_to?(:klass) &&
-            @subject.class.columns_hash[@attribute.to_s].klass
+            @subject.class.columns_hash[@attribute.to_s].respond_to?(:type) &&
+            @subject.class.columns_hash[@attribute.to_s].type
         end
 
         def collection?

lib/shoulda/matchers/error.rb

@@ -20,6 +20,10 @@ def initialize(*args)
       def message
         ""
       end
+
+      def inspect
+        %(#<#{self.class}: #{message}>)
+      end
     end
   end
 end