Repository: citrusframework/citrus
Issue: BooleanExpressionParser parses expression groups containing boolean strings inconsistently

**Citrus Version**
2.7.8

**Expected behavior**
As a user I would expect Citrus to consistently parse valid boolean expression like `(i = 5) and (${conditional} = true)` in e.g. iteration containers.

**Actual behavior**
Expression groups containing strings, like `(${conditional} = true)`, are handled inconsistently by `BooleanExpressionParser`.  Due to its implementation, these groups require additional spaces between expression and parentheses, otherwise parsing will fail.

`com.consol.citrus.exceptions.CitrusRuntimeException: Unknown operator 'false)'`

Source: [stackoverflow](https://stackoverflow.com/questions/53928625/cannot-use-test-on-variable-to-exit-in-citrus-repeat-until-statement)

**Test case sample**
> Please, share the test case (as small as possible) which shows the issue

```
public class ExpressionParserTest extends TestNGCitrusTestRunner {

    /**
     * Test fails b/c '(${conditional} = true)' is not parsed correctly
     * @param runner Citrus TestRunner
     */
    @Test
    @Parameters("runner")
    @CitrusTest
    public void failingTest(@Optional @CitrusResource TestRunner runner) {
        runner.createVariable("conditional", String.valueOf(true));
        runner.repeat().until("(i = 5) and (${conditional} = true)").index("i").actions(
                runner.sleep(1000),
                new AbstractTestAction() {
                    @Override
                    public void doExecute(TestContext testContext) {
                        System.out.println(testContext.getVariable("i"));
                    }
                }
        );
    }

    /**
     * Test succeeds due to spaces in expression
     * @param runner Citrus TestRunner
     */
    @Test
    @Parameters("runner")
    @CitrusTest
    public void successfulTest(@Optional @CitrusResource TestRunner runner) {
        runner.createVariable("conditional", String.valueOf(true));
        runner.repeat().until("(j = 5) and ( ${conditional} = true )").index("j").actions(
                runner.sleep(1000),
                new AbstractTestAction() {
                    @Override
                    public void doExecute(TestContext testContext) {
                        System.out.println(testContext.getVariable("j"));
                    }
                }
        );
    }
}
```

Changed files:
modules/citrus-core/src/test/java/com/consol/citrus/util/BooleanExpressionParserTest.java
modules/citrus-core/src/main/java/com/consol/citrus/util/BooleanExpressionParser.java

diff --git a/modules/citrus-core/src/main/java/com/consol/citrus/util/BooleanExpressionParser.java b/modules/citrus-core/src/main/java/com/consol/citrus/util/BooleanExpressionParser.java
index 0b89a1796..8ea578945 100644
--- a/modules/citrus-core/src/main/java/com/consol/citrus/util/BooleanExpressionParser.java
+++ b/modules/citrus-core/src/main/java/com/consol/citrus/util/BooleanExpressionParser.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2006-2010 the original author or authors.
+ * Copyright 2006-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,164 +19,319 @@ package com.consol.citrus.util;
 import com.consol.citrus.exceptions.CitrusRuntimeException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.springframework.util.CollectionUtils;
 
-import java.util.*;
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Deque;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import static java.lang.Boolean.FALSE;
+import static java.lang.Boolean.TRUE;
 
 /**
  * Parses boolean expression strings and evaluates to boolean result.
- * 
- * @author Christoph Deppisch
  */
-@SuppressWarnings("unchecked")
 public final class BooleanExpressionParser {
-    
-    /** List of known operators */
-    private static final List<String> OPERATORS = new ArrayList<String>(
-            CollectionUtils.arrayToList(new String[]{"(", "=", "and", "or", "lt", "lt=", "gt", "gt=", ")"}));
 
-    /** List of known boolean values */
-    private static final List<String> BOOLEAN_VALUES = new ArrayList<String>(
-            CollectionUtils.arrayToList(new String[]{"true", "false"}));
-    
+    /**
+     * List of known non-boolean operators
+     */
+    private static final List<String> OPERATORS = new ArrayList<>(Arrays.asList("lt", "lt=", "gt", "gt="));
+
+    /**
+     * List of known boolean operators
+     */
+    private static final List<String> BOOLEAN_OPERATORS = new ArrayList<>(Arrays.asList("=", "and", "or"));
+
+    /**
+     * List of known boolean values
+     */
+    private static final List<String> BOOLEAN_VALUES = new ArrayList<>(
+            Arrays.asList(TRUE.toString(), FALSE.toString()));
+
+    /**
+     * SeparatorToken is an explicit type to identify different kinds of separators.
+     */
+    private enum SeparatorToken {
+        SPACE(' '),
+        OPEN_PARENTHESIS('('),
+        CLOSE_PARENTHESIS(')');
+
+        private final Character value;
+
+        SeparatorToken(final Character value) {
+            this.value = value;
+        }
+
+        @Override
+        public String toString(){
+            return value.toString();
+        }
+    }
+
     /**
      * Logger
      */
-    private static Logger log = LoggerFactory.getLogger(BooleanExpressionParser.class);
+    private static final Logger log = LoggerFactory.getLogger(BooleanExpressionParser.class);
 
     /**
      * Prevent instantiation.
      */
     private BooleanExpressionParser() {
     }
-    
+
     /**
      * Perform evaluation of boolean expression string.
-     * @param expression
-     * @throws CitrusRuntimeException
-     * @return
+     *
+     * @param expression The expression to evaluate
+     * @return boolean result
+     * @throws CitrusRuntimeException When unable to parse expression
      */
-    public static boolean evaluate(String expression) {
-        Stack<String> operators = new Stack<String>();
-        Stack<String> values = new Stack<String>();
-        boolean result = true;
+    public static boolean evaluate(final String expression) {
+        final Deque<String> operators = new ArrayDeque<>();
+        final Deque<String> values = new ArrayDeque<>();
+        final boolean result;
 
-        char actChar;
+        char currentCharacter;
+        int currentCharacterIndex = 0;
 
         try {
-            for (int i = 0; i < expression.length(); i++) {
-                actChar = expression.charAt(i);
-    
-                if (actChar == '(') {
-                    operators.push("(");
-                } else if (actChar == ' ') {
-                    continue; //ignore
-                } else if (actChar == ')') {
-                    String operator = operators.pop();
-                    while (!(operator).equals("(")) {
-                        values.push(getBooleanResultAsString(operator, values.pop(), values.pop()));
-                        operator = operators.pop();
-                    }
-                } else if (!Character.isDigit(actChar)) {
-                    StringBuffer operatorBuffer = new StringBuffer();
-    
-                    int m = i;
-                    do {
-                        operatorBuffer.append(actChar);
-                        m++;
-                        
-                        if (m < expression.length()) {
-                            actChar = expression.charAt(m);
-                        }
-                    } while (m < expression.length() && !Character.isDigit(actChar) && !(actChar == ' ') && !(actChar == '('));
-    
-                    i = m - 1;
-
-                    if (BOOLEAN_VALUES.contains(operatorBuffer.toString())) {
-                        values.push(Boolean.valueOf(operatorBuffer.toString()) ? "1" : "0");
+            while (currentCharacterIndex < expression.length()) {
+                currentCharacter = expression.charAt(currentCharacterIndex);
+
+                if (SeparatorToken.OPEN_PARENTHESIS.value == currentCharacter) {
+                    operators.push(SeparatorToken.OPEN_PARENTHESIS.toString());
+                    currentCharacterIndex += moveCursor(SeparatorToken.OPEN_PARENTHESIS.toString());
+                } else if (SeparatorToken.SPACE.value == currentCharacter) {
+                    currentCharacterIndex += moveCursor(SeparatorToken.SPACE.toString());
+                } else if (SeparatorToken.CLOSE_PARENTHESIS.value == currentCharacter) {
+                    evaluateSubexpression(operators, values);
+                    currentCharacterIndex += moveCursor(SeparatorToken.CLOSE_PARENTHESIS.toString());
+                } else if (!Character.isDigit(currentCharacter)) {
+                    final String parsedNonDigit = parseNonDigits(expression, currentCharacterIndex);
+                    if (isBoolean(parsedNonDigit)) {
+                        values.push(replaceBooleanStringByIntegerRepresentation(parsedNonDigit));
                     } else {
-                        operators.push(validateOperator(operatorBuffer.toString()));
+                        operators.push(validateOperator(parsedNonDigit));
                     }
-                } else if (Character.isDigit(actChar)) {
-                    StringBuffer digitBuffer = new StringBuffer();
-    
-                    int m = i;
-                    do {
-                        digitBuffer.append(actChar);
-                        m++;
-                        
-                        if (m < expression.length()) {
-                            actChar = expression.charAt(m);
-                        }
-                    } while (m < expression.length() && Character.isDigit(actChar));
-    
-                    i = m - 1;
-    
-                    values.push(digitBuffer.toString());
+                    currentCharacterIndex += moveCursor(parsedNonDigit);
+                } else if (Character.isDigit(currentCharacter)) {
+                    final String parsedDigits = parseDigits(expression, currentCharacterIndex);
+                    values.push(parsedDigits);
+                    currentCharacterIndex += moveCursor(parsedDigits);
                 }
             }
-    
-            while (!operators.isEmpty()) {
-                values.push(getBooleanResultAsString(operators.pop(), values.pop(), values.pop()));
-            }
-    
-            String value = values.pop();
 
-            if (value.equals("0")) {
-                value = "false";
-            } else if (value.equals("1")) {
-                value = "true";
-            }
+            result = Boolean.valueOf(evaluateExpressionStack(operators, values));
 
-            result = Boolean.valueOf(value).booleanValue();
-    
             if (log.isDebugEnabled()) {
-                log.debug("Boolean expression " + expression + " evaluates to " + result);
+                log.debug("Boolean expression {} evaluates to {}", expression, result);
             }
-        } catch(EmptyStackException e) {
+        } catch (final NoSuchElementException e) {
             throw new CitrusRuntimeException("Unable to parse boolean expression '" + expression + "'. Maybe expression is incomplete!", e);
         }
 
         return result;
     }
-    
+
+    /**
+     * This method takes stacks of operators and values and evaluates possible expressions
+     * This is done by popping one operator and two values, applying the operator to the values and pushing the result back onto the value stack
+     *
+     * @param operators Operators to apply
+     * @param values    Values
+     * @return The final result popped of the values stack
+     */
+    private static String evaluateExpressionStack(final Deque<String> operators, final Deque<String> values) {
+        while (!operators.isEmpty()) {
+            values.push(getBooleanResultAsString(operators.pop(), values.pop(), values.pop()));
+        }
+        return replaceIntegerStringByBooleanRepresentation(values.pop());
+    }
+
+    /**
+     * Evaluates a sub expression within a pair of parentheses and pushes its result onto the stack of values
+     *
+     * @param operators Stack of operators
+     * @param values    Stack of values
+     */
+    private static void evaluateSubexpression(final Deque<String> operators, final Deque<String> values) {
+        String operator = operators.pop();
+        while (!(operator).equals(SeparatorToken.OPEN_PARENTHESIS.toString())) {
+            values.push(getBooleanResultAsString(operator,
+                    values.pop(),
+                    values.pop()));
+            operator = operators.pop();
+        }
+    }
+
+    /**
+     * This method reads digit characters from a given string, starting at a given index.
+     * It will read till the end of the string or up until it encounters a non-digit character
+     *
+     * @param expression The string to parse
+     * @param startIndex The start index from where to parse
+     * @return The parsed substring
+     */
+    private static String parseDigits(final String expression, final int startIndex) {
+        final StringBuilder digitBuffer = new StringBuilder();
+
+        char currentCharacter = expression.charAt(startIndex);
+        int subExpressionIndex = startIndex;
+
+        do {
+            digitBuffer.append(currentCharacter);
+            ++subExpressionIndex;
+
+            if (subExpressionIndex < expression.length()) {
+                currentCharacter = expression.charAt(subExpressionIndex);
+            }
+        } while (subExpressionIndex < expression.length() && Character.isDigit(currentCharacter));
+
+        return digitBuffer.toString();
+    }
+
+    /**
+     * This method reads non-digit characters from a given string, starting at a given index.
+     * It will read till the end of the string or up until it encounters
+     * <p>
+     * - a digit
+     * - a separator token
+     *
+     * @param expression The string to parse
+     * @param startIndex The start index from where to parse
+     * @return The parsed substring
+     */
+    private static String parseNonDigits(final String expression, final int startIndex) {
+        final StringBuilder operatorBuffer = new StringBuilder();
+
+        char currentCharacter = expression.charAt(startIndex);
+        int subExpressionIndex = startIndex;
+        do {
+            operatorBuffer.append(currentCharacter);
+            subExpressionIndex++;
+
+            if (subExpressionIndex < expression.length()) {
+                currentCharacter = expression.charAt(subExpressionIndex);
+            }
+        } while (subExpressionIndex < expression.length() && !Character.isDigit(currentCharacter) && !isSeparatorToken(currentCharacter));
+
+        return operatorBuffer.toString();
+    }
+
+    /**
+     * Checks whether a string can be interpreted as a boolean value.
+     *
+     * @param possibleBoolean The possible boolean value as string
+     * @return Either true or false
+     */
+    private static Boolean isBoolean(final String possibleBoolean) {
+        return BOOLEAN_VALUES.contains(possibleBoolean);
+    }
+
+    /**
+     * Checks whether a String is a Boolean value and replaces it with its Integer representation
+     * "true" -> "1"
+     * "false" -> "0"
+     *
+     * @param possibleBooleanString "true" or "false"
+     * @return "1" or "0"
+     */
+    private static String replaceBooleanStringByIntegerRepresentation(final String possibleBooleanString) {
+        if (possibleBooleanString.equals(TRUE.toString())) {
+            return "1";
+        } else if (possibleBooleanString.equals(FALSE.toString())) {
+            return "0";
+        }
+        return possibleBooleanString;
+    }
+
+    /**
+     * Counterpart of {@link #replaceBooleanStringByIntegerRepresentation}
+     * Checks whether a String is the Integer representation of a Boolean value and replaces it with its Boolean representation
+     * "1" -> "true"
+     * "0" -> "false"
+     * otherwise -> value
+     *
+     * @param value "1", "0" or other string
+     * @return "true", "false" or the input value
+     */
+    private static String replaceIntegerStringByBooleanRepresentation(final String value) {
+        if (value.equals("0")) {
+            return FALSE.toString();
+        } else if (value.equals("1")) {
+            return TRUE.toString();
+        }
+        return value;
+    }
+
+    /**
+     * Checks whether a given character is a known separator token or no
+     *
+     * @param possibleSeparatorChar The character to check
+     * @return True in case its a separator, false otherwise
+     */
+    private static boolean isSeparatorToken(final char possibleSeparatorChar) {
+        for (final SeparatorToken token : SeparatorToken.values()) {
+            if (token.value == possibleSeparatorChar) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     /**
      * Check if operator is known to this class.
+     *
      * @param operator to validate
      * @return the operator itself.
-     * @throws CitrusRuntimeException
+     * @throws CitrusRuntimeException When encountering an unknown operator
      */
-    private static String validateOperator(String operator) {
-        if (!OPERATORS.contains(operator)) {
+    private static String validateOperator(final String operator) {
+        if (!OPERATORS.contains(operator) && !BOOLEAN_OPERATORS.contains(operator)) {
             throw new CitrusRuntimeException("Unknown operator '" + operator + "'");
         }
         return operator;
     }
 
+    /**
+     * Returns the amount of characters to move the cursor after parsing a token
+     *
+     * @param lastToken Last parsed token
+     * @return Amount of characters to move forward
+     */
+    private static int moveCursor(final String lastToken) {
+        return lastToken.length();
+    }
+
     /**
      * Evaluates a boolean expression to a String representation (true/false).
-     * @param operator
-     * @param value1
-     * @param value2
+     *
+     * @param operator     The operator to apply on operands
+     * @param rightOperand The right hand side of the expression
+     * @param leftOperand  The left hand side of the expression
      * @return true/false as String
      */
-    private static String getBooleanResultAsString(String operator, String value1, String value2) {
-        if (operator.equals("lt")) {
-            return Boolean.valueOf(Integer.valueOf(value2).intValue() < Integer.valueOf(value1).intValue()).toString();
-        } else if (operator.equals("lt=")) {
-            return Boolean.valueOf(Integer.valueOf(value2).intValue() <= Integer.valueOf(value1).intValue()).toString();
-        } else if (operator.equals("gt")) {
-            return Boolean.valueOf(Integer.valueOf(value2).intValue() > Integer.valueOf(value1).intValue()).toString();
-        } else if (operator.equals("gt=")) {
-            return Boolean.valueOf(Integer.valueOf(value2).intValue() >= Integer.valueOf(value1).intValue()).toString();
-        } else if (operator.equals("=")) {
-            return Boolean.valueOf(Integer.valueOf(value2).intValue() == Integer.valueOf(value1).intValue()).toString();
-        } else if (operator.equals("and")) {
-            return Boolean.valueOf(Boolean.valueOf(value2).booleanValue() && Boolean.valueOf(value1).booleanValue()).toString();
-        } else if (operator.equals("or")) {
-            return Boolean.valueOf(Boolean.valueOf(value2).booleanValue() || Boolean.valueOf(value1).booleanValue()).toString();
-        } else {
-            throw new CitrusRuntimeException("Unknown operator '" + operator + "'");
+    private static String getBooleanResultAsString(final String operator, final String rightOperand, final String leftOperand) {
+        switch (operator) {
+            case "lt":
+                return Boolean.toString(Integer.valueOf(leftOperand) < Integer.valueOf(rightOperand));
+            case "lt=":
+                return Boolean.toString(Integer.valueOf(leftOperand) <= Integer.valueOf(rightOperand));
+            case "gt":
+                return Boolean.toString(Integer.valueOf(leftOperand) > Integer.valueOf(rightOperand));
+            case "gt=":
+                return Boolean.toString(Integer.valueOf(leftOperand) >= Integer.valueOf(rightOperand));
+            case "=":
+                return Boolean.toString(Integer.parseInt(leftOperand) == Integer.parseInt(rightOperand));
+            case "and":
+                return Boolean.toString(Boolean.valueOf(leftOperand) && Boolean.valueOf(rightOperand));
+            case "or":
+                return Boolean.toString(Boolean.valueOf(leftOperand) || Boolean.valueOf(rightOperand));
+            default:
+                throw new CitrusRuntimeException("Unknown operator '" + operator + "'");
         }
     }
 }
diff --git a/modules/citrus-core/src/test/java/com/consol/citrus/util/BooleanExpressionParserTest.java b/modules/citrus-core/src/test/java/com/consol/citrus/util/BooleanExpressionParserTest.java
index 40aff4ac5..28d319b31 100644
--- a/modules/citrus-core/src/test/java/com/consol/citrus/util/BooleanExpressionParserTest.java
+++ b/modules/citrus-core/src/test/java/com/consol/citrus/util/BooleanExpressionParserTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2006-2010 the original author or authors.
+ * Copyright 2006-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -48,6 +48,10 @@ public class BooleanExpressionParserTest {
         Assert.assertFalse(BooleanExpressionParser.evaluate("(1 lt 1) and (2 gt 2)"));
         Assert.assertFalse(BooleanExpressionParser.evaluate("(1 gt 2) or (2 = 3)"));
         Assert.assertFalse(BooleanExpressionParser.evaluate("((1 = 5) and (2 = 6)) or (2 lt 1)"));
+    }
+
+    @Test
+    public void testExpressionParserWithStringValues() {
         Assert.assertTrue(BooleanExpressionParser.evaluate("true"));
         Assert.assertTrue(BooleanExpressionParser.evaluate("true = true"));
         Assert.assertTrue(BooleanExpressionParser.evaluate("false = false"));
@@ -56,13 +60,21 @@ public class BooleanExpressionParserTest {
         Assert.assertFalse(BooleanExpressionParser.evaluate("false = true"));
         Assert.assertTrue(BooleanExpressionParser.evaluate("( false = false ) and ( true = true )"));
         Assert.assertFalse(BooleanExpressionParser.evaluate("( false = false ) and ( true = false )"));
+        Assert.assertTrue(BooleanExpressionParser.evaluate("(false = false) and (true = true)"));
+        Assert.assertFalse(BooleanExpressionParser.evaluate("(false = false) and (true = false)"));
+        Assert.assertTrue(BooleanExpressionParser.evaluate("(   false = false) and (true = true    )"));
+        Assert.assertFalse(BooleanExpressionParser.evaluate("(false = false    ) and     (    true = false)"));
+        Assert.assertTrue(BooleanExpressionParser.evaluate("( true = false ) or ( false = false )"));
+        Assert.assertTrue(BooleanExpressionParser.evaluate("(false = false) or (true = true)"));
+        Assert.assertTrue(BooleanExpressionParser.evaluate("(false = false) or (true = false)"));
+        Assert.assertTrue(BooleanExpressionParser.evaluate("(false = false    ) or (    true = false)"));
     }
     
     @Test
     public void testExpressionParserWithUnknownOperator() {
         try {
             BooleanExpressionParser.evaluate("wahr");
-        } catch(CitrusRuntimeException e) {
+        } catch(final CitrusRuntimeException e) {
             Assert.assertEquals(e.getLocalizedMessage(), "Unknown operator 'wahr'");
             return;
         }
@@ -74,7 +86,7 @@ public class BooleanExpressionParserTest {
     public void testExpressionParserWithBrokenExpression() {
         try {
             BooleanExpressionParser.evaluate("1 = ");
-        } catch(CitrusRuntimeException e) {
+        } catch(final CitrusRuntimeException e) {
             Assert.assertEquals(e.getLocalizedMessage(), "Unable to parse boolean expression '1 = '. Maybe expression is incomplete!");
             return;
         }