Fix numericality matcher w/ numeric columns

Fix the matcher so it still raises a CouldNotSetAttributeError against a
numeric column, but only if the matcher has not been qualified at all.
When the matcher is qualified with anything else, then it's okay to use
a numeric column, as long as the matcher no longer asserts that the
record disallows a non-numeric value.

NEWS.md

@@ -4,17 +4,24 @@
 
 * Fix `validate_inclusion_of` + `in_array` when used against a date or datetime
   column/attribute so that it does not raise a CouldNotSetAttributeError.
-  ([8fa97b4])
+  ([#783], [8fa97b4])
+
+* Fix `validate_numericality_of` when used against a numeric column so that it
+  no longer raises a CouldNotSetAttributeError if the matcher has been qualified
+  in any way (`only_integer`, `greater_than`, `odd`, etc.). ([#784])
 
 ### Improvements
 
 * `validate_uniqueness_of` now raises a NonCaseSwappableValueError if the value
   the matcher is using to test uniqueness cannot be case-swapped -- in other
   words, if it doesn't contain any alpha characters. When this is the case, the
-  matcher cannot work effectively. ([#789])
+  matcher cannot work effectively. ([#789], [ada9bd3])
 
-[#789]: /~https://github.com/thoughtbot/shoulda-matchers/pull/789
+[#783]: /~https://github.com/thoughtbot/shoulda-matchers/pull/783
 [8fa97b4]: /~https://github.com/thoughtbot/shoulda-matchers/commit/8fa97b4ff33b57ce16dfb96be1ec892502f2aa9e
+[#784]: /~https://github.com/thoughtbot/shoulda-matchers/pull/784
+[#789]: /~https://github.com/thoughtbot/shoulda-matchers/pull/789
+[ada9bd3]: /~https://github.com/thoughtbot/shoulda-matchers/commit/ada9bd3a1b9f2bb9fa74d0dfe1f8f7080314298c
 
 # 3.0.0
 

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

@@ -20,7 +20,6 @@ def initialize(numericality_matcher, value, operator)
             @value = value
             @operator = operator
             @message = ERROR_MESSAGES[operator]
-            @comparison_combos = comparison_combos
             @strict = false
           end
 
@@ -80,11 +79,7 @@ def last_failing_submatcher
           def submatchers
             @_submatchers ||=
               comparison_combos.map do |diff, submatcher_method_name|
-                matcher = __send__(
-                  submatcher_method_name,
-                  (@value + diff).to_s,
-                  nil
-                )
+                matcher = __send__(submatcher_method_name, diff, nil)
                 matcher.with_message(@message, values: { count: @value })
                 matcher
               end
@@ -127,8 +122,14 @@ def assertions
           end
 
           def diffs_to_compare
-            diff = @numericality_matcher.diff_to_compare
-            [-diff, 0, diff]
+            diff_to_compare = @numericality_matcher.diff_to_compare
+            values = [-1, 0, 1].map { |sign| @value + (diff_to_compare * sign) }
+
+            if @numericality_matcher.given_numeric_column?
+              values
+            else
+              values.map(&:to_s)
+            end
           end
 
           def comparison_expectation

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

@@ -6,21 +6,27 @@ module NumericalityMatchers
         class EvenNumberMatcher < NumericTypeMatcher
           NON_EVEN_NUMBER_VALUE = 1
 
-          def initialize(attribute, options = {})
-            @attribute = attribute
-            @disallow_value_matcher =
-              DisallowValueMatcher.new(NON_EVEN_NUMBER_VALUE.to_s).
-              for(@attribute).
-              with_message(:even)
-          end
-
           def allowed_type
             'even numbers'
           end
 
           def diff_to_compare
             2
           end
+
+          protected
+
+          def wrap_disallow_value_matcher(matcher)
+            matcher.with_message(:even)
+          end
+
+          def disallowed_value
+            if @numeric_type_matcher.given_numeric_column?
+              NON_EVEN_NUMBER_VALUE
+            else
+              NON_EVEN_NUMBER_VALUE.to_s
+            end
+          end
         end
       end
     end

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

@@ -1,29 +1,37 @@
+require 'forwardable'
+
 module Shoulda
   module Matchers
     module ActiveModel
       module NumericalityMatchers
         # @private
         class NumericTypeMatcher
-          def initialize
-            raise NotImplementedError
-          end
+          extend Forwardable
+
+          def_delegators :disallow_value_matcher, :matches?, :failure_message,
+            :failure_message_when_negated
 
-          def matches?(subject)
-            @disallow_value_matcher.matches?(subject)
+          def initialize(numeric_type_matcher, attribute, options = {})
+            @numeric_type_matcher = numeric_type_matcher
+            @attribute = attribute
+            @options = options
+            @message = nil
+            @context = nil
+            @strict = false
           end
 
           def with_message(message)
-            @disallow_value_matcher.with_message(message)
+            @message = message
             self
           end
 
           def strict
-            @disallow_value_matcher.strict
+            @strict = true
             self
           end
 
           def on(context)
-            @disallow_value_matcher.on(context)
+            @context = context
             self
           end
 
@@ -35,12 +43,39 @@ def diff_to_compare
             raise NotImplementedError
           end
 
-          def failure_message
-            @disallow_value_matcher.failure_message
+          protected
+
+          attr_reader :attribute
+
+          def wrap_disallow_value_matcher(matcher)
+            raise NotImplementedError
+          end
+
+          def disallowed_value
+            raise NotImplementedError
           end
 
-          def failure_message_when_negated
-            @disallow_value_matcher.failure_message_when_negated
+          private
+
+          def disallow_value_matcher
+            @_disallow_value_matcher ||= begin
+              DisallowValueMatcher.new(disallowed_value).tap do |matcher|
+                matcher.for(attribute)
+                wrap_disallow_value_matcher(matcher)
+
+                if @message
+                  matcher.with_message(@message)
+                end
+
+                if @strict
+                  matcher.strict
+                end
+
+                if @context
+                  matcher.on(@context)
+                end
+              end
+            end
           end
         end
       end

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

@@ -4,15 +4,7 @@ module ActiveModel
       module NumericalityMatchers
         # @private
         class OddNumberMatcher < NumericTypeMatcher
-          NON_ODD_NUMBER_VALUE  = 2
-
-          def initialize(attribute, options = {})
-            @attribute = attribute
-            @disallow_value_matcher =
-              DisallowValueMatcher.new(NON_ODD_NUMBER_VALUE.to_s).
-              for(@attribute).
-              with_message(:odd)
-          end
+          NON_ODD_NUMBER_VALUE = 2
 
           def allowed_type
             'odd numbers'
@@ -21,6 +13,20 @@ def allowed_type
           def diff_to_compare
             2
           end
+
+          protected
+
+          def wrap_disallow_value_matcher(matcher)
+            matcher.with_message(:odd)
+          end
+
+          def disallowed_value
+            if @numeric_type_matcher.given_numeric_column?
+              NON_ODD_NUMBER_VALUE
+            else
+              NON_ODD_NUMBER_VALUE.to_s
+            end
+          end
         end
       end
     end

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

@@ -5,13 +5,6 @@ module NumericalityMatchers
         # @private
         class OnlyIntegerMatcher < NumericTypeMatcher
           NON_INTEGER_VALUE = 0.1
-          def initialize(attribute)
-            @attribute = attribute
-            @disallow_value_matcher =
-              DisallowValueMatcher.new(NON_INTEGER_VALUE.to_s).
-              for(attribute).
-              with_message(:not_an_integer)
-          end
 
           def allowed_type
             'integers'
@@ -20,6 +13,20 @@ def allowed_type
           def diff_to_compare
             1
           end
+
+          protected
+
+          def wrap_disallow_value_matcher(matcher)
+            matcher.with_message(:not_an_integer)
+          end
+
+          def disallowed_value
+            if @numeric_type_matcher.given_numeric_column?
+              NON_INTEGER_VALUE
+            else
+              NON_INTEGER_VALUE.to_s
+            end
+          end
         end
       end
     end

lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb

@@ -315,18 +315,19 @@ def initialize(attribute)
           @submatchers = []
           @diff_to_compare = DEFAULT_DIFF_TO_COMPARE
           @strict = false
+
           add_disallow_value_matcher
         end
 
         def strict
-          @submatchers.each(&:strict)
           @strict = true
+          @submatchers.each(&:strict)
           self
         end
 
         def only_integer
           prepare_submatcher(
-            NumericalityMatchers::OnlyIntegerMatcher.new(@attribute)
+            NumericalityMatchers::OnlyIntegerMatcher.new(self, @attribute)
           )
           self
         end
@@ -342,14 +343,14 @@ def allow_nil
 
         def odd
           prepare_submatcher(
-            NumericalityMatchers::OddNumberMatcher.new(@attribute)
+            NumericalityMatchers::OddNumberMatcher.new(self, @attribute)
           )
           self
         end
 
         def even
           prepare_submatcher(
-            NumericalityMatchers::EvenNumberMatcher.new(@attribute)
+            NumericalityMatchers::EvenNumberMatcher.new(self, @attribute)
           )
           self
         end
@@ -391,6 +392,19 @@ def on(context)
 
         def matches?(subject)
           @subject = subject
+
+          if given_numeric_column?
+            remove_disallow_value_matcher
+          end
+
+          if @submatchers.empty?
+            raise IneffectiveTestError.create(
+              model: @subject.class,
+              attribute: @attribute,
+              column_type: column_type
+            )
+          end
+
           first_failing_submatcher.nil?
         end
 
@@ -417,8 +431,18 @@ def failure_message_when_negated
           first_failing_submatcher.failure_message_when_negated
         end
 
+        def given_numeric_column?
+          [:integer, :float].include?(column_type)
+        end
+
         private
 
+        def column_type
+          if @subject.class.respond_to?(:columns_hash)
+            @subject.class.columns_hash[@attribute.to_s].type
+          end
+        end
+
         def add_disallow_value_matcher
           disallow_value_matcher = DisallowValueMatcher.new(NON_NUMERIC_VALUE).
             for(@attribute).
@@ -427,8 +451,13 @@ def add_disallow_value_matcher
           add_submatcher(disallow_value_matcher)
         end
 
+        def remove_disallow_value_matcher
+          @submatchers.shift
+        end
+
         def prepare_submatcher(submatcher)
           add_submatcher(submatcher)
+
           if submatcher.respond_to?(:diff_to_compare)
             update_diff_to_compare(submatcher)
           end
@@ -476,6 +505,32 @@ def submatcher_comparison_descriptions
             arr
           end
         end
+
+        class IneffectiveTestError < Shoulda::Matchers::Error
+          attr_accessor :model, :attribute, :column_type
+
+          def message
+            Shoulda::Matchers.word_wrap \
+<<MESSAGE
+You are attempting to use validate_numericality_of, but the attribute you're
+testing, :#{attribute}, is a #{column_type} column. One of the things that the
+numericality matcher does is to assert that setting :#{attribute} to a string
+that doesn't look like a #{column_type} will cause your
+#{humanized_model_name} to become invalid. In this case, it's impossible to make
+this assertion since :#{attribute} will typecast any incoming value to a
+#{column_type}. This means that it's already guaranteed to be numeric!
+Since this matcher isn't doing anything, you can remove it from your model
+tests, and in fact, you can remove the validation from your model as it isn't
+doing anything either.
+MESSAGE
+          end
+
+          private
+
+          def humanized_model_name
+            model.name.humanize.downcase
+          end
+        end
       end
     end
   end

spec/unit/shoulda/matchers/active_model/numericality_matchers/comparison_matcher_spec.rb

@@ -283,6 +283,6 @@ def build_matcher(operator: :==, value: 0)
   end
 
   def numericality_matcher
-    double(diff_to_compare: 1)
+    double(diff_to_compare: 1, given_numeric_column?: nil)
   end
 end