refactor(script): add compiled script caching to GroovyEngine and improve DocBoostMatcher error handling (#3074)

* refactor(script): add compiled script caching to GroovyEngine and improve DocBoostMatcher error handling

GroovyEngine now caches compiled Groovy Script classes in an LRU map (max 100 entries),
avoiding repeated compilation of the same script text. Each evaluation still creates a
fresh Script instance for thread-safe binding isolation. GroovyClassLoaders are properly
closed on eviction and via @PreDestroy.

DocBoostMatcher now catches NumberFormatException when parsing boost values, logging a
warning and returning 0.0f instead of propagating the exception.

Includes comprehensive tests for cache behavior, binding isolation, eviction, concurrent
evaluation, close lifecycle, and new DocBoostMatcher edge cases.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* refactor(script): replace synchronized LinkedHashMap with Guava Cache in GroovyEngine

Replace the manual synchronized LinkedHashMap-based LRU cache with Guava
Cache for compiled Groovy scripts. This provides segment-based locking
for lock-free concurrent reads and automatic eviction via RemovalListener.
Also makes scriptCacheSize and maxScriptLogLength configurable via DI.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>

Author: marevol

src/main/java/org/codelibs/fess/indexer/DocBoostMatcher.java

@@ -17,6 +17,8 @@
 
 import java.util.Map;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.codelibs.fess.Constants;
 import org.codelibs.fess.opensearch.config.exentity.BoostDocumentRule;
 import org.codelibs.fess.util.ComponentUtil;
@@ -29,6 +31,7 @@
  *
  */
 public class DocBoostMatcher {
+    private static final Logger logger = LogManager.getLogger(DocBoostMatcher.class);
 
     /** The expression used to calculate the boost value (defaults to "0") */
     private String boostExpression = "0";
@@ -105,7 +108,12 @@ public float getValue(final Map<String, Object> map) {
             return ((Double) value).floatValue();
         }
         if (value != null) {
-            return Float.parseFloat(value.toString());
+            try {
+                return Float.parseFloat(value.toString());
+            } catch (final NumberFormatException e) {
+                logger.warn("Failed to parse boost value: expression={}, value={}", boostExpression, value, e);
+                return 0.0f;
+            }
         }
 
         return 0.0f;

src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java

@@ -15,9 +15,11 @@
  */
 package org.codelibs.fess.script.groovy;
 
+import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ExecutionException;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -30,37 +32,100 @@
 import org.lastaflute.di.core.factory.SingletonLaContainerFactory;
 import org.lastaflute.job.LaJobRuntime;
 
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.RemovalNotification;
+
 import groovy.lang.Binding;
 import groovy.lang.GroovyClassLoader;
-import groovy.lang.GroovyShell;
+import groovy.lang.Script;
+import jakarta.annotation.PostConstruct;
+import jakarta.annotation.PreDestroy;
 
 /**
  * Groovy script engine implementation that extends AbstractScriptEngine.
  * This class provides support for executing Groovy scripts with parameter binding
  * and DI container integration.
  *
- * <p>Thread Safety: This class is thread-safe. Each evaluate() call creates
- * a new GroovyShell instance to ensure thread isolation.</p>
+ * <p>Thread Safety: This class is thread-safe. Each cached entry holds its own
+ * GroovyClassLoader. The cache uses Guava Cache with segment-based locking for
+ * lock-free concurrent reads. Each evaluate() call creates a new Script instance
+ * to ensure thread isolation of bindings.</p>
+ *
+ * <p>Note on class-level isolation: Compiled Script classes are cached and reused.
+ * Class-level state (static fields, metaclass mutations) persists across evaluations
+ * of the same script. In Fess, scripts are short expressions configured by
+ * administrators (e.g., "data1 &gt; 10", "10 * boost1 + boost2") and do not use
+ * static state, so this is acceptable.</p>
  *
- * <p>Resource Management: GroovyClassLoader instances are properly managed
- * and cleaned up after script evaluation to prevent memory leaks.</p>
+ * <p>Resource Management: Each cached entry's GroovyClassLoader is closed on
+ * eviction via RemovalListener. All remaining entries are cleaned up via close() (@PreDestroy).</p>
  */
 public class GroovyEngine extends AbstractScriptEngine {
     private static final Logger logger = LogManager.getLogger(GroovyEngine.class);
 
+    /** Maximum number of compiled scripts to cache. Configurable via DI. */
+    protected int scriptCacheSize = 1000;
+
+    /** Maximum length of script text included in warning log messages. Configurable via DI. */
+    protected int maxScriptLogLength = 200;
+
+    private Cache<String, CachedScript> scriptCache;
+
     /**
      * Default constructor for GroovyEngine.
      */
     public GroovyEngine() {
         super();
+        buildScriptCache();
+    }
+
+    /**
+     * Rebuilds the script cache after DI injection.
+     * Called by the DI container after property injection.
+     */
+    @PostConstruct
+    public void init() {
+        buildScriptCache();
+    }
+
+    private void buildScriptCache() {
+        final Cache<String, CachedScript> oldCache = scriptCache;
+        scriptCache = CacheBuilder.newBuilder()
+                .maximumSize(scriptCacheSize)
+                .removalListener((final RemovalNotification<String, CachedScript> notification) -> {
+                    notification.getValue().close();
+                })
+                .build();
+        if (oldCache != null) {
+            oldCache.invalidateAll();
+        }
+    }
+
+    /**
+     * Sets the maximum number of compiled scripts to cache.
+     *
+     * @param scriptCacheSize the cache size
+     */
+    public void setScriptCacheSize(final int scriptCacheSize) {
+        this.scriptCacheSize = scriptCacheSize;
+    }
+
+    /**
+     * Sets the maximum length of script text included in warning log messages.
+     *
+     * @param maxScriptLogLength the max length
+     */
+    public void setMaxScriptLogLength(final int maxScriptLogLength) {
+        this.maxScriptLogLength = maxScriptLogLength;
     }
 
     /**
      * Evaluates a Groovy script template with the provided parameters.
      *
-     * <p>This method creates a new GroovyShell instance for each evaluation to ensure
-     * thread safety. The DI container is automatically injected into the binding map
-     * as "container" for script access.</p>
+     * <p>This method caches compiled Script classes per script text.
+     * Each evaluation creates a new Script instance to ensure thread-safe binding isolation.
+     * The DI container is automatically injected into the binding map as "container".</p>
      *
      * @param template the Groovy script to evaluate (null-safe, returns null if empty)
      * @param paramMap the parameters to bind to the script (null-safe, treated as empty map if null)
@@ -70,66 +135,83 @@ public GroovyEngine() {
      */
     @Override
     public Object evaluate(final String template, final Map<String, Object> paramMap) {
-        // Null-safety: return null for blank templates
-        // Early return is safe here as no resources have been allocated yet
         if (StringUtil.isBlank(template)) {
             if (logger.isDebugEnabled()) {
                 logger.debug("Template is blank, returning null");
             }
             return null;
         }
 
-        // Null-safety: use empty map if paramMap is null
         final Map<String, Object> safeParamMap = paramMap != null ? paramMap : Collections.emptyMap();
 
-        // Prepare binding map with parameters and DI container
         final Map<String, Object> bindingMap = new HashMap<>(safeParamMap);
         bindingMap.put("container", SingletonLaContainerFactory.getContainer());
 
-        // Create GroovyShell with custom class loader for proper resource management
-        GroovyClassLoader classLoader = null;
         try {
-            // Get parent class loader with fallback to ensure robustness across threading contexts
-            ClassLoader parentClassLoader = Thread.currentThread().getContextClassLoader();
-            if (parentClassLoader == null) {
-                parentClassLoader = GroovyEngine.class.getClassLoader();
-            }
-            classLoader = new GroovyClassLoader(parentClassLoader);
-            final GroovyShell groovyShell = new GroovyShell(classLoader, new Binding(bindingMap));
+            final CachedScript cached = getOrCompile(template);
+            final Script script = cached.scriptClass.getDeclaredConstructor().newInstance();
+            script.setBinding(new Binding(bindingMap));
 
             if (logger.isDebugEnabled()) {
                 logger.debug("Evaluating Groovy script: template={}", template);
             }
 
-            final Object result = groovyShell.evaluate(template);
+            final Object result = script.run();
             logScriptExecution(template, "success");
             return result;
         } catch (final JobProcessingException e) {
-            // Rethrow JobProcessingException to allow scripts to signal job-specific errors
-            // that should be handled by the job framework
             if (logger.isDebugEnabled()) {
                 logger.debug("Script raised JobProcessingException", e);
             }
             logScriptExecution(template, "failure:" + e.getClass().getSimpleName());
             throw e;
         } catch (final Exception e) {
-            // Log and return null for other exceptions to maintain backward compatibility
-            logger.warn("Failed to evaluate Groovy script: template={}, parameters={}", template, safeParamMap, e);
+            final String truncatedScript =
+                    template.length() > maxScriptLogLength ? template.substring(0, maxScriptLogLength) + "..." : template;
+            logger.warn("Failed to evaluate Groovy script: script(length={})={}, parameterKeys={}", template.length(), truncatedScript,
+                    safeParamMap.keySet(), e);
             logScriptExecution(template, "failure:" + e.getClass().getSimpleName());
             return null;
-        } finally {
-            // Properly clean up GroovyClassLoader resources
-            if (classLoader != null) {
+        }
+    }
+
+    @SuppressWarnings("unchecked")
+    private CachedScript getOrCompile(final String template) {
+        try {
+            return scriptCache.get(template, () -> {
+                ClassLoader parentClassLoader = Thread.currentThread().getContextClassLoader();
+                if (parentClassLoader == null) {
+                    parentClassLoader = GroovyEngine.class.getClassLoader();
+                }
+                final GroovyClassLoader classLoader = new GroovyClassLoader(parentClassLoader);
                 try {
-                    classLoader.clearCache();
-                    classLoader.close();
+                    final Class<? extends Script> scriptClass = (Class<? extends Script>) classLoader.parseClass(template);
+                    return new CachedScript(scriptClass, classLoader);
                 } catch (final Exception e) {
-                    logger.warn("Failed to close GroovyClassLoader", e);
+                    try {
+                        classLoader.clearCache();
+                        classLoader.close();
+                    } catch (final IOException closeEx) {
+                        logger.warn("Failed to close GroovyClassLoader after compilation failure", closeEx);
+                    }
+                    throw e;
                 }
-            }
+            });
+        } catch (final ExecutionException e) {
+            throw (RuntimeException) e.getCause();
         }
     }
 
+    /**
+     * Closes all cached GroovyClassLoaders and clears the script cache.
+     * Called by the DI container on shutdown.
+     */
+    @PreDestroy
+    public void close() {
+        scriptCache.invalidateAll();
+        scriptCache.cleanUp();
+    }
+
     /**
      * Returns the name identifier for this script engine.
      *
@@ -195,4 +277,27 @@ protected void logScriptExecution(final String script, final String result) {
         }
     }
 
+    /**
+     * Holds a compiled Script class and its associated GroovyClassLoader.
+     * When evicted from the cache, close() releases the class loader resources.
+     */
+    private static class CachedScript {
+        final Class<? extends Script> scriptClass;
+        private final GroovyClassLoader classLoader;
+
+        CachedScript(final Class<? extends Script> scriptClass, final GroovyClassLoader classLoader) {
+            this.scriptClass = scriptClass;
+            this.classLoader = classLoader;
+        }
+
+        void close() {
+            try {
+                classLoader.clearCache();
+                classLoader.close();
+            } catch (final IOException e) {
+                LogManager.getLogger(GroovyEngine.class).warn("Failed to close GroovyClassLoader", e);
+            }
+        }
+    }
+
 }

src/main/resources/fess_se++.xml

@@ -4,6 +4,11 @@
 <components>
 	<component name="groovyEngine"
 		class="org.codelibs.fess.script.groovy.GroovyEngine">
+		<!--
+		<property name="scriptCacheSize">1000</property>
+		<property name="maxScriptLogLength">200</property>
+		-->
+		<postConstruct name="init"></postConstruct>
 		<postConstruct name="register"></postConstruct>
 	</component>
 </components>