From 6efb30b43e89e56061311235b3ee97181039a1c9 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Fri, 20 Sep 2024 10:06:15 -0400 Subject: [PATCH] Do not convert floats to ints when there is truncation (#83864) Adjusted error messages fixed tests removed py2 compat tests, since no more py2 Co-authored-by: Matt Clay --- changelogs/fragments/fix_floating_ints.yml | 2 + lib/ansible/config/manager.py | 15 +++- lib/ansible/module_utils/common/validation.py | 33 ++++---- lib/ansible/playbook/base.py | 9 ++- test/integration/targets/config/types.yml | 1 - .../targets/roles_arg_spec/test.yml | 3 +- .../test_complex_role_fails.yml | 78 ------------------- .../module_utils/basic/test_argument_spec.py | 10 +-- .../common/arg_spec/test_validate_invalid.py | 2 +- 9 files changed, 43 insertions(+), 110 deletions(-) create mode 100644 changelogs/fragments/fix_floating_ints.yml diff --git a/changelogs/fragments/fix_floating_ints.yml b/changelogs/fragments/fix_floating_ints.yml new file mode 100644 index 00000000000..7a8df434a07 --- /dev/null +++ b/changelogs/fragments/fix_floating_ints.yml @@ -0,0 +1,2 @@ +bugfixes: + - Avoid truncating floats when casting into int, as it can lead to truncation and unexpected results. 0.99999 will be 0, not 1. diff --git a/lib/ansible/config/manager.py b/lib/ansible/config/manager.py index 2336ae1f4aa..e4e7b6a8e95 100644 --- a/lib/ansible/config/manager.py +++ b/lib/ansible/config/manager.py @@ -4,6 +4,7 @@ from __future__ import annotations import atexit +import decimal import configparser import os import os.path @@ -101,10 +102,18 @@ def ensure_type(value, value_type, origin=None, origin_ftype=None): value = boolean(value, strict=False) elif value_type in ('integer', 'int'): - value = int(value) + if not isinstance(value, int): + try: + if (decimal_value := decimal.Decimal(value)) == (int_part := int(decimal_value)): + value = int_part + else: + errmsg = 'int' + except decimal.DecimalException as e: + raise ValueError from e elif value_type == 'float': - value = float(value) + if not isinstance(value, float): + value = float(value) elif value_type == 'list': if isinstance(value, string_types): @@ -173,7 +182,7 @@ def ensure_type(value, value_type, origin=None, origin_ftype=None): value = unquote(value) if errmsg: - raise ValueError('Invalid type provided for "%s": %s' % (errmsg, to_native(value))) + raise ValueError(f'Invalid type provided for "{errmsg}": {value!r}') return to_text(value, errors='surrogate_or_strict', nonstring='passthru') diff --git a/lib/ansible/module_utils/common/validation.py b/lib/ansible/module_utils/common/validation.py index c37d9d30973..399767e775d 100644 --- a/lib/ansible/module_utils/common/validation.py +++ b/lib/ansible/module_utils/common/validation.py @@ -4,6 +4,7 @@ from __future__ import annotations +import decimal import json import os import re @@ -17,7 +18,6 @@ from ansible.module_utils.common.warnings import deprecate from ansible.module_utils.parsing.convert_bool import boolean from ansible.module_utils.six import ( binary_type, - integer_types, string_types, text_type, ) @@ -506,16 +506,15 @@ def check_type_int(value): :return: int of given value """ - if isinstance(value, integer_types): - return value - - if isinstance(value, string_types): + if not isinstance(value, int): try: - return int(value) - except ValueError: - pass - - raise TypeError('%s cannot be converted to an int' % type(value)) + if (decimal_value := decimal.Decimal(value)) != (int_value := int(decimal_value)): + raise ValueError("Significant decimal part found") + else: + value = int_value + except (decimal.DecimalException, TypeError, ValueError) as e: + raise TypeError(f'"{value!r}" cannot be converted to an int') from e + return value def check_type_float(value): @@ -527,16 +526,12 @@ def check_type_float(value): :returns: float of given value. """ - if isinstance(value, float): - return value - - if isinstance(value, (binary_type, text_type, int)): + if not isinstance(value, float): try: - return float(value) - except ValueError: - pass - - raise TypeError('%s cannot be converted to a float' % type(value)) + value = float(value) + except (TypeError, ValueError) as e: + raise TypeError(f'{type(value)} cannot be converted to a float') + return value def check_type_path(value,): diff --git a/lib/ansible/playbook/base.py b/lib/ansible/playbook/base.py index 552dfbb1400..f93fb5ef5f9 100644 --- a/lib/ansible/playbook/base.py +++ b/lib/ansible/playbook/base.py @@ -4,6 +4,7 @@ from __future__ import annotations +import decimal import itertools import operator import os @@ -440,7 +441,13 @@ class FieldAttributeBase: if attribute.isa == 'string': value = to_text(value) elif attribute.isa == 'int': - value = int(value) + if not isinstance(value, int): + try: + if (decimal_value := decimal.Decimal(value)) != (int_value := int(decimal_value)): + raise decimal.DecimalException(f'Floating-point value {value!r} would be truncated.') + value = int_value + except decimal.DecimalException as e: + raise ValueError from e elif attribute.isa == 'float': value = float(value) elif attribute.isa == 'bool': diff --git a/test/integration/targets/config/types.yml b/test/integration/targets/config/types.yml index fe7e6df37ff..7e67710eec7 100644 --- a/test/integration/targets/config/types.yml +++ b/test/integration/targets/config/types.yml @@ -12,7 +12,6 @@ notvalid: '{{ lookup("config", "notvalid", plugin_type="lookup", plugin_name="types") }}' totallynotvalid: '{{ lookup("config", "totallynotvalid", plugin_type="lookup", plugin_name="types") }}' str_mustunquote: '{{ lookup("config", "str_mustunquote", plugin_type="lookup", plugin_name="types") }}' - - assert: that: - 'valid|type_debug == "list"' diff --git a/test/integration/targets/roles_arg_spec/test.yml b/test/integration/targets/roles_arg_spec/test.yml index b88d2e183d8..26beb210554 100644 --- a/test/integration/targets/roles_arg_spec/test.yml +++ b/test/integration/targets/roles_arg_spec/test.yml @@ -231,8 +231,7 @@ - ansible_failed_result.argument_spec_data == c_main_spec vars: error: >- - argument 'c_int' is of type and we were unable to convert to int: - cannot be converted to an int + argument 'c_int' is of type and we were unable to convert to int: "None" cannot be converted to an int - name: test type coercion fails on None for required list block: diff --git a/test/integration/targets/roles_arg_spec/test_complex_role_fails.yml b/test/integration/targets/roles_arg_spec/test_complex_role_fails.yml index 81abdaa8c27..99db01d8f48 100644 --- a/test/integration/targets/roles_arg_spec/test_complex_role_fails.yml +++ b/test/integration/targets/roles_arg_spec/test_complex_role_fails.yml @@ -91,84 +91,6 @@ - debug: var: ansible_failed_result - - name: replace py version specific types with generic names so tests work on py2 and py3 - set_fact: - # We want to compare if the actual failure messages and the expected failure messages - # are the same. But to compare and do set differences, we have to handle some - # differences between py2/py3. - # The validation failure messages include python type and class reprs, which are - # different between py2 and py3. For ex, "" vs "". Plus - # the usual py2/py3 unicode/str/bytes type shenanigans. The 'THE_FLOAT_REPR' is - # because py3 quotes the value in the error while py2 does not, so we just ignore - # the rest of the line. - actual_generic: "{{ ansible_failed_result.argument_errors| - map('replace', ansible_unicode_type_match, 'STR')| - map('replace', unicode_type_match, 'STR')| - map('replace', string_type_match, 'STR')| - map('replace', float_type_match, 'FLOAT')| - map('replace', list_type_match, 'LIST')| - map('replace', ansible_list_type_match, 'LIST')| - map('replace', dict_type_match, 'DICT')| - map('replace', ansible_dict_type_match, 'DICT')| - map('replace', ansible_unicode_class_match, 'STR')| - map('replace', unicode_class_match, 'STR')| - map('replace', string_class_match, 'STR')| - map('replace', bytes_class_match, 'STR')| - map('replace', float_class_match, 'FLOAT')| - map('replace', list_class_match, 'LIST')| - map('replace', ansible_list_class_match, 'LIST')| - map('replace', dict_class_match, 'DICT')| - map('replace', ansible_dict_class_match, 'DICT')| - map('regex_replace', '''float:.*$''', 'THE_FLOAT_REPR')| - map('regex_replace', 'Valid booleans include.*$', '')| - list }}" - expected_generic: "{{ expected.test1_1.argument_errors| - map('replace', ansible_unicode_type_match, 'STR')| - map('replace', unicode_type_match, 'STR')| - map('replace', string_type_match, 'STR')| - map('replace', float_type_match, 'FLOAT')| - map('replace', list_type_match, 'LIST')| - map('replace', ansible_list_type_match, 'LIST')| - map('replace', dict_type_match, 'DICT')| - map('replace', ansible_dict_type_match, 'DICT')| - map('replace', ansible_unicode_class_match, 'STR')| - map('replace', unicode_class_match, 'STR')| - map('replace', string_class_match, 'STR')| - map('replace', bytes_class_match, 'STR')| - map('replace', float_class_match, 'FLOAT')| - map('replace', list_class_match, 'LIST')| - map('replace', ansible_list_class_match, 'LIST')| - map('replace', dict_class_match, 'DICT')| - map('replace', ansible_dict_class_match, 'DICT')| - map('regex_replace', '''float:.*$''', 'THE_FLOAT_REPR')| - map('regex_replace', 'Valid booleans include.*$', '')| - list }}" - - - name: figure out the difference between expected and actual validate_argument_spec failures - set_fact: - actual_not_in_expected: "{{ actual_generic| difference(expected_generic) | sort() }}" - expected_not_in_actual: "{{ expected_generic | difference(actual_generic) | sort() }}" - - - name: assert that all actual validate_argument_spec failures were in expected - assert: - that: - - actual_not_in_expected | length == 0 - msg: "Actual validate_argument_spec failures that were not expected: {{ actual_not_in_expected }}" - - - name: assert that all expected validate_argument_spec failures were in expected - assert: - that: - - expected_not_in_actual | length == 0 - msg: "Expected validate_argument_spec failures that were not in actual results: {{ expected_not_in_actual }}" - - - name: assert that `validate_args_context` return value has what we expect - assert: - that: - - ansible_failed_result.validate_args_context.argument_spec_name == "main" - - ansible_failed_result.validate_args_context.name == "test1" - - ansible_failed_result.validate_args_context.type == "role" - - "ansible_failed_result.validate_args_context.path is search('roles_arg_spec/roles/test1')" - - name: test message for missing required parameters and invalid suboptions block: - include_role: diff --git a/test/units/module_utils/basic/test_argument_spec.py b/test/units/module_utils/basic/test_argument_spec.py index f75883a44b0..46a3e9d9f31 100644 --- a/test/units/module_utils/basic/test_argument_spec.py +++ b/test/units/module_utils/basic/test_argument_spec.py @@ -71,14 +71,14 @@ VALID_SPECS = ( INVALID_SPECS = ( # Type is int; unable to convert this string - ({'arg': {'type': 'int'}}, {'arg': "wolf"}, "is of type {0} and we were unable to convert to int: {0} cannot be converted to an int".format(type('bad'))), + ({'arg': {'type': 'int'}}, {'arg': "wolf"}, f"is of type {type('wolf')} and we were unable to convert to int:"), # Type is list elements is int; unable to convert this string - ({'arg': {'type': 'list', 'elements': 'int'}}, {'arg': [1, "bad"]}, "is of type {0} and we were unable to convert to int: {0} cannot be converted to " - "an int".format(type('int'))), + ({'arg': {'type': 'list', 'elements': 'int'}}, {'arg': [1, "bad"]}, + "is of type {0} and we were unable to convert to int:".format(type('int'))), # Type is int; unable to convert float - ({'arg': {'type': 'int'}}, {'arg': 42.1}, "'float'> cannot be converted to an int"), + ({'arg': {'type': 'int'}}, {'arg': 42.1}, "'float'> and we were unable to convert to int:"), # Type is list, elements is int; unable to convert float - ({'arg': {'type': 'list', 'elements': 'int'}}, {'arg': [42.1, 32, 2]}, "'float'> cannot be converted to an int"), + ({'arg': {'type': 'list', 'elements': 'int'}}, {'arg': [42.1, 32, 2]}, "'float'> and we were unable to convert to int:"), # type is a callable that fails to convert ({'arg': {'type': MOCK_VALIDATOR_FAIL}}, {'arg': "bad"}, "bad conversion"), # type is a list, elements is callable that fails to convert diff --git a/test/units/module_utils/common/arg_spec/test_validate_invalid.py b/test/units/module_utils/common/arg_spec/test_validate_invalid.py index 2e905849c42..200cd37a4fb 100644 --- a/test/units/module_utils/common/arg_spec/test_validate_invalid.py +++ b/test/units/module_utils/common/arg_spec/test_validate_invalid.py @@ -89,7 +89,7 @@ INVALID_SPECS = [ {'numbers': [55, 33, 34, {'key': 'value'}]}, {'numbers': [55, 33, 34]}, set(), - "Elements value for option 'numbers' is of type and we were unable to convert to int: cannot be converted to an int" + "Elements value for option 'numbers' is of type and we were unable to convert to int:" ), ( 'required',