From 993fa9290dd4298f168438c5195e7033c73f81c3 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Fri, 13 Oct 2017 09:28:27 -0400 Subject: [PATCH] additional verbosity for vars parsing exceptions --- awx/api/serializers.py | 16 ++++---- .../functional/api/test_job_runtime_params.py | 2 +- awx/main/tests/unit/utils/test_common.py | 40 +++++++++++++++++++ awx/main/utils/common.py | 28 ++++++++++--- awx/main/validators.py | 5 ++- 5 files changed, 73 insertions(+), 18 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index f0015d3b79..6640aa6e43 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -25,7 +25,7 @@ from django.utils.timezone import now from django.utils.functional import cached_property # Django REST Framework -from rest_framework.exceptions import ValidationError, PermissionDenied +from rest_framework.exceptions import ValidationError, PermissionDenied, ParseError from rest_framework import fields from rest_framework import serializers from rest_framework import validators @@ -3278,10 +3278,9 @@ class JobLaunchSerializer(BaseSerializer): try: extra_vars = parse_yaml_or_json(extra_vars, silent_failure=False) - except: - # Log exception, in case of yet-unknown parsing failures - logger.exception('extra_vars parsing error on Job Template launch.') - errors['extra_vars'] = _('Must be valid JSON or YAML.') + except ParseError as e: + # Catch known user variable formatting errors + errors['extra_vars'] = str(e) if self.get_survey_enabled(obj): validation_errors = obj.survey_variable_validation(extra_vars) @@ -3357,10 +3356,9 @@ class WorkflowJobLaunchSerializer(BaseSerializer): try: extra_vars = parse_yaml_or_json(extra_vars, silent_failure=False) - except: - # Log exception, in case of yet-unknown parsing failures - logger.exception('extra_vars parsing error on Workflow JT launch.') - errors['extra_vars'] = _('Must be valid JSON or YAML.') + except ParseError as e: + # Catch known user variable formatting errors + errors['extra_vars'] = str(e) if self.get_survey_enabled(obj): validation_errors = obj.survey_variable_validation(extra_vars) diff --git a/awx/main/tests/functional/api/test_job_runtime_params.py b/awx/main/tests/functional/api/test_job_runtime_params.py index b197ac2520..be6f0b56fa 100644 --- a/awx/main/tests/functional/api/test_job_runtime_params.py +++ b/awx/main/tests/functional/api/test_job_runtime_params.py @@ -199,7 +199,7 @@ def test_job_reject_invalid_prompted_extra_vars(runtime_data, job_template_promp dict(extra_vars='{"unbalanced brackets":'), admin_user, expect=400) assert 'extra_vars' in response.data - assert 'valid JSON or YAML' in str(response.data['extra_vars'][0]) + assert 'Cannot parse as' in str(response.data['extra_vars'][0]) @pytest.mark.django_db diff --git a/awx/main/tests/unit/utils/test_common.py b/awx/main/tests/unit/utils/test_common.py index 41f9012040..d31f2b7472 100644 --- a/awx/main/tests/unit/utils/test_common.py +++ b/awx/main/tests/unit/utils/test_common.py @@ -5,9 +5,13 @@ import os import pytest from uuid import uuid4 +import json +import yaml from django.core.cache import cache +from rest_framework.exceptions import ParseError + from awx.main.utils import common from awx.main.models import ( @@ -38,6 +42,42 @@ def test_parse_yaml_or_json(input_, output): assert common.parse_yaml_or_json(input_) == output +class TestParserExceptions: + + @staticmethod + def json_error(data): + try: + json.loads(data) + return None + except Exception as e: + return str(e) + + @staticmethod + def yaml_error(data): + try: + yaml.load(data) + return None + except Exception as e: + return str(e) + + def test_invalid_JSON_and_YAML(self): + data = "{key:val" + with pytest.raises(ParseError) as exc: + common.parse_yaml_or_json(data, silent_failure=False) + message = str(exc.value) + assert "Cannot parse as" in message + assert self.json_error(data) in message + assert self.yaml_error(data) in message + + def test_invalid_vars_type(self): + data = "[1, 2, 3]" + with pytest.raises(ParseError) as exc: + common.parse_yaml_or_json(data, silent_failure=False) + message = str(exc.value) + assert "Cannot parse as" in message + assert "Input type `list` is not a dictionary" in message + + def test_set_environ(): key = str(uuid4()) old_environ = os.environ.copy() diff --git a/awx/main/utils/common.py b/awx/main/utils/common.py index 87779041d2..902621ca62 100644 --- a/awx/main/utils/common.py +++ b/awx/main/utils/common.py @@ -582,12 +582,25 @@ def cache_list_capabilities(page, prefetch_list, model, user): obj.capabilities_cache[display_method] = True +def validate_vars_type(vars_obj): + if not isinstance(vars_obj, dict): + vars_type = type(vars_obj) + if hasattr(vars_type, '__name__'): + data_type = vars_type.__name__ + else: + data_type = str(vars_type) + raise AssertionError( + _('Input type `{data_type}` is not a dictionary').format( + data_type=data_type) + ) + + def parse_yaml_or_json(vars_str, silent_failure=True): ''' Attempt to parse a string of variables. First, with JSON parser, if that fails, then with PyYAML. If both attempts fail, return an empty dictionary if `silent_failure` - is True, re-raise the last error if `silent_failure` if False. + is True, re-raise combination error if `silent_failure` if False. ''' if isinstance(vars_str, dict): return vars_str @@ -596,18 +609,21 @@ def parse_yaml_or_json(vars_str, silent_failure=True): try: vars_dict = json.loads(vars_str) - assert isinstance(vars_dict, dict) - except (ValueError, TypeError, AssertionError): + validate_vars_type(vars_dict) + except (ValueError, TypeError, AssertionError) as json_err: try: vars_dict = yaml.safe_load(vars_str) # Can be None if '---' if vars_dict is None: return {} - assert isinstance(vars_dict, dict) - except (yaml.YAMLError, TypeError, AttributeError, AssertionError): + validate_vars_type(vars_dict) + except (yaml.YAMLError, TypeError, AttributeError, AssertionError) as yaml_err: if silent_failure: return {} - raise + raise ParseError(_( + 'Cannot parse as JSON (error: {json_error}) or ' + 'YAML (error: {yaml_error}).').format( + json_error=str(json_err), yaml_error=str(yaml_err))) return vars_dict diff --git a/awx/main/validators.py b/awx/main/validators.py index ce1c5b73b5..5af078a861 100644 --- a/awx/main/validators.py +++ b/awx/main/validators.py @@ -11,6 +11,7 @@ from django.core.exceptions import ValidationError # REST framework from rest_framework.serializers import ValidationError as RestValidationError +from rest_framework.exceptions import ParseError # AWX from awx.main.utils import parse_yaml_or_json @@ -183,6 +184,6 @@ def vars_validate_or_raise(vars_str): try: parse_yaml_or_json(vars_str, silent_failure=False) return vars_str - except: - raise RestValidationError(_('Must be valid JSON or YAML.')) + except ParseError as e: + raise RestValidationError(str(e))