JEXL-359: refactored OperatorController, tidy code linked to strictne…

…ss, clean-up test;
apache · Feb 14, 2022 · 73bb932 · 73bb932
1 parent 99054fb
commit 73bb932
Show file tree

Hide file tree

Showing 10 changed files with 155 additions and 132 deletions.
diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt
@@ -38,6 +38,7 @@ allow fine-tuning the scripting integration into any project.
 
 New Features in 3.3:
 ====================
+* JEXL-359:     Allow per-operator arithmetic handling of null arguments
 * JEXL-357:     Configure accessible packages/classes/methods/fields
 
 Bugs Fixed in 3.3:

diff --git a/src/main/java/org/apache/commons/jexl3/JexlArithmetic.java b/src/main/java/org/apache/commons/jexl3/JexlArithmetic.java
@@ -372,6 +372,22 @@ public boolean isStrict() {
         return this.strict;
     }
 
+    /**
+     * Checks whether this arithmetic considers a given operator as strict or null-safe.
+     * <p>When an operator is strict, it does <em>not</em> accept null arguments when the arithmetic is strict.
+     * If null-safe (ie not-strict), the operator does accept null arguments even if the arithmetic itself is strict.</p>
+     * <p>The default implementation considers equal/not-equal operators as null-safe so one can check for null as in
+     * <code>if (myvar == null) {...}</code>. Note that this operator is used for equal and not-equal syntax.</p>
+     * <p>An arithmetic refining its strict behavior handling for more operators must declare which by overriding
+     * this method.</p>
+     * @param operator the operator to check for null-argument(s) handling
+     * @return true if operator considers null arguments as errors, false if operator has appropriate semantics
+     * for null argument(s)
+     */
+    public boolean isStrict(JexlOperator operator) {
+        return operator == JexlOperator.EQ? false : isStrict();
+    }
+
     /**
      * The MathContext instance used for +,-,/,*,% operations on big decimals.
      *
@@ -746,7 +762,7 @@ public Object add(final Object left, final Object right) {
                 }
             }
         }
-        return toString(left).concat(toString(right));
+        return (left == null? "" : toString(left)).concat(right == null ? "" : toString(right));
     }
 
     /**

diff --git a/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java b/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java
@@ -264,7 +264,7 @@ protected Object visit(final ASTAddNode node, final Object data) {
             final Object result = operators.tryOverload(node, JexlOperator.ADD, left, right);
             return result != JexlEngine.TRY_FAILED ? result : arithmetic.add(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, "+ error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "+ error", xrt);
         }
     }
 
@@ -276,7 +276,7 @@ protected Object visit(final ASTSubNode node, final Object data) {
             final Object result = operators.tryOverload(node, JexlOperator.SUBTRACT, left, right);
             return result != JexlEngine.TRY_FAILED ? result : arithmetic.subtract(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, "- error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "- error", xrt);
         }
     }
 
@@ -288,8 +288,7 @@ protected Object visit(final ASTMulNode node, final Object data) {
             final Object result = operators.tryOverload(node, JexlOperator.MULTIPLY, left, right);
             return result != JexlEngine.TRY_FAILED ? result : arithmetic.multiply(left, right);
         } catch (final ArithmeticException xrt) {
-            final JexlNode xnode = findNullOperand(xrt, node, left, right);
-            throw new JexlException(xnode, "* error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "* error", xrt);
         }
     }
 
@@ -304,8 +303,7 @@ protected Object visit(final ASTDivNode node, final Object data) {
             if (!arithmetic.isStrict()) {
                 return 0.0d;
             }
-            final JexlNode xnode = findNullOperand(xrt, node, left, right);
-            throw new JexlException(xnode, "/ error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "/ error", xrt);
         }
     }
 
@@ -320,8 +318,7 @@ protected Object visit(final ASTModNode node, final Object data) {
             if (!arithmetic.isStrict()) {
                 return 0.0d;
             }
-            final JexlNode xnode = findNullOperand(xrt, node, left, right);
-            throw new JexlException(xnode, "% error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "% error", xrt);
         }
     }
 
@@ -333,19 +330,22 @@ protected Object visit(final ASTBitwiseAndNode node, final Object data) {
             final Object result = operators.tryOverload(node, JexlOperator.AND, left, right);
             return result != JexlEngine.TRY_FAILED ? result : arithmetic.and(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, "& error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "& error", xrt);
         }
     }
 
     @Override
     protected Object visit(final ASTBitwiseOrNode node, final Object data) {
         final Object left = node.jjtGetChild(0).jjtAccept(this, data);
         final Object right = node.jjtGetChild(1).jjtAccept(this, data);
+        if (arithmetic.isStrict(JexlOperator.OR) && left == null || right == null) {
+            // boum
+        }
         try {
             final Object result = operators.tryOverload(node, JexlOperator.OR, left, right);
             return result != JexlEngine.TRY_FAILED ? result : arithmetic.or(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, "| error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "| error", xrt);
         }
     }
 
@@ -357,7 +357,7 @@ protected Object visit(final ASTBitwiseXorNode node, final Object data) {
             final Object result = operators.tryOverload(node, JexlOperator.XOR, left, right);
             return result != JexlEngine.TRY_FAILED ? result : arithmetic.xor(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, "^ error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "^ error", xrt);
         }
     }
 
@@ -369,7 +369,7 @@ protected Object visit(final ASTEQNode node, final Object data) {
             final Object result = operators.tryOverload(node, JexlOperator.EQ, left, right);
             return result != JexlEngine.TRY_FAILED ? result : arithmetic.equals(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, "== error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "== error", xrt);
         }
     }
 
@@ -383,8 +383,7 @@ protected Object visit(final ASTNENode node, final Object data) {
                    ? !arithmetic.toBoolean(result)
                    : !arithmetic.equals(left, right);
         } catch (final ArithmeticException xrt) {
-            final JexlNode xnode = findNullOperand(xrt, node, left, right);
-            throw new JexlException(xnode, "!= error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "!= error", xrt);
         }
     }
 
@@ -398,7 +397,7 @@ protected Object visit(final ASTGENode node, final Object data) {
                    ? result
                    : arithmetic.greaterThanOrEqual(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, ">= error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), ">= error", xrt);
         }
     }
 
@@ -412,7 +411,7 @@ protected Object visit(final ASTGTNode node, final Object data) {
                    ? result
                    : arithmetic.greaterThan(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, "> error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "> error", xrt);
         }
     }
 
@@ -426,7 +425,7 @@ protected Object visit(final ASTLENode node, final Object data) {
                    ? result
                    : arithmetic.lessThanOrEqual(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, "<= error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "<= error", xrt);
         }
     }
 
@@ -440,7 +439,7 @@ protected Object visit(final ASTLTNode node, final Object data) {
                    ? result
                    : arithmetic.lessThan(left, right);
         } catch (final ArithmeticException xrt) {
-            throw new JexlException(node, "< error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), "< error", xrt);
         }
     }
 
@@ -493,8 +492,7 @@ protected Object visit(final ASTRangeNode node, final Object data) {
         try {
             return arithmetic.createRange(left, right);
         } catch (final ArithmeticException xrt) {
-            final JexlNode xnode = findNullOperand(xrt, node, left, right);
-            throw new JexlException(xnode, ".. error", xrt);
+            throw new JexlException(findNullOperand(node, left, right), ".. error", xrt);
         }
     }
 
@@ -1231,7 +1229,7 @@ protected Object visit(final ASTReference node, final Object data) {
                 final String aname = ant != null ? ant.toString() : "?";
                 final boolean defined = isVariableDefined(frame, block, aname);
                 // defined but null; arg of a strict operator?
-                if (defined && (!arithmetic.isStrict() || !node.jjtGetParent().isStrictOperator())) {
+                if (defined && !node.jjtGetParent().isStrictOperator(arithmetic)) {
                     return null;
                 }
                 return unsolvableVariable(node, aname, !defined);

diff --git a/src/main/java/org/apache/commons/jexl3/internal/InterpreterBase.java b/src/main/java/org/apache/commons/jexl3/internal/InterpreterBase.java
@@ -309,7 +309,7 @@ protected Object getVariable(final Frame frame, final LexicalScope block, final
             // not out of scope with no lexical shade ?
             if (value != Scope.UNDEFINED) {
                 // null argument of an arithmetic operator ?
-                if (value == null && arithmetic.isStrict() && identifier.jjtGetParent().isStrictOperator()) {
+                if (value == null && identifier.jjtGetParent().isStrictOperator(arithmetic)) {
                     return unsolvableVariable(identifier, name, false); // defined but null
                 }
                 return value;
@@ -328,7 +328,7 @@ protected Object getVariable(final Frame frame, final LexicalScope block, final
                 if (!ignore) {
                     return undefinedVariable(identifier, name); // undefined
                 }
-            } else if (arithmetic.isStrict() && identifier.jjtGetParent().isStrictOperator()) {
+            } else if (identifier.jjtGetParent().isStrictOperator(arithmetic)) {
                 return unsolvableVariable(identifier, name, false); // defined but null
             }
         }
@@ -385,22 +385,24 @@ protected boolean isCancellable() {
         return options.isCancellable();
     }
 
+    @Deprecated
+    protected JexlNode findNullOperand(final RuntimeException xrt, final JexlNode node, final Object left, final Object right) {
+        return findNullOperand(node, left, right);
+    }
+
     /**
      * Finds the node causing a NPE for diadic operators.
-     * @param xrt   the RuntimeException
      * @param node  the parent node
      * @param left  the left argument
      * @param right the right argument
      * @return the left, right or parent node
      */
-    protected JexlNode findNullOperand(final RuntimeException xrt, final JexlNode node, final Object left, final Object right) {
-        if (xrt instanceof JexlArithmetic.NullOperand) {
-            if (left == null) {
-                return node.jjtGetChild(0);
-            }
-            if (right == null) {
-                return node.jjtGetChild(1);
-            }
+    protected JexlNode findNullOperand(final JexlNode node, final Object left, final Object right) {
+        if (left == null) {
+            return node.jjtGetChild(0);
+        }
+        if (right == null) {
+            return node.jjtGetChild(1);
         }
         return node;
     }

diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/ClassMap.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/ClassMap.java
@@ -62,7 +62,7 @@ public static Method cacheMiss() {
     /**
      * The cache miss marker method.
      */
-    private static final Method CACHE_MISS = cacheMiss();
+    static final Method CACHE_MISS = cacheMiss();
     /**
      * This is the cache to store and look up the method information.
      * <p>

diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/Permissions.java
@@ -111,7 +111,7 @@ void addNoJexl(String key, NoJexlClass njc) {
      * @param clazz the clazz
      * @return the clazz key
      */
-    private static String classKey(final Class<?> clazz) {
+    static String classKey(final Class<?> clazz) {
         return classKey(clazz, null);
     }
 
@@ -122,7 +122,7 @@ private static String classKey(final Class<?> clazz) {
      * @param strb the buffer to compose the key
      * @return the clazz key
      */
-    private static String classKey(final Class<?> clazz, final StringBuilder strb) {
+    static String classKey(final Class<?> clazz, final StringBuilder strb) {
         StringBuilder keyb = strb;
         Class<?> outer = clazz.getEnclosingClass();
         if (outer != null) {
@@ -175,50 +175,44 @@ boolean deny(Constructor<?> method) {
 
     /** Marker for whole NoJexl class. */
     static final NoJexlClass NOJEXL_CLASS = new NoJexlClass(Collections.emptySet(), Collections.emptySet()) {
-        @Override
-        boolean deny(Field field) {
+        @Override boolean deny(Field field) {
             return true;
         }
 
-        @Override
-        boolean deny(Method method) {
+        @Override boolean deny(Method method) {
             return true;
         }
 
-        @Override
-        boolean deny(Constructor<?> method) {
+        @Override boolean deny(Constructor<?> method) {
             return true;
         }
     };
 
     /** Marker for allowed class. */
     static final NoJexlClass JEXL_CLASS = new NoJexlClass(Collections.emptySet(), Collections.emptySet()) {
-        @Override
-        boolean deny(Field field) {
+        @Override boolean deny(Field field) {
             return false;
         }
 
-        @Override
-        boolean deny(Method method) {
+        @Override  boolean deny(Method method) {
             return false;
         }
 
-        @Override
-        boolean deny(Constructor<?> method) {
+        @Override boolean deny(Constructor<?> method) {
             return false;
         }
     };
 
     /** Marker for @NoJexl package. */
     static final NoJexlPackage NOJEXL_PACKAGE = new NoJexlPackage(Collections.emptyMap()) {
-        NoJexlClass getNoJexl(Class<?> clazz) {
+        @Override NoJexlClass getNoJexl(Class<?> clazz) {
             return NOJEXL_CLASS;
         }
     };
 
     /** Marker for fully allowed package. */
     static final NoJexlPackage JEXL_PACKAGE = new NoJexlPackage(Collections.emptyMap()) {
-        NoJexlClass getNoJexl(Class<?> clazz) {
+        @Override NoJexlClass getNoJexl(Class<?> clazz) {
             return JEXL_CLASS;
         }
     };

diff --git a/src/main/java/org/apache/commons/jexl3/parser/JexlNode.java b/src/main/java/org/apache/commons/jexl3/parser/JexlNode.java
@@ -16,6 +16,7 @@
  */
 package org.apache.commons.jexl3.parser;
 
+import org.apache.commons.jexl3.JexlArithmetic;
 import org.apache.commons.jexl3.JexlException;
 import org.apache.commons.jexl3.JexlInfo;
 import org.apache.commons.jexl3.internal.ScriptVisitor;
@@ -115,24 +116,15 @@ public void clearCache() {
         }
     }
 
-    /**
-     * Checks whether this node is an operator.
-     *
-     * @return true if node is an operator node, false otherwise
-     */
-    public boolean isOperator() {
-        return OperatorController.INSTANCE.control(this, Boolean.TRUE);
-    }
-
     /**
      * Checks whether this node is an operator that accepts a null argument
      * even when arithmetic is in strict mode.
-     * The sole cases are equals and not equals.
+     * The default cases are equals and not equals.
      *
      * @return true if node accepts null arguments, false otherwise
      */
-    public boolean isStrictOperator() {
-        return OperatorController.INSTANCE.control(this, Boolean.FALSE);
+    public boolean isStrictOperator(JexlArithmetic arithmetic) {
+        return OperatorController.INSTANCE.isStrict(arithmetic, this);
     }
 
     /**