Merge lp:~methanal-developers/methanal/validate-large-numbers into lp:methanal

[Jonathan Jacobs]

[Jonathan Jacobs (jjacobs)]

I have a few concerns that I'd like some input on:

  1. There is no realtime validation for these limits. I could pass the ranges through to the client and do some validation but for really large values (like 2 ** 63) Javascript has rounding errors (since numerics are all internally backed by a double) and so validation for the really large numbers breaks down because the ranges are now inaccurately represented. This may not be a concern since there is still validation on the server side and for smaller numbers we will have still accurate validation. Is this worth implementing?

  2. The message that comes back from the server side doesn't indicate what input generated the error (if an input did generate one), so getting an error like "ValueError: 5 is bigger than 4" is not likely to help anyone, really. But maybe implementing client side validation, as in my first point, would help for the cases where the limits are more likely to impact users.

[Tristan Seligmann (mithrandi)]

1. The precision errors (not rounding errors, technically) are somewhat of an issue with JavaScript, but they shouldn't pose a problem to doing validation, as long as you define the range exclusively instead of inclusively. In other words, -9223372036854775809 < n < 9223372036854775808. 9223372036854775808 will actually end up as 9223372036854776000, but that's fine; any number above 9223372036854775295 will end up as that, and thus be out of range. The only way to avoid this would be to have IntegerInput send the value to the server as a string, and only do the integer conversion there, but that would make writing validators somewhat challenging. Ugh. I just noticed DecimalInput suffers from pretty much the same precision problem; how have we not tripped over this already? Perhaps we should discuss this further in a different venue.

2. Hopefully hitting that error would be a relatively rare case; I'd be more concerned about the silent inaccuracy of JS numbers of such a magnitude. I suppose the "preview" value would at least expose the issue to the user, but it is somewhat subtle.

review: Needs Fixing

[Jonathan Jacobs (jjacobs)]

> 1. The precision errors (not rounding errors, technically) are somewhat of an
> issue with JavaScript, but they shouldn't pose a problem to doing validation,
> as long as you define the range exclusively instead of inclusively. In other
> words, -9223372036854775809 < n < 9223372036854775808. 9223372036854775808
> will actually end up as 9223372036854776000, but that's fine; any number above
> 9223372036854775295 will end up as that, and thus be out of range. The only
> way to avoid this would be to have IntegerInput send the value to the server
> as a string, and only do the integer conversion there, but that would make
> writing validators somewhat challenging. Ugh. I just noticed DecimalInput
> suffers from pretty much the same precision problem; how have we not tripped
> over this already? Perhaps we should discuss this further in a different
> venue.

Done.

> 2. Hopefully hitting that error would be a relatively rare case; I'd be more
> concerned about the silent inaccuracy of JS numbers of such a magnitude. I
> suppose the "preview" value would at least expose the issue to the user, but
> it is somewhat subtle.

As we discussed, the preview functionality has its own quirks. Hopefully nobody is using these inputs to for really large numbers.

[Tristan Seligmann (mithrandi)]

Looks good :)

review: Approve

=== modified file 'methanal/js/Methanal/Tests/TestView.js'
--- methanal/js/Methanal/Tests/TestView.js	2010-07-14 10:13:31 +0000
+++ methanal/js/Methanal/Tests/TestView.js	2010-07-19 13:49:42 +0000
@@ -979,6 +979,47 @@
                 self.assertInvalidInput(control, 'a');
                 self.assertInvalidInput(control, '1.2');
             });
+    },
+
+
+    /**
+     * L{Methanal.View.IntegerInput} validates that values fall within a certain
+     * exclusive range.
+     */
+    function test_bounds(self) {
+        self.testControl({value: null,
+                          lowerBound: -11,
+                          upperBound:   8,},
+            function (control) {
+                self.assertValidInput(control, '-10');
+                self.assertInvalidInput(control, '-11');
+                self.assertValidInput(control, '7');
+                self.assertInvalidInput(control, '8');
+            });
+    },
+
+
+    /**
+     * L{Methanal.View.IntegerInput} validates that values fall within a certain
+     * exclusive range, even for really big numbers that have precision
+     * problems in Javascript.
+     */
+    function test_bigBounds(self) {
+        // These turn into -9223372036854776000 and 9223372036854776000.
+        self.testControl({value: null,
+                          lowerBound: -9223372036854775809,
+                          upperBound:  9223372036854775808},
+            function (control) {
+                self.assertValidInput(control, null);
+                self.assertValidInput(control, '');
+                self.assertValidInput(control, '1');
+                self.assertInvalidInput(control, '-9223372036854775800');
+                self.assertInvalidInput(control, '-9223372036854775809');
+                self.assertInvalidInput(control, '-92233720368547758090');
+                self.assertInvalidInput(control,  '9223372036854775800');
+                self.assertInvalidInput(control,  '9223372036854775808');
+                self.assertInvalidInput(control,  '92233720368547758080');
+            });
     });
 
 
@@ -1055,6 +1096,55 @@
                 self.assertInvalidInput(control, 'a');
                 self.assertInvalidInput(control, '1.234');
             });
+    },
+
+
+    /**
+     * L{Methanal.View.DecimalInput} validates that values fall within a certain
+     * exclusive range.
+     */
+    function test_bounds(self) {
+        self.testControl({value: null,
+                          decimalPlaces: 2,
+                          lowerBound: -10.5,
+                          upperBound:   7.5,},
+            function (control) {
+                self.assertValidInput(control, '-10');
+                self.assertValidInput(control, '-10.4');
+                self.assertInvalidInput(control, '-11');
+                self.assertInvalidInput(control, '-10.6');
+                self.assertValidInput(control, '7');
+                self.assertValidInput(control, '7.4');
+                self.assertInvalidInput(control, '8');
+                self.assertInvalidInput(control, '7.6');
+            });
+    },
+
+
+    /**
+     * L{Methanal.View.DecimalInput} validates that values fall within a certain
+     * exclusive range, even for really big numbers that have precision
+     * problems in Javascript.
+     */
+    function test_bigBounds(self) {
+        // These turn into -9223372036854776000 and 9223372036854776000.
+        self.testControl({value: null,
+                          decimalPlaces: 2,
+                          lowerBound: -9223372036854775809.5,
+                          upperBound:  9223372036854775808.5},
+            function (control) {
+                self.assertValidInput(control, null);
+                self.assertValidInput(control, '');
+                self.assertValidInput(control, '1');
+                self.assertInvalidInput(control, '-9223372036854775800');
+                self.assertInvalidInput(control, '-9223372036854775809');
+                self.assertInvalidInput(control, '-9223372036854775808.5');
+                self.assertInvalidInput(control, '-92233720368547758090');
+                self.assertInvalidInput(control,  '9223372036854775800');
+                self.assertInvalidInput(control,  '9223372036854775808');
+                self.assertInvalidInput(control,  '9223372036854775807.5');
+                self.assertInvalidInput(control,  '92233720368547758080');
+            });
     });
 
 

=== modified file 'methanal/js/Methanal/View.js'
--- methanal/js/Methanal/View.js	2010-07-17 14:02:19 +0000
+++ methanal/js/Methanal/View.js	2010-07-19 13:49:42 +0000
@@ -2228,6 +2228,13 @@
  *     for validity
  */
 Methanal.View.TextInput.subclass(Methanal.View, 'NumericInput').methods(
+    function __init__(self, node, args) {
+        self.lowerBound = args.lowerBound;
+        self.upperBound = args.upperBound;
+        Methanal.View.NumericInput.upcall(self, '__init__', node, args);
+    },
+
+
     function getValue(self) {
         var value = Methanal.View.NumericInput.upcall(self, 'getValue');
         if (!value) {
@@ -2241,8 +2248,14 @@
 
 
     function baseValidator(self, value) {
-        if (value === undefined || isNaN(value)) {
+        if (value === null) {
+            return;
+        } else if (value === undefined || isNaN(value)) {
             return 'Numerical value only';
+        } else if (value <= self.lowerBound) {
+            return 'Value too small; must be greater than ' + self.lowerBound;
+        } else if (value >= self.upperBound) {
+            return 'Value too large; must be less than ' + self.upperBound;
         }
     });
 

=== modified file 'methanal/test/test_view.py'
--- methanal/test/test_view.py	2010-07-13 09:36:38 +0000
+++ methanal/test/test_view.py	2010-07-19 13:49:42 +0000
@@ -267,7 +267,55 @@
 
 
 
-class IntegerInputTests(FormInputTests):
+class NumericInputTestsMixin(object):
+    """
+    Tests mixin for numeric inputs.
+    """
+    def _testMinimumMaximum(self, control, acceptable, tooSmall, tooLarge):
+        """
+        Assert that C{control} sets its parameter's value to C{None} value or
+        C{acceptable}, when invoked with these values. When invoked with
+        C{tooSmall} or C{tooLarge} an exception is raised indicating that the
+        value is too small or too large.
+        """
+        param = control.parent.param
+
+        data = {param.name: None}
+        control.invoke(data)
+        self.assertIdentical(param.value, None)
+
+        data = {param.name: acceptable}
+        control.invoke(data)
+        self.assertEquals(param.value, acceptable)
+
+        data = {param.name: tooSmall}
+        e = self.assertRaises(ValueError, control.invoke, data)
+        self.assertIn('is smaller than', str(e))
+
+        data = {param.name: tooLarge}
+        e = self.assertRaises(ValueError, control.invoke, data)
+        self.assertIn('is larger than', str(e))
+
+
+    def test_minimumMaximumDefaultValues(self):
+        """
+        Numeric inputs default to not accepting values smaller than C{-(2 **
+        63)} or larger than C{2 ** 63 }.
+        """
+        control = self.createControl(dict())
+        self._testMinimumMaximum(control, 42, -(2 ** 63) - 1, 2 ** 63)
+
+
+    def test_minimumMaximumOverrideValues(self):
+        """
+        Numeric inputs can override their minimum and maximum value range.
+        """
+        control = self.createControl(dict(minimumValue=0, maximumValue=10))
+        self._testMinimumMaximum(control, 5, -1, 42)
+
+
+
+class IntegerInputTests(FormInputTests, NumericInputTestsMixin):
     """
     Tests for L{methanal.view.IntegerInput}.
     """
@@ -294,7 +342,7 @@
 
 
 
-class DecimalInputTests(FormInputTests):
+class DecimalInputTests(FormInputTests, NumericInputTestsMixin):
     """
     Tests for L{methanal.view.DecimalInput}.
     """

=== modified file 'methanal/view.py'
--- methanal/view.py	2010-07-13 11:43:58 +0000
+++ methanal/view.py	2010-07-19 13:49:42 +0000
@@ -632,12 +632,70 @@
 
 
 
-class IntegerInput(TextInput):
+class NumericInput(TextInput):
+    """
+    Base class for numeric inputs.
+
+    @ivar minimumValue: Minimum allowed value, defaults to the smallest integer
+        value allowed in SQLite.
+
+    @ivar maximumValue: Maximum allowed value, defaults to the largest integer
+        value allowed in SQLite.
+    """
+    MINIMUM = -9223372036854775808
+    MAXIMUM =  9223372036854775807
+
+
+    def __init__(self, minimumValue=None, maximumValue=None, **kw):
+        super(NumericInput, self).__init__(**kw)
+        if minimumValue is None:
+            minimumValue = self.MINIMUM
+        self.minimumValue = minimumValue
+
+        if maximumValue is None:
+            maximumValue = self.MAXIMUM
+        self.maximumValue = maximumValue
+
+
+    def getArgs(self):
+        return {
+            u'lowerBound': self.minimumValue - 1,
+            u'upperBound': self.maximumValue + 1}
+
+
+    def checkValue(self, value):
+        """
+        Check that C{value} is within the minimum and maximum ranges. Raise
+        C{ValueError} if C{value} is outside of the allowed range.
+        """
+        if value is None:
+            return
+        if value > self.maximumValue:
+            raise ValueError('%s is larger than %s' % (
+                value, self.maximumValue))
+        elif value < self.minimumValue:
+            raise ValueError('%s is smaller than %s' % (
+                value, self.minimumValue))
+
+
+    def convertValue(self, value):
+        return value
+
+
+    def invoke(self, data):
+        value = self.convertValue(data[self.param.name])
+        self.checkValue(value)
+        self.param.value = value
+
+
+
+class IntegerInput(NumericInput):
     """
     Integer input.
     """
     jsClass = u'Methanal.View.IntegerInput'
 
+
     def getValue(self):
         value = self.param.value
         if value is None:
@@ -646,7 +704,7 @@
 
 
 
-class DecimalInput(TextInput):
+class DecimalInput(NumericInput):
     """
     Decimal input.
     """
@@ -667,6 +725,12 @@
         self.decimalPlaces = decimalPlaces
 
 
+    def convertValue(self, value):
+        if value is not None:
+            value = Decimal(str(value))
+        return value
+
+
     def getArgs(self):
         return {u'decimalPlaces': self.decimalPlaces}
 
@@ -679,14 +743,6 @@
         return float(value)
 
 
-    def invoke(self, data):
-        value = data[self.param.name]
-        if value is None:
-            self.param.value = None
-        else:
-            self.param.value = Decimal(str(value))
-
-
 
 class PercentInput(DecimalInput):
     """