[incubator-kie-drools-6016] Emit a warning when an eval is improperly…

… and unnecessarily used
apache · Jul 12, 2024 · fd33fc1 · fd33fc1
1 parent ddb72c6
commit fd33fc1
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 1 deletion.
diff --git a/drools-base/src/main/java/org/drools/base/rule/EvalCondition.java b/drools-base/src/main/java/org/drools/base/rule/EvalCondition.java
@@ -32,11 +32,18 @@
 import org.drools.base.rule.accessor.CompiledInvoker;
 import org.drools.base.rule.accessor.EvalExpression;
 import org.drools.base.rule.accessor.Wireable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class EvalCondition extends ConditionalElement
     implements
     Externalizable,
         Wireable {
+
+    private static final Logger LOG = LoggerFactory.getLogger(EvalCondition.class);
+
+    private static long warnLogCounter = 0;
+
     private static final long          serialVersionUID   = 510l;
 
     protected EvalExpression             expression;
@@ -223,4 +230,36 @@ public void setCloned(List<EvalCondition> cloned) {
     public String toString() {
         return this.expression.toString();
     }
+
+    public static void logWarnIfImproperEval(EvalCondition evalCondition, String evalExpression) {
+        if (warnLogCounter == 10) {
+            warnLogCounter++;
+            LOG.warn("More eval warnings will be suppressed...");
+            return;
+        } else if (warnLogCounter > 10) {
+            return; // avoid flooding the logs
+        }
+
+        if (evalExpression == null || evalExpression.isEmpty()) {
+            return; // cannot provide a meaningful warning
+        }
+
+        StringBuilder sb = new StringBuilder();
+        for (Declaration declaration : evalCondition.getRequiredDeclarations()) {
+            if (declaration.getPattern() != null) {
+                sb.append("'");
+                sb.append(declaration.getIdentifier());
+                sb.append("' comes from previous pattern '");
+                String className = declaration.getPattern().getObjectType().getClassName();
+                sb.append(className.substring(className.lastIndexOf('.') + 1));
+                sb.append("'. ");
+            }
+        }
+        if (!sb.isEmpty()) {
+            warnLogCounter++;
+            LOG.warn("In an eval expression [{}] : {}" +
+                             "Consider placing the constraint in the pattern and removing the eval if possible," +
+                             " as eval is not performance-efficient.", evalExpression, sb);
+        }
+    }
 }
diff --git a/...odel/drools-model-compiler/src/main/java/org/drools/modelcompiler/KiePackagesBuilder.java b/...odel/drools-model-compiler/src/main/java/org/drools/modelcompiler/KiePackagesBuilder.java
@@ -151,6 +151,7 @@
 import org.kie.internal.builder.conf.PropertySpecificOption;
 
 import static java.util.stream.Collectors.toList;
+import static org.drools.base.rule.EvalCondition.logWarnIfImproperEval;
 import static org.drools.base.rule.GroupElement.AND;
 import static org.drools.base.rule.GroupElement.OR;
 import static org.drools.compiler.rule.builder.RuleBuilder.buildTimer;
@@ -669,7 +670,9 @@ private void recursivelyAddConditions(RuleContext ctx, GroupElement group, Group
     private EvalCondition buildEval(RuleContext ctx, EvalImpl eval) {
         Declaration[] declarations = Stream.of( eval.getExpr().getVariables() ).map( ctx::getDeclaration ).toArray( Declaration[]::new );
         EvalExpression evalExpr = new LambdaEvalExpression(declarations, eval.getExpr());
-        return new EvalCondition(evalExpr, declarations);
+        EvalCondition evalCondition = new EvalCondition(evalExpr, declarations);
+        logWarnIfImproperEval(evalCondition, eval.getExpr().predicateInformation().getStringConstraint());
+        return evalCondition;
     }
 
     private ConditionalBranch buildConditionalConsequence(RuleContext ctx, ConditionalNamedConsequenceImpl consequence) {

diff --git a/drools-mvel/src/main/java/org/drools/mvel/asm/AbstractASMEvalBuilder.java b/drools-mvel/src/main/java/org/drools/mvel/asm/AbstractASMEvalBuilder.java
@@ -38,6 +38,7 @@
 import org.drools.base.rule.RuleConditionElement;
 import org.drools.base.rule.accessor.DeclarationScopeResolver;
 
+import static org.drools.base.rule.EvalCondition.logWarnIfImproperEval;
 import static org.drools.compiler.rule.builder.PatternBuilder.buildAnalysis;
 import static org.drools.compiler.rule.builder.PatternBuilder.createImplicitBindings;
 import static org.drools.mvel.java.JavaRuleBuilderHelper.createVariableContext;
@@ -90,6 +91,7 @@ private RuleConditionElement buildEval(RuleBuildContext context, EvalDescr evalD
         Arrays.sort(declarations, SortDeclarations.instance);
 
         EvalCondition eval = EvalConditionFactory.Factory.get().createEvalCondition(declarations);
+        logWarnIfImproperEval(eval, (String) evalDescr.getContent());
 
         Map<String, Object> vars = createVariableContext( className,
                                                           (String)evalDescr.getContent(),

diff --git a/drools-mvel/src/main/java/org/drools/mvel/builder/MVELEvalBuilder.java b/drools-mvel/src/main/java/org/drools/mvel/builder/MVELEvalBuilder.java
@@ -40,6 +40,7 @@
 import org.drools.mvel.expr.MVELCompilationUnit;
 import org.drools.mvel.expr.MVELEvalExpression;
 
+import static org.drools.base.rule.EvalCondition.logWarnIfImproperEval;
 import static org.drools.mvel.asm.AsmUtil.copyErrorLocation;
 
 public class MVELEvalBuilder
@@ -99,6 +100,7 @@ public RuleConditionElement build(final RuleBuildContext context,
                                                                        false,
                                                                        MVELCompilationUnit.Scope.EXPRESSION );
             final EvalCondition eval = EvalConditionFactory.Factory.get().createEvalCondition( previousDeclarations );
+            logWarnIfImproperEval(eval, (String) evalDescr.getContent());
 
             MVELEvalExpression expr = new MVELEvalExpression( unit,
                                                               dialect.getId() );