#8 Operator whitespace usage problem due to comparison after token.trim()

pending
nobody
None
Bug
Done
2014-08-21
2014-05-14
seifrox
No

Hi,
While writing my own evaluator, I had the following problem:
If you define operators without spaces e.g. "OR" also strings that are part of a literal get tokenized e.g. "type=PORT OR true" becomes ["type=P", "OR", "T ", "OR", " true"], on the other hand using " OR " solves that problem, but later on " OR " gets not recognized as an operator since tokens get trimmed before comparing and unfortunately "OR" does not match " OR ".

Unfortunately this cannot be solved by overwriting something in my class the operators map and the toToken(Token, String) method are private, so for now I fixed it myself by overwriting the operators map via reflection (I don't have the luxury to wait for an update, and also want the library to use as is without modifying it myself)

In my opinion the best and easiest fix would be to change one line in the AbstractEvaluator(Parameters) constructor

58: this.operators.put(ope.getSymbol(), known);

to

this.operators.put(ope.getSymbol().trim(), known);

Best Regards,
seifrox

Discussion

  • Fathzer
    Fathzer
    2014-05-14

    Hello,
    Thanks a lot for this bug report.
    I'm quite busy, but I hope I will have time to study it this week-end.
    Best regards,
    Jean-Marc Astesana

     
  • Fathzer
    Fathzer
    2014-05-18

    Hello,
    After having a closer look to the problem, it seems to me that there's a simple solution: You should override the tokenize method of your evaluator.

    Here is the test case of an evaluator that I suppose very close to your need:
    :::java

    package com.fathzer.soft.javaluator.junit;
    
    import static org.junit.Assert.*;
    
    import java.util.Arrays;
    import java.util.HashMap;
    import java.util.Iterator;
    import java.util.Map;
    
    import org.junit.Test;
    import com.fathzer.soft.javaluator.AbstractEvaluator;
    import com.fathzer.soft.javaluator.Operator;
    import com.fathzer.soft.javaluator.Parameters;
    
    public class TextOperatorsTest {
    
    @Test
    public void test() {
        Map<String,String> variableToValue = new HashMap<String, String>();
        variableToValue.put("type", "PORT");
        MyEvaluator evaluator = new MyEvaluator();
        assertTrue(evaluator.evaluate("type=PORT", variableToValue));
        assertFalse(evaluator.evaluate("type=NORTH", variableToValue));
        assertTrue(evaluator.evaluate("type=PORT AND true", variableToValue));
    }
    
    private static class MyEvaluator extends AbstractEvaluator<Boolean> {
        /** The negate unary operator. */
        public final static Operator NEGATE = new Operator("NOT", 1, Operator.Associativity.RIGHT, 3);
        /** The logical AND operator. */
        private static final Operator AND = new Operator("AND", 2, Operator.Associativity.LEFT, 2);
        /** The logical OR operator. */
        public final static Operator OR = new Operator("OR", 2, Operator.Associativity.LEFT, 1);
    
        private static final Parameters PARAMETERS;
    
        static {
            // Create the evaluator's parameters
            PARAMETERS = new Parameters();
            // Add the supported operators
            PARAMETERS.add(AND);
            PARAMETERS.add(OR);
            PARAMETERS.add(NEGATE);
        }
    
        protected MyEvaluator() {
            super(PARAMETERS);
        }
    
        @Override
        protected Boolean toValue(String literal, Object evaluationContext) {
            int index = literal.indexOf('=');
            if (index>=0) {
                String variable = literal.substring(0, index);
                String value = literal.substring(index+1);
                System.out.println ("variable test detected is "+variable+" equals to "+value);
                return value.equals(((Map<String, String>)evaluationContext).get(variable));
            } else {
                System.out.println ("constant detected "+literal);
                return Boolean.valueOf(literal);
            }
        }
    
        @Override
        protected Boolean evaluate(Operator operator,
                Iterator<Boolean> operands, Object evaluationContext) {
            if (operator == NEGATE) {
                return !operands.next();
            } else if (operator == OR) {
                Boolean o1 = operands.next();
                Boolean o2 = operands.next();
                return o1 || o2;
            } else if (operator == AND) {
                Boolean o1 = operands.next();
                Boolean o2 = operands.next();
                return o1 && o2;
            } else {
                return super.evaluate(operator, operands, evaluationContext);
            }
        }
    
        @Override
        protected Iterator<String> tokenize(String expression) {
            return Arrays.asList(expression.split("\\s")).iterator();
        }
    }
    }
    

    Let me know if I'm wrong and this do not solve your problem.

    Best regards,

    Jean-Marc Astesana

     
  • seifrox
    seifrox
    2014-05-18

    Hi,

    Thank you for the quick response, this solution would work indeed for me, however, I still think my suggested fix would be better, from a generic point of view. For example in case one wants to mix operaters that need to be separated via whitespace from literals and operators that don't need to be separated e.g., if in the example I want additionally to define the operator '=', in which case I may not want the requirement to write whitespace before and after.

    What about my suggested solution? would there be side effects I am not aware of? or do you just not like it?

    Best regards,
    seifrox

     
  • Fathzer
    Fathzer
    2014-05-19

    Hello,

    To be perfectly honest, I'm not found of the suggested solution, sorry.

    First I should agree that the current implementation of Javaluator is far from perfect :-(
    The tokenizer is instantiated, even if the tokenize method is overridden and never uses it. And the trim call in the beginning of the evaluate is just ... ugly (it proves the default tokenizer fails to divide the expression into tokens !). I need to rewrite some code there ...
    Second, for sure, your solution would probably makes the job on your problem.

    But ...

    Dividing the expression into the right list of tokens is the responsibility of the tokenize method. It was exposed to be overridden in order to accomplish this goal in some particular situations like yours. Expose a private method to solve a problem the tokenize method should solve would lead to a "strange" design (from my point of view).

    I also think of others problems:
    - What about NOT operator. How to ignore NOT in "type=NOTHING", but take it in "(NOT true)" or "AND NOT true" (if space is in the AND token, it can't be in the NOT token, unless there's two spaces).
    - What about other "space like" characters (non-breaking space, tab) ? You will have to define OPERATORS with all combination of "space like" characters.

    I think a good way in your case would be:
    1°) I deliver a new version with Tokenizer constructor made public, so you will be able to reuse it.
    2°) You override the tokenize method, and perform a two pass parsing.
    - Divide the expression in 'space like' separated "super-tokens" (as in my suggestion).
    - Divide every "super-tokens" in tokens using a com.fathzer.soft.javaluator.Tokenizer built with all non alphabetic operators.
    - return these tokens.

    I'm still very busy, but I will have more time next week - I'll have a week's holiday :-)

    Best regards,

    Jean-Marc Astesana

    PS: Sorry, I don't speak english very well. I hope my post is understandable ...

     
    Last edit: Fathzer 2014-05-19
  • Fathzer
    Fathzer
    2014-05-27

    Hello,

    I've released version 3.0.0.
    In this version, the evaluator does not trim anymore the tokens returned by the AbstractEvaluator.tokenize method.

    In addition, you can reuse the Tokenizer class if you need (the constructor is now public).

    Best regards,

    Jean-Marc Astesana

     


Anonymous


Cancel   Add attachments