On Thu, 2012-06-14 at 09:05 -0400, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Wed, 2012-06-13 at 18:05 -0400, Rob Crittenden wrote:
> >> Martin Kosek wrote:
> >>> 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
> >>
> >> Getting some lint errors. Ran out of time to investigate but its strange
> >> because AFAICT these are members.
> >>
> >> ipalib/parameters.py:1266: [E1101, Decimal._enforce_numberclass]
> >> Instance of 'Decimal' has no 'numberclass' member
> >> ipalib/parameters.py:1271: [E1101, Decimal._enforce_numberclass]
> >> Instance of 'Decimal' has no 'numberclass' member
> >> ipalib/parameters.py:1288: [E1101, Decimal._remove_exponent] Instance of
> >> 'Decimal' has no 'exponential' member
> >>
> >
> > I assume you run this lint:
> >
> > # ./make-lint ipalib/parameters.py
> >
> > This produces a lot of false positive lint errors... But if you run lint
> > for the entire project, my patches will pass:
> >
> > # git
> > apply
> > /home/mkosek/freeipa-mkosek-275-2-decimal-parameter-conversion-and-normalization.patch
> > # ./make-lint
> > #
> >
> > That's why 275-2 includes a change for make-lint to not report these new
> > attributes:
> > diff --git a/make-lint b/make-lint
> > index
> > 7ecd59d7e8c5a644f812d4b8987866e7d06236b5..30c5e00c1f0606c75ff1f7fec675ff673a6b87a0
> > 100755
> > --- a/make-lint
> > +++ b/make-lint
> > @@ -61,7 +61,8 @@ class IPATypeChecker(TypeChecker):
> > 'csv', 'csv_separator', 'csv_skipspace'],
> > 'ipalib.parameters.Bool': ['truths', 'falsehoods'],
> > 'ipalib.parameters.Int': ['minvalue', 'maxvalue'],
> > - 'ipalib.parameters.Decimal': ['minvalue', 'maxvalue', 'precision'],
> > + 'ipalib.parameters.Decimal': ['minvalue', 'maxvalue', 'precision',
> > + 'numberclass', 'exponential'],
> > 'ipalib.parameters.Data': ['minlength', 'maxlength', 'length',
> > 'pattern', 'pattern_errmsg'],
> > 'ipalib.parameters.Enum': ['values'],
> >
> >
> > Bottom line - I do not think there is a lint problem with my patch :-)
> >
> > Martin
> >
>
> This was output from './make-lint'. It blew up when I was trying to
> build the rpms.
>
> rob
Hm, I guess you have a different version of pylint, mine is error-less
(I use pylint-0.25.1-1.fc17.noarch).
Anyway, I have disabled the false positive pylint errors you reported,
it should not hurt and will keep your build clean. Updated patch is
attached.
Martin
>From d9f3d1450eb733c21fd4f7e922e5bc870c2bc4af 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 | 54 ++++++++++++++++++--
tests/test_ipalib/test_parameters.py | 93 +++++++++++++++++++++++++++++++++-
2 files changed, 141 insertions(+), 6 deletions(-)
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index cdc991f48183db8955172b5f970cba03cf86ed54..98b02dd6d48a332eb407dd836e55c8291b128df2 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,70 @@ class Decimal(Number):
maxvalue=self.maxvalue,
)
+ def _enforce_numberclass(self, value):
+ #pylint: disable=E1101
+ 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: #pylint: disable=E1101
+ 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
