From 7ca2bab65d3f7733e20d8aa6c6a0b830279f3e33 Mon Sep 17 00:00:00 2001
From: Madhu Kanoor <mkanoor@redhat.com>
Date: Tue, 7 Feb 2023 04:09:33 -0500
Subject: [PATCH] Improve condition parsing errors (#350)

https://issues.redhat.com/browse/AAP-8397
Added new exceptions for handling different kinds of condition parsing
errors
---
 ansible_rulebook/condition_parser.py |  8 ++++----
 ansible_rulebook/exception.py        | 10 ++++++++++
 ansible_rulebook/json_generator.py   |  8 +++++++-
 tests/test_ast.py                    | 24 ++++++++++++++++++++++++
 4 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/ansible_rulebook/condition_parser.py b/ansible_rulebook/condition_parser.py
index 0f67fa48..083be6e5 100644
--- a/ansible_rulebook/condition_parser.py
+++ b/ansible_rulebook/condition_parser.py
@@ -13,7 +13,6 @@
 #  limitations under the License.
 
 import logging
-import sys
 
 from pyparsing import (
     Combine,
@@ -33,6 +32,7 @@
 )
 
 from ansible_rulebook.exception import (
+    ConditionParsingException,
     SelectattrOperatorException,
     SelectOperatorException,
 )
@@ -306,6 +306,6 @@ def parse_condition(condition_string: str) -> Condition:
     try:
         return condition.parseString(condition_string, parse_all=True)[0]
     except ParseException as pe:
-        print(pe.explain(depth=0), file=sys.stderr)
-        logger.error(pe.explain(depth=0))
-        raise
+        msg = f"Error parsing: {condition_string}. {pe}"
+        logger.debug(pe.explain(depth=0))
+        raise ConditionParsingException(msg)
diff --git a/ansible_rulebook/exception.py b/ansible_rulebook/exception.py
index fc640b7a..5e017233 100644
--- a/ansible_rulebook/exception.py
+++ b/ansible_rulebook/exception.py
@@ -48,6 +48,16 @@ class SelectattrOperatorException(Exception):
     pass
 
 
+class InvalidIdentifierException(Exception):
+
+    pass
+
+
 class SelectOperatorException(Exception):
 
     pass
+
+
+class ConditionParsingException(Exception):
+
+    pass
diff --git a/ansible_rulebook/json_generator.py b/ansible_rulebook/json_generator.py
index ced17735..cf1db8e8 100644
--- a/ansible_rulebook/json_generator.py
+++ b/ansible_rulebook/json_generator.py
@@ -34,6 +34,7 @@
 )
 from ansible_rulebook.exception import (
     InvalidAssignmentException,
+    InvalidIdentifierException,
     VarsKeyMissingException,
 )
 from ansible_rulebook.rule_types import (
@@ -99,7 +100,12 @@ def visit_condition(parsed_condition: ConditionTypes, variables: Dict):
                     f"vars does not contain key: {key}"
                 )
         else:
-            raise Exception(f"Unhandled identifier {parsed_condition.value}")
+            msg = (
+                f"Invalid identifier : {parsed_condition.value} "
+                + "Should start with event., events.,fact., facts. or vars."
+            )
+            raise InvalidIdentifierException(msg)
+
     elif isinstance(parsed_condition, String):
         return {
             "String": substitute_variables(parsed_condition.value, variables)
diff --git a/tests/test_ast.py b/tests/test_ast.py
index dd826d0d..1d7a6198 100644
--- a/tests/test_ast.py
+++ b/tests/test_ast.py
@@ -19,7 +19,9 @@
 
 from ansible_rulebook.condition_parser import parse_condition
 from ansible_rulebook.exception import (
+    ConditionParsingException,
     InvalidAssignmentException,
+    InvalidIdentifierException,
     SelectattrOperatorException,
     SelectOperatorException,
 )
@@ -500,3 +502,25 @@ def test_invalid_nested_assignment_operator():
             parse_condition("events.first.abc.xyz << fact.range.i == 'Hello'"),
             {},
         )
+
+
+def test_invalid_identifier():
+    with pytest.raises(InvalidIdentifierException):
+        visit_condition(
+            parse_condition("event is defined"),
+            {},
+        )
+
+
+@pytest.mark.parametrize(
+    "invalid_condition",
+    [
+        "event. is defined",
+    ],
+)
+def test_invalid_conditions(invalid_condition):
+    with pytest.raises(ConditionParsingException):
+        visit_condition(
+            parse_condition(invalid_condition),
+            {},
+        )