On Thu, 2012-06-07 at 22:38 -0400, Rob Crittenden wrote:
> Martin Kosek wrote:
> > When invalid data is passed, an unhandled decimal exception could
> > be raised in Decimal number conversion. Handle the exception
> > more gracefully and report proper ipalib.errors.ConversionError.
> >
> > https://fedorahosted.org/freeipa/ticket/2705
> 
> I'm being pedantic but I think the Decimal special values need to be 
> handled better. Using Infinity returns a rather odd message:
> 
> $ ipa dnsrecord-add example.com
> Record name: foo
> Please choose a type of DNS resource record to be added
> The most common types for this type of zone are: A, AAAA
> 
> DNS resource record type: LOC
> LOC Degrees Latitude: 90
> [LOC Minutes Latitude]: 59
> [LOC Seconds Latitude]: 
> 999999999999999999999999999999999999999999999999999999999999999999999
>  >>> LOC Seconds Latitude: quantize result has too many digits for 
> current context
> [LOC Seconds Latitude]: Infinity
>  >>> LOC Seconds Latitude: quantize with one INF
> 
> And using NaN raises an unhandled exception:
> 
> [LOC Seconds Latitude]: NaN
> ipa: ERROR: InvalidOperation: comparison involving NaN
> Traceback (most recent call last):
>    File "/home/rcrit/redhat/freeipa/ipalib/cli.py", line 1263, in run
>      sys.exit(api.Backend.cli.run(argv))
>    File "/home/rcrit/redhat/freeipa/ipalib/cli.py", line 1049, in run
>      kw = self.argv_to_keyword_arguments(cmd, argv[1:])
>    File "/home/rcrit/redhat/freeipa/ipalib/cli.py", line 1035, in 
> argv_to_keyword_arguments
>      self.prompt_interactively(cmd, kw)
>    File "/home/rcrit/redhat/freeipa/ipalib/cli.py", line 1199, in 
> prompt_interactively
>      callback(kw)
>    File "/home/rcrit/redhat/freeipa/ipalib/plugins/dns.py", line 2147, 
> in interactive_prompt_callback
>      user_options = param.prompt_parts(self.Backend)
>    File "/home/rcrit/redhat/freeipa/ipalib/plugins/dns.py", line 768, in 
> prompt_parts
>      self.__get_part_param(backend, part, user_options, default)
>    File "/home/rcrit/redhat/freeipa/ipalib/plugins/dns.py", line 747, in 
> __get_part_param
>      output_kw[name] = part(raw)
>    File "/home/rcrit/redhat/freeipa/ipalib/parameters.py", line 556, in 
> __call__
>      self.validate(value, supplied=self.name in kw)
>    File "/home/rcrit/redhat/freeipa/ipalib/parameters.py", line 879, in 
> validate
>      self._validate_scalar(value)
>    File "/home/rcrit/redhat/freeipa/ipalib/parameters.py", line 893, in 
> _validate_scalar
>      error = rule(ugettext, value)
>    File "/home/rcrit/redhat/freeipa/ipalib/parameters.py", line 1244, in 
> _rule_minvalue
>      if value < self.minvalue:
>    File "/usr/lib64/python2.7/decimal.py", line 884, in __lt__
>      ans = self._compare_check_nans(other, context)
>    File "/usr/lib64/python2.7/decimal.py", line 786, in _compare_check_nans
>      self)
>    File "/usr/lib64/python2.7/decimal.py", line 3866, in _raise_error
>      raise error(explanation)
> InvalidOperation: comparison involving NaN
> ipa: ERROR: an internal error has occurred
> 
> Otherwise it does what it should.
> 
> rob

Thanks for being pedantic, I found out that Decimal number validation
and normalization needs more care, dnsrecord-add would also fail with
values such as "1E4" or "-0".

Attached patch improves Decimal number validation a lot and adds
optional exponent normalization. I also added missing tests for all
Decimal Parameter attributes.

Martin
>From 14e6092a141f0aeec8154e3761ffe55b42c138f7 Mon Sep 17 00:00:00 2001
From: Martin Kosek <mko...@redhat.com>
Date: Thu, 7 Jun 2012 09:25:19 +0200
Subject: [PATCH] Decimal parameter conversion and normalization

Parameter Decimal does not have a sufficient value checks. Some values
cause Decimal parameter with a custom precision to crash with
an unhandled exception.

Improve parameter conversion and normalization operations to handle
decimal exceptions more gracefully. Decimal parameter now also has
new attributes enabling 2 new validation/normalization methods:
 * exponential: when False, decimal number is normalized to its
                non-exponential form
 * numberclass: a set of allowed decimal number classes
                (e.g. +Infinity, -Normal, ...) that are enforced
                for every Decimal parameter value

https://fedorahosted.org/freeipa/ticket/2705
---
 ipalib/parameters.py                 |   53 +++++++++++++++++--
 tests/test_ipalib/test_parameters.py |   93 +++++++++++++++++++++++++++++++++-
 2 files changed, 140 insertions(+), 6 deletions(-)

diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index cdc991f48183db8955172b5f970cba03cf86ed54..1975a5b8db9b9b0156521e42ac73a24ab963905c 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -1206,7 +1206,12 @@ class Decimal(Number):
     kwargs = Param.kwargs + (
         ('minvalue', decimal.Decimal, None),
         ('maxvalue', decimal.Decimal, None),
+        # round Decimal to given precision
         ('precision', int, None),
+        # when False, number is normalized to non-exponential form
+        ('exponential', bool, False),
+        # set of allowed decimal number classes
+        ('numberclass', tuple, ('-Normal', '+Zero', '+Normal')),
     )
 
     def __init__(self, name, *rules, **kw):
@@ -1256,31 +1261,69 @@ class Decimal(Number):
                 maxvalue=self.maxvalue,
             )
 
+    def _enforce_numberclass(self, value):
+        numberclass = value.number_class()
+        if numberclass not in self.numberclass:
+            raise ValidationError(name=self.get_param_name(),
+                    error=_("number class '%(cls)s' is not included in a list "
+                            "of allowed number classes: %(allowed)s") \
+                            % dict(cls=numberclass,
+                                   allowed=u', '.join(self.numberclass))
+                )
+
     def _enforce_precision(self, value):
         assert type(value) is decimal.Decimal
         if self.precision is not None:
             quantize_exp = decimal.Decimal(10) ** -self.precision
-            return value.quantize(quantize_exp)
+            try:
+                value = value.quantize(quantize_exp)
+            except decimal.DecimalException, e:
+                raise ConversionError(name=self.get_param_name(),
+                                      error=unicode(e))
+        return value
 
+    def _remove_exponent(self, value):
+        assert type(value) is decimal.Decimal
+
+        if not self.exponential:
+            try:
+                # adopted from http://docs.python.org/library/decimal.html
+                value = value.quantize(decimal.Decimal(1)) \
+                        if value == value.to_integral() \
+                        else value.normalize()
+            except decimal.DecimalException, e:
+                raise ConversionError(name=self.get_param_name(),
+                                      error=unicode(e))
+
+        return value
+
+    def _test_and_normalize(self, value):
+        """
+        This method is run in conversion and normalization methods to test
+        that the Decimal number conforms to Parameter boundaries and then
+        normalizes the value.
+        """
+        self._enforce_numberclass(value)
+        value = self._remove_exponent(value)
+        value = self._enforce_precision(value)
         return value
 
     def _convert_scalar(self, value, index=None):
         if isinstance(value, (basestring, float)):
             try:
                 value = decimal.Decimal(value)
-            except Exception, e:
+            except decimal.DecimalException, e:
                 raise ConversionError(name=self.get_param_name(), index=index,
                                       error=unicode(e))
 
         if isinstance(value, decimal.Decimal):
-            x = self._enforce_precision(value)
-            return x
+            return self._test_and_normalize(value)
 
         return super(Decimal, self)._convert_scalar(value, index)
 
     def _normalize_scalar(self, value):
         if isinstance(value, decimal.Decimal):
-            value = self._enforce_precision(value)
+            return self._test_and_normalize(value)
 
         return super(Decimal, self)._normalize_scalar(value)
 
diff --git a/tests/test_ipalib/test_parameters.py b/tests/test_ipalib/test_parameters.py
index fc9569e736fd6e3b862b9add5c5dd3c95f5ed45d..0b6fae375639ee0e012a9cee12311adc62b63934 100644
--- a/tests/test_ipalib/test_parameters.py
+++ b/tests/test_ipalib/test_parameters.py
@@ -32,7 +32,7 @@ from tests.util import dummy_ugettext, assert_equal
 from tests.data import binary_bytes, utf8_bytes, unicode_str
 from ipalib import parameters, text, errors, config
 from ipalib.constants import TYPE_ERROR, CALLABLE_ERROR, NULLS
-from ipalib.errors import ValidationError
+from ipalib.errors import ValidationError, ConversionError
 from ipalib import _
 from xmlrpclib import MAXINT, MININT
 
@@ -1358,6 +1358,97 @@ class test_Decimal(ClassChecker):
             assert dummy.called() is True
             dummy.reset()
 
+    def test_precision(self):
+        """
+        Test the `ipalib.parameters.Decimal` precision attribute
+        """
+        # precission is None
+        param = self.cls('my_number')
+
+        for value in (Decimal('0'), Decimal('4.4'), Decimal('4.67')):
+            assert_equal(
+                param(value),
+                value)
+
+        # precision is 0
+        param = self.cls('my_number', precision=0)
+        for original,expected in ((Decimal('0'), '0'),
+                                  (Decimal('1.1'), '1'),
+                                  (Decimal('4.67'), '5')):
+            assert_equal(
+                str(param(original)),
+                expected)
+
+        # precision is 1
+        param = self.cls('my_number', precision=1)
+        for original,expected in ((Decimal('0'), '0.0'),
+                                  (Decimal('1.1'), '1.1'),
+                                  (Decimal('4.67'), '4.7')):
+            assert_equal(
+                str(param(original)),
+                expected)
+
+        # value has too many digits
+        param = self.cls('my_number', precision=1)
+        e = raises(ConversionError, param, '123456789012345678901234567890')
+
+        assert str(e) == \
+        "invalid 'my_number': quantize result has too many digits for current context"
+
+    def test_exponential(self):
+        """
+        Test the `ipalib.parameters.Decimal` exponential attribute
+        """
+        param = self.cls('my_number', exponential=True)
+        for original,expected in ((Decimal('0'), '0'),
+                                  (Decimal('1E3'), '1E+3'),
+                                  (Decimal('3.4E2'), '3.4E+2')):
+            assert_equal(
+                str(param(original)),
+                expected)
+
+
+        param = self.cls('my_number', exponential=False)
+        for original,expected in ((Decimal('0'), '0'),
+                                  (Decimal('1E3'), '1000'),
+                                  (Decimal('3.4E2'), '340')):
+            assert_equal(
+                str(param(original)),
+                expected)
+
+    def test_numberclass(self):
+        """
+        Test the `ipalib.parameters.Decimal` numberclass attribute
+        """
+        # test default value: '-Normal', '+Zero', '+Normal'
+        param = self.cls('my_number')
+        for value,raises_verror in ((Decimal('0'), False),
+                                    (Decimal('-0'), True),
+                                    (Decimal('1E8'), False),
+                                    (Decimal('-1.1'), False),
+                                    (Decimal('-Infinity'), True),
+                                    (Decimal('+Infinity'), True),
+                                    (Decimal('NaN'), True)):
+            if raises_verror:
+                raises(ValidationError, param, value)
+            else:
+                param(value)
+
+
+        param = self.cls('my_number', exponential=True,
+                numberclass=('-Normal', '+Zero', '+Infinity'))
+        for value,raises_verror in ((Decimal('0'), False),
+                                    (Decimal('-0'), True),
+                                    (Decimal('1E8'), True),
+                                    (Decimal('-1.1'), False),
+                                    (Decimal('-Infinity'), True),
+                                    (Decimal('+Infinity'), False),
+                                    (Decimal('NaN'), True)):
+            if raises_verror:
+                raises(ValidationError, param, value)
+            else:
+                param(value)
+
 class test_AccessTime(ClassChecker):
     """
     Test the `ipalib.parameters.AccessTime` class.
-- 
1.7.7.6

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to