Merge pull request BroadleafCommerce#325 from BroadleafCommerce/qa-1508-rule-parsing

Rule-based evaluation does not work with various attribute maps

Author: phillipuniverse

common/src/main/java/org/broadleafcommerce/common/rule/AbstractRuleProcessor.java

@@ -22,7 +22,6 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.broadleafcommerce.common.util.EfficientLRUMap;
-import org.mvel2.CompileException;
 import org.mvel2.MVEL;
 import org.mvel2.ParserContext;
 
@@ -61,26 +60,7 @@ protected ParserContext getParserContext() {
      * @return the result of the expression
      */
     protected Boolean executeExpression(String expression, Map<String, Object> vars) {
-        Serializable exp = (Serializable) expressionCache.get(expression);
-        vars.put("MVEL", MVEL.class);
-
-        if (exp == null) {
-            try {
-                exp = MVEL.compileExpression(expression, getParserContext());
-            } catch (CompileException ce) {
-                LOG.warn("Compile exception processing phrase: " + expression,ce);
-                return Boolean.FALSE;
-            }
-            expressionCache.put(expression, exp);
-        }
-
-        try {
-            return (Boolean) MVEL.executeExpression(exp, vars);
-        } catch (Exception e) {
-            LOG.error(e);
-        }
-
-        return false;
+        return MvelHelper.evaluateRule(expression, vars, expressionCache);
     }
 
     /**

common/src/main/java/org/broadleafcommerce/common/rule/MvelHelper.java

@@ -19,6 +19,7 @@
  */
 package org.broadleafcommerce.common.rule;
 
+import org.apache.commons.collections4.MapUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.broadleafcommerce.common.RequestDTO;
@@ -36,6 +37,7 @@
 import java.text.ParseException;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Map.Entry;
 
 import javax.servlet.http.HttpServletRequest;
 
@@ -62,7 +64,7 @@ public class MvelHelper {
 
     // The following attribute is set in BroadleafProcessURLFilter
     public static final String REQUEST_DTO = "blRequestDTO";    
-
+    
     /**
      * Converts a field to the specified type.    Useful when 
      * @param type
@@ -122,16 +124,38 @@ public static boolean evaluateRule(String rule, Map<String, Object> ruleParamete
      */
     public static boolean evaluateRule(String rule, Map<String, Object> ruleParameters,
             Map<String, Serializable> expressionCache) {
+        return evaluateRule(rule, ruleParameters, expressionCache, null);
+    }
+    
+    /**
+     * @param rule
+     * @param ruleParameters
+     * @param expressionCache
+     * @param additionalContextImports additional imports to give to the {@link ParserContext} besides "MVEL" ({@link MVEL} and
+     * "MvelHelper" ({@link MvelHelper}) since they are automatically added 
+     * @return
+     */
+    public static boolean evaluateRule(String rule, Map<String, Object> ruleParameters,
+        Map<String, Serializable> expressionCache, Map<String, Class<?>> additionalContextImports) {
+        
         // Null or empty is a match
         if (rule == null || "".equals(rule)) {
             return true;
         } else {
             // MVEL expression compiling can be expensive so let's cache the expression
-            Serializable exp = (Serializable) expressionCache.get(rule);
+            Serializable exp = expressionCache.get(rule);
             if (exp == null) {
                 ParserContext context = new ParserContext();
                 context.addImport("MVEL", MVEL.class);
                 context.addImport("MvelHelper", MvelHelper.class);
+                if (MapUtils.isNotEmpty(additionalContextImports)) {
+                    for (Entry<String, Class<?>> entry : additionalContextImports.entrySet()) {
+                        context.addImport(entry.getKey(), entry.getValue());
+                    }
+                }
+                
+                rule = modifyExpression(rule, ruleParameters, context);
+                
                 exp = MVEL.compileExpression(rule, context);
                 expressionCache.put(rule, exp);
             }
@@ -161,6 +185,45 @@ public static boolean evaluateRule(String rule, Map<String, Object> ruleParamete
             }
         }
     }
+    
+    /**
+     * <p>
+     * Provides a hook point to modify the final expression before it's built. By default, this looks for attribute
+     * maps and replaces them such that it does string comparison.
+     * 
+     * <p>
+     * For example, given an expression like getProductAttributes()['somekey'] == 'someval', getProductAttributes()['somekey']
+     * actually returns a ProductAttribute object, not a String, so the comparison is wrong. Instead, we actually want
+     * to do this: getProductAttributes()['somekey'].?value == 'someval'. This function performs that replacement
+     * 
+     * @param rule the rule to replace
+     * @return a modified version of <b>rule</b>
+     * @see {@link #getRuleAttributeMaps()}
+     */
+    protected static String modifyExpression(String rule, Map<String, Object> ruleParameters, ParserContext context) {
+        String modifiedExpression = rule;
+        for (String attributeMap : getRuleAttributeMaps()) {
+            modifiedExpression = modifiedExpression.replaceAll(attributeMap + "\\(\\)\\[(.*)\\](?!\\.\\?value)", attributeMap + "()[$1].?value");
+        }
+        return modifiedExpression;
+    }
+
+    /**
+     * Returns an array of attribute map field names that we need to do replacements for in
+     * {@link #modifyExpression(String, Map, ParserContext)}
+     */
+    protected static String[] getRuleAttributeMaps() {
+        // intentionally left out pricing context getPricingContextAttributes because that's a Map<String, String>
+        return new String[]{ "getProductAttributes",
+            "getCategoryAttributesMap",
+            "getSkuAttributes",
+            "getOrderItemAttributes",
+            "getCustomerAttributes",
+            // Map<String, PageAttribute>
+            "getAdditionalAttributes",
+            // Map<String, AdminUserAttribute>
+            "getAdditionalFields"}; 
+    }
 
     /**
      * When true, LOG.info statement will be suppressed.   Should only be set from within MvelHelperTest.
@@ -185,7 +248,7 @@ public static Map<String, Object> buildMvelParameters() {
         if (brc != null && brc.getRequest() != null) {
            TimeDTO timeDto = new TimeDTO(SystemTime.asCalendar());
             HttpServletRequest request = brc.getRequest();
-            RequestDTO requestDto = (RequestDTO) brc.getRequestDTO();
+            RequestDTO requestDto = brc.getRequestDTO();
             mvelParameters.put("time", timeDto);
             mvelParameters.put("request", requestDto);
 

core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/offer/service/processor/AbstractBaseProcessor.java

@@ -43,10 +43,7 @@
 import org.broadleafcommerce.core.order.service.type.FulfillmentType;
 import org.broadleafcommerce.profile.core.domain.Customer;
 import org.joda.time.LocalDateTime;
-import org.mvel2.MVEL;
-import org.mvel2.ParserContext;
 
-import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Calendar;
 import java.util.Collection;
@@ -235,35 +232,12 @@ protected boolean couldOrderItemMeetOfferRequirement(OfferItemCriteria criteria,
      * @return a Boolean object containing the result of executing the MVEL expression
      */
     public Boolean executeExpression(String expression, Map<String, Object> vars) {
-        try {
-            Serializable exp;
-            synchronized (EXPRESSION_CACHE) {
-                exp = (Serializable) EXPRESSION_CACHE.get(expression);
-                if (exp == null) {
-                    ParserContext context = new ParserContext();
-                    context.addImport("OfferType", OfferType.class);
-                    context.addImport("FulfillmentType", FulfillmentType.class);
-                    context.addImport("MVEL", MVEL.class);
-                    context.addImport("MvelHelper", MvelHelper.class);
-                    //            StringBuffer completeExpression = new StringBuffer(functions.toString());
-                    //            completeExpression.append(" ").append(expression);
-                    exp = MVEL.compileExpression(expression, context);
-
-                    EXPRESSION_CACHE.put(expression, exp);
-                }
-            }
-
-            Object test = MVEL.executeExpression(exp, vars);
-            
-            return (Boolean) test;
-        } catch (Exception e) {
-            //Unable to execute the MVEL expression for some reason
-            //Return false, but notify about the bad expression through logs
-            LOG.warn("Unable to parse and/or execute an mvel expression. Reporting to the logs and returning false " +
-                    "for the match expression:" + expression, e);
-            return false;
+        synchronized (EXPRESSION_CACHE) {
+            Map<String, Class<?>> contextImports = new HashMap<>();
+            contextImports.put("OfferType", OfferType.class);
+            contextImports.put("FulfillmentType", FulfillmentType.class);
+            return MvelHelper.evaluateRule(expression, vars, EXPRESSION_CACHE, contextImports);
         }
-
     }
 
     /**

core/broadleaf-framework/src/main/java/org/broadleafcommerce/core/pricing/service/workflow/ConsolidateFulfillmentFeesActivity.java

@@ -33,10 +33,7 @@
 import org.broadleafcommerce.core.order.service.FulfillmentGroupService;
 import org.broadleafcommerce.core.workflow.BaseActivity;
 import org.broadleafcommerce.core.workflow.ProcessContext;
-import org.mvel2.MVEL;
-import org.mvel2.ParserContext;
 
-import java.io.Serializable;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -106,19 +103,12 @@ protected boolean shouldApplyFeeToFulfillmentGroup(SkuFee fee, FulfillmentGroup
         boolean appliesToFulfillmentGroup = true;
         String feeExpression = fee.getExpression();
 
-        if (!StringUtils.isEmpty(feeExpression)) {
-            Serializable exp = (Serializable) EXPRESSION_CACHE.get(feeExpression);
-            if (exp == null) {
-                ParserContext mvelContext = new ParserContext();
-                mvelContext.addImport("MVEL", MVEL.class);
-                mvelContext.addImport("MvelHelper", MvelHelper.class);
-                exp = MVEL.compileExpression(feeExpression, mvelContext);
-            
-                EXPRESSION_CACHE.put(feeExpression, exp);
+        if (StringUtils.isNotEmpty(feeExpression)) {
+            synchronized (EXPRESSION_CACHE) {
+                HashMap<String, Object> vars = new HashMap<String, Object>();
+                vars.put("fulfillmentGroup", fulfillmentGroup);
+                MvelHelper.evaluateRule(feeExpression, vars, EXPRESSION_CACHE);
             }
-            HashMap<String, Object> vars = new HashMap<String, Object>();
-            vars.put("fulfillmentGroup", fulfillmentGroup);
-            return (Boolean)MVEL.executeExpression(feeExpression, vars);
         }
 
         return appliesToFulfillmentGroup;