From 7ea99ecbf56aa40f8f714f05b25b79fd3da89309 Mon Sep 17 00:00:00 2001
From: Jarrod Johnson <jjohnson2@lenovo.com>
Date: Fri, 26 Jan 2018 14:16:23 -0500
Subject: [PATCH] Improve expression error handling

First, refactor the attrname translation to be in a single method.

Recognize if 'None' comes back rather than a dictionary with na 'value'

Add a proper ValueError if an invalid attribute name is used.

Properly catch and transform ValueErrors in the API
---
 .../confluent/config/configmanager.py         | 37 +++++++++++--------
 .../plugins/configuration/attributes.py       | 13 +++++--
 2 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/confluent_server/confluent/config/configmanager.py b/confluent_server/confluent/config/configmanager.py
index 233ba9a6..ad028b2a 100644
--- a/confluent_server/confluent/config/configmanager.py
+++ b/confluent_server/confluent/config/configmanager.py
@@ -233,14 +233,9 @@ def decrypt_value(cryptvalue,
 
 def fixup_attribute(attrname, attrval):
     # Normalize some data, for example strings and numbers to bool
-    if attrname.startswith('custom.'):
+    attrname = _get_valid_attrname(attrname)
+    if attrname not in allattributes.node:  # no fixup possible
         return attrval
-    if attrname.startswith('net.'):
-        # For net.* attribtues, split on the dots and put back together
-        # longer term we might want a generic approach, but
-        # right now it's just net. attributes
-        netattrparts = attrname.split('.')
-        attrname = netattrparts[0] + '.' + netattrparts[-1]
     if 'type' in allattributes.node[attrname] and not isinstance(attrval, allattributes.node[attrname]['type']):
         if (allattributes.node[attrname]['type'] == bool and
                 (isinstance(attrval, str) or isinstance(attrval, unicode))):
@@ -253,12 +248,7 @@ def attribute_is_invalid(attrname, attrval):
         # No type checking or name checking is provided for custom,
         # it's not possible
         return False
-    if attrname.startswith('net.'):
-        # For net.* attribtues, split on the dots and put back together
-        # longer term we might want a generic approach, but
-        # right now it's just net. attributes
-        netattrparts = attrname.split('.')
-        attrname = netattrparts[0] + '.' + netattrparts[-1]
+    attrname = _get_valid_attrname(attrname)
     if attrname not in allattributes.node:
         # Otherwise, it must be in the allattributes key list
         return True
@@ -268,6 +258,17 @@ def attribute_is_invalid(attrname, attrval):
             return True
     return False
 
+
+def _get_valid_attrname(attrname):
+    if attrname.startswith('net.'):
+        # For net.* attribtues, split on the dots and put back together
+        # longer term we might want a generic approach, but
+        # right now it's just net. attributes
+        netattrparts = attrname.split('.')
+        attrname = netattrparts[0] + '.' + netattrparts[-1]
+    return attrname
+
+
 def crypt_value(value,
                 key=None,
                 integritykey=None):
@@ -419,10 +420,10 @@ class _ExpressionFormat(string.Formatter):
             right = node.attr
             key = left + '.' + right
             val = self._expand_attribute(key)
-            return val['value'] if 'value' in val else ""
+            return val['value'] if val and 'value' in val else ""
         elif isinstance(node, ast.Name):
             var = node.id
-            if var == 'nodename':
+            if var in ('node', 'nodename'):
                 return self._nodename
             if var in _attraliases:
                 val = self._expand_attribute(_attraliases[var])
@@ -436,7 +437,11 @@ class _ExpressionFormat(string.Formatter):
             else:
                 if var in self._nodeobj:
                     val = self._expand_attribute(var)
-                    return val['value'] if 'value' in val else ""
+                    return val['value'] if val and 'value' in val else ""
+                elif (not var.startswith('custom.') and
+                        _get_valid_attrname(var) not in allattributes.node):
+                    raise ValueError(
+                        '{0} is not a valid attribute name'.format(var))
         elif isinstance(node, ast.BinOp):
             optype = type(node.op)
             if optype not in self._supported_ops:
diff --git a/confluent_server/confluent/plugins/configuration/attributes.py b/confluent_server/confluent/plugins/configuration/attributes.py
index 06e061c0..2e7ddf98 100644
--- a/confluent_server/confluent/plugins/configuration/attributes.py
+++ b/confluent_server/confluent/plugins/configuration/attributes.py
@@ -177,10 +177,15 @@ def _expand_expression(nodes, configmanager, inputdata):
     if type(expression) is dict:
         expression = expression['expression']
     pernodeexpressions = {}
-    for expanded in configmanager.expand_attrib_expression(nodes, expression):
-        pernodeexpressions[expanded[0]] = expanded[1]
-    for node in util.natural_sort(pernodeexpressions):
-        yield msg.KeyValueData({'value': pernodeexpressions[node]}, node)
+    try:
+        for expanded in configmanager.expand_attrib_expression(nodes,
+                                                               expression):
+            pernodeexpressions[expanded[0]] = expanded[1]
+        for node in util.natural_sort(pernodeexpressions):
+            yield msg.KeyValueData({'value': pernodeexpressions[node]}, node)
+    except ValueError as e:
+        raise exc.InvalidArgumentException(str(e))
+
 
 
 def create(nodes, element, configmanager, inputdata):