Merge pull request #406 from AlanCoding/variables_debt

Consolidation of variables parsing throughout codebase

awx/api/serializers.py

@@ -11,9 +11,6 @@ import urllib
 from collections import OrderedDict
 from dateutil import rrule
 
-# PyYAML
-import yaml
-
 # Django
 from django.conf import settings
 from django.contrib.auth import authenticate
@@ -28,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
@@ -1312,6 +1309,9 @@ class HostSerializer(BaseSerializerWithVariables):
             raise serializers.ValidationError({"detail": _("Cannot create Host for Smart Inventory")})
         return value
 
+    def validate_variables(self, value):
+        return vars_validate_or_raise(value)
+
     def validate(self, attrs):
         name = force_text(attrs.get('name', self.instance and self.instance.name or ''))
         host, port = self._get_host_port_from_name(name)
@@ -1319,19 +1319,9 @@ class HostSerializer(BaseSerializerWithVariables):
         if port:
             attrs['name'] = host
             variables = force_text(attrs.get('variables', self.instance and self.instance.variables or ''))
-            try:
+            vars_dict = parse_yaml_or_json(variables)
-                vars_dict = json.loads(variables.strip() or '{}')
+            vars_dict['ansible_ssh_port'] = port
-                vars_dict['ansible_ssh_port'] = port
+            attrs['variables'] = json.dumps(vars_dict)
-                attrs['variables'] = json.dumps(vars_dict)
-            except (ValueError, TypeError):
-                try:
-                    vars_dict = yaml.safe_load(variables)
-                    if vars_dict is None:
-                        vars_dict = {}
-                    vars_dict['ansible_ssh_port'] = port
-                    attrs['variables'] = yaml.dump(vars_dict)
-                except (yaml.YAMLError, TypeError):
-                    raise serializers.ValidationError({'variables': _('Must be valid JSON or YAML.')})
 
         return super(HostSerializer, self).validate(attrs)
 
@@ -3326,18 +3316,11 @@ class JobLaunchSerializer(BaseSerializer):
 
         extra_vars = attrs.get('extra_vars', {})
 
-        if isinstance(extra_vars, basestring):
+        try:
-            try:
+            extra_vars = parse_yaml_or_json(extra_vars, silent_failure=False)
-                extra_vars = json.loads(extra_vars)
+        except ParseError as e:
-            except (ValueError, TypeError):
+            # Catch known user variable formatting errors
-                try:
+            errors['extra_vars'] = str(e)
-                    extra_vars = yaml.safe_load(extra_vars)
-                    assert isinstance(extra_vars, dict)
-                except (yaml.YAMLError, TypeError, AttributeError, AssertionError):
-                    errors['extra_vars'] = _('Must be valid JSON or YAML.')
-
-        if not isinstance(extra_vars, dict):
-            extra_vars = {}
 
         if self.get_survey_enabled(obj):
             validation_errors = obj.survey_variable_validation(extra_vars)
@@ -3411,18 +3394,11 @@ class WorkflowJobLaunchSerializer(BaseSerializer):
 
         extra_vars = attrs.get('extra_vars', {})
 
-        if isinstance(extra_vars, basestring):
+        try:
-            try:
+            extra_vars = parse_yaml_or_json(extra_vars, silent_failure=False)
-                extra_vars = json.loads(extra_vars)
+        except ParseError as e:
-            except (ValueError, TypeError):
+            # Catch known user variable formatting errors
-                try:
+            errors['extra_vars'] = str(e)
-                    extra_vars = yaml.safe_load(extra_vars)
-                    assert isinstance(extra_vars, dict)
-                except (yaml.YAMLError, TypeError, AttributeError, AssertionError):
-                    errors['extra_vars'] = _('Must be valid JSON or YAML.')
-
-        if not isinstance(extra_vars, dict):
-            extra_vars = {}
 
         if self.get_survey_enabled(obj):
             validation_errors = obj.survey_variable_validation(extra_vars)

awx/main/models/base.py

@@ -1,13 +1,6 @@
 # Copyright (c) 2015 Ansible, Inc.
 # All Rights Reserved.
 
-# Python
-import json
-import shlex
-
-# PyYAML
-import yaml
-
 # Django
 from django.db import models
 from django.core.exceptions import ValidationError, ObjectDoesNotExist
@@ -21,7 +14,7 @@ from taggit.managers import TaggableManager
 from crum import get_current_user
 
 # AWX
-from awx.main.utils import encrypt_field
+from awx.main.utils import encrypt_field, parse_yaml_or_json
 
 __all__ = ['prevent_search', 'VarsDictProperty', 'BaseModel', 'CreatedModifiedModel',
            'PasswordFieldsModel', 'PrimordialModel', 'CommonModel',
@@ -80,26 +73,7 @@ class VarsDictProperty(object):
         if hasattr(v, 'items'):
             return v
         v = v.encode('utf-8')
-        d = None
+        return parse_yaml_or_json(v)
-        try:
-            d = json.loads(v.strip() or '{}')
-        except ValueError:
-            pass
-        if d is None:
-            try:
-                d = yaml.safe_load(v)
-                # This can happen if the whole file is commented out
-                if d is None:
-                    d = {}
-            except yaml.YAMLError:
-                pass
-        if d is None and self.key_value:
-            d = {}
-            for kv in [x.decode('utf-8') for x in shlex.split(v, posix=True)]:
-                if '=' in kv:
-                    k, v = kv.split('=', 1)
-                    d[k] = v
-        return d if hasattr(d, 'items') else {}
 
     def __set__(self, obj, value):
         raise AttributeError('readonly property')

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

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()

awx/main/utils/common.py

@@ -582,21 +582,48 @@ def cache_list_capabilities(page, prefetch_list, model, user):
                 obj.capabilities_cache[display_method] = True
 
 
-def parse_yaml_or_json(vars_str):
+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 with variables, and if attempt fails,
+    Attempt to parse a string of variables.
-    return an empty dictionary.
+    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 combination error if `silent_failure` if False.
     '''
     if isinstance(vars_str, dict):
         return vars_str
+    elif isinstance(vars_str, basestring) and vars_str == '""':
+        return {}
+
     try:
         vars_dict = json.loads(vars_str)
-    except (ValueError, TypeError):
+        validate_vars_type(vars_dict)
+    except (ValueError, TypeError, AssertionError) as json_err:
         try:
             vars_dict = yaml.safe_load(vars_str)
-            assert isinstance(vars_dict, dict)
+            # Can be None if '---'
-        except (yaml.YAMLError, TypeError, AttributeError, AssertionError):
+            if vars_dict is None:
-            vars_dict = {}
+                return {}
+            validate_vars_type(vars_dict)
+        except (yaml.YAMLError, TypeError, AttributeError, AssertionError) as yaml_err:
+            if silent_failure:
+                return {}
+            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
 
 

awx/main/validators.py

@@ -4,8 +4,6 @@
 # Python
 import base64
 import re
-import yaml
-import json
 
 # Django
 from django.utils.translation import ugettext_lazy as _
@@ -13,6 +11,10 @@ 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
 
 
 def validate_pem(data, min_keys=0, max_keys=None, min_certs=0, max_certs=None):
@@ -179,21 +181,9 @@ def vars_validate_or_raise(vars_str):
     job templates, inventories, or hosts are either an acceptable
     blank string, or are valid JSON or YAML dict
     """
-    if isinstance(vars_str, dict) or (isinstance(vars_str, basestring) and vars_str == '""'):
+    try:
+        parse_yaml_or_json(vars_str, silent_failure=False)
         return vars_str
-
+    except ParseError as e:
-    try:
+        raise RestValidationError(str(e))
-        r = json.loads((vars_str or '').strip() or '{}')
-        if isinstance(r, dict):
-            return vars_str
-    except ValueError:
-        pass
-    try:
-        r = yaml.safe_load(vars_str)
-        # Can be None if '---'
-        if isinstance(r, dict) or r is None:
-            return vars_str
-    except yaml.YAMLError:
-        pass
-    raise RestValidationError(_('Must be valid JSON or YAML.'))