diff --git a/src/main/java/org/codelibs/fess/indexer/DocBoostMatcher.java b/src/main/java/org/codelibs/fess/indexer/DocBoostMatcher.java index 2bd0b80e28..c54004f8ca 100644 --- a/src/main/java/org/codelibs/fess/indexer/DocBoostMatcher.java +++ b/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 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; diff --git a/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java b/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java index cfe3ba6c62..9a52d2ee4d 100644 --- a/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java +++ b/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. * - *

Thread Safety: This class is thread-safe. Each evaluate() call creates - * a new GroovyShell instance to ensure thread isolation.

+ *

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.

+ * + *

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 > 10", "10 * boost1 + boost2") and do not use + * static state, so this is acceptable.

* - *

Resource Management: GroovyClassLoader instances are properly managed - * and cleaned up after script evaluation to prevent memory leaks.

+ *

Resource Management: Each cached entry's GroovyClassLoader is closed on + * eviction via RemovalListener. All remaining entries are cleaned up via close() (@PreDestroy).

*/ 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 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 oldCache = scriptCache; + scriptCache = CacheBuilder.newBuilder() + .maximumSize(scriptCacheSize) + .removalListener((final RemovalNotification 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. * - *

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.

+ *

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".

* * @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,8 +135,6 @@ public GroovyEngine() { */ @Override public Object evaluate(final String template, final Map 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"); @@ -79,57 +142,76 @@ public Object evaluate(final String template, final Map paramMap return null; } - // Null-safety: use empty map if paramMap is null final Map safeParamMap = paramMap != null ? paramMap : Collections.emptyMap(); - // Prepare binding map with parameters and DI container final Map 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 scriptClass = (Class) 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 scriptClass; + private final GroovyClassLoader classLoader; + + CachedScript(final Class 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); + } + } + } + } diff --git a/src/main/resources/fess_se++.xml b/src/main/resources/fess_se++.xml index 6ffeed2ad3..4831a49ec7 100644 --- a/src/main/resources/fess_se++.xml +++ b/src/main/resources/fess_se++.xml @@ -4,6 +4,11 @@ + + diff --git a/src/test/java/org/codelibs/fess/indexer/DocBoostMatcherTest.java b/src/test/java/org/codelibs/fess/indexer/DocBoostMatcherTest.java index 66cf111abd..23a6b44d6b 100644 --- a/src/test/java/org/codelibs/fess/indexer/DocBoostMatcherTest.java +++ b/src/test/java/org/codelibs/fess/indexer/DocBoostMatcherTest.java @@ -136,4 +136,122 @@ public void test_boost_params() { map.put("boost2", 2); assertTrue(12.0f == docBoostMatcher.getValue(map)); } + + @Test + public void test_getValue_floatReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("1.5f"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(1.5f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_doubleReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("2.5d"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(2.5f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_longReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("100L"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(100.0f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_numericStringReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("'3.14'"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertTrue(Math.abs(3.14f - docBoostMatcher.getValue(map)) < 0.001f); + } + + @Test + public void test_getValue_emptyStringReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("''"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(0.0f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_nullReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("null"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(0.0f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_match_nullMap() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setMatchExpression("true"); + assertFalse(docBoostMatcher.match(null)); + } + + @Test + public void test_match_emptyMap() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setMatchExpression("true"); + assertFalse(docBoostMatcher.match(new HashMap<>())); + } + + @Test + public void test_match_nullExpression() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + // matchExpression is null by default + final Map map = new HashMap(); + map.put("data1", 1); + assertFalse(docBoostMatcher.match(map)); + } + + @Test + public void test_match_nonBooleanReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setMatchExpression("'string_value'"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertFalse(docBoostMatcher.match(map)); + } + + @Test + public void test_getValue_nonNumericString() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("'not_a_number'"); + docBoostMatcher.setMatchExpression("true"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(0.0f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_nullMap() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("10"); + assertEquals(0.0f, docBoostMatcher.getValue(null)); + } + + @Test + public void test_getValue_emptyMap() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("10"); + assertEquals(0.0f, docBoostMatcher.getValue(new HashMap<>())); + } } diff --git a/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java b/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java index 5807c173aa..665542b022 100644 --- a/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java +++ b/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java @@ -15,9 +15,16 @@ */ package org.codelibs.fess.script.groovy; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicInteger; import org.codelibs.fess.exception.JobProcessingException; import org.codelibs.fess.unit.UnitFessTestCase; @@ -37,6 +44,9 @@ protected void setUp(TestInfo testInfo) throws Exception { @Override protected void tearDown(TestInfo testInfo) throws Exception { + if (groovyEngine != null) { + groovyEngine.close(); + } ComponentUtil.setFessConfig(null); super.tearDown(testInfo); } @@ -408,6 +418,272 @@ public void test_evaluate_returnCollections() { assertTrue(mapResult instanceof java.util.Map); } + // ===== Cache Tests ===== + + /** + * Test that the same script evaluated multiple times produces correct results (cache hit) + */ + @Test + public void test_evaluate_cacheHit() { + final Map params = new HashMap<>(); + params.put("x", 5); + assertEquals(10, groovyEngine.evaluate("return x * 2", params)); + + params.put("x", 7); + assertEquals(14, groovyEngine.evaluate("return x * 2", params)); + + params.put("x", 0); + assertEquals(0, groovyEngine.evaluate("return x * 2", params)); + } + + /** + * Test that different scripts are cached separately + */ + @Test + public void test_evaluate_differentScriptsCached() { + final Map params = new HashMap<>(); + params.put("x", 5); + + assertEquals(10, groovyEngine.evaluate("return x * 2", params)); + assertEquals(25, groovyEngine.evaluate("return x * 5", params)); + assertEquals(10, groovyEngine.evaluate("return x * 2", params)); + } + + /** + * Test that cached scripts maintain binding isolation between evaluations + */ + @Test + public void test_evaluate_cacheBindingIsolation() { + final Map params1 = new HashMap<>(); + params1.put("name", "Alice"); + + final Map params2 = new HashMap<>(); + params2.put("name", "Bob"); + + assertEquals("Alice", groovyEngine.evaluate("return name", params1)); + assertEquals("Bob", groovyEngine.evaluate("return name", params2)); + assertEquals("Alice", groovyEngine.evaluate("return name", params1)); + } + + /** + * Test that configurable script cache size works with eviction + */ + @Test + public void test_configurableScriptCacheSize() { + final GroovyEngine engine = new GroovyEngine(); + engine.scriptCacheSize = 5; + engine.init(); + + final Map params = new HashMap<>(); + for (int i = 0; i < 10; i++) { + assertEquals(i, engine.evaluate("return " + i, params)); + } + // eviction後も再コンパイルで正しく動作 + assertEquals(0, engine.evaluate("return 0", params)); + engine.close(); + } + + /** + * Test that close() can be called without error + */ + @Test + public void test_close() { + final GroovyEngine engine = new GroovyEngine(); + engine.evaluate("return 1", new HashMap<>()); + engine.close(); + } + + /** + * Test that cache eviction works when exceeding max size (100). + * Evaluates 110 distinct scripts, then verifies all still produce correct results. + */ + @Test + public void test_evaluate_cacheEviction() { + final GroovyEngine engine = new GroovyEngine(); + final Map params = new HashMap<>(); + + for (int i = 0; i < 110; i++) { + assertEquals(i, engine.evaluate("return " + i, params)); + } + + // Earlier scripts should still work (recompiled after eviction) + assertEquals(0, engine.evaluate("return 0", params)); + assertEquals(50, engine.evaluate("return 50", params)); + assertEquals(109, engine.evaluate("return 109", params)); + + engine.close(); + } + + /** + * Test that cached scripts do not leak binding state between evaluations. + * Verifies that local variables in one evaluation do not affect the next. + */ + @Test + public void test_evaluate_noStateLeakBetweenEvaluations() { + final GroovyEngine engine = new GroovyEngine(); + final String script = "def counter = 0; counter += x; return counter"; + + final Map params1 = new HashMap<>(); + params1.put("x", 5); + assertEquals(5, engine.evaluate(script, params1)); + + // Second evaluation of the same cached script should not see previous state + final Map params2 = new HashMap<>(); + params2.put("x", 3); + assertEquals(3, engine.evaluate(script, params2)); + + engine.close(); + } + + // ===== Compilation Failure Tests ===== + + /** + * Test that syntax errors are not cached and can be retried + */ + @Test + public void test_evaluate_syntaxErrorNotCached() { + final GroovyEngine engine = new GroovyEngine(); + final Map params = new HashMap<>(); + final String invalidScript = "this is not valid {{{"; + + // First call: syntax error returns null + assertNull(engine.evaluate(invalidScript, params)); + + // Second call: should also return null (not cached, recompiles and fails again) + assertNull(engine.evaluate(invalidScript, params)); + + // Valid script should still work after syntax errors + assertEquals(42, engine.evaluate("return 42", params)); + + engine.close(); + } + + /** + * Test that repeated syntax errors don't accumulate resources + */ + @Test + public void test_evaluate_repeatedSyntaxErrors() { + final GroovyEngine engine = new GroovyEngine(); + final Map params = new HashMap<>(); + + for (int i = 0; i < 10; i++) { + assertNull(engine.evaluate("invalid script {{{ " + i, params)); + } + + // Engine should still function normally + assertEquals("ok", engine.evaluate("return 'ok'", params)); + + engine.close(); + } + + // ===== Concurrent Evaluation Tests ===== + + /** + * Test concurrent evaluation of the same script from multiple threads + */ + @Test + public void test_evaluate_concurrentSameScript() throws Exception { + final GroovyEngine engine = new GroovyEngine(); + final int threadCount = 8; + final int iterationsPerThread = 50; + final ExecutorService executor = Executors.newFixedThreadPool(threadCount); + final CountDownLatch startLatch = new CountDownLatch(1); + final AtomicInteger errors = new AtomicInteger(0); + + final List> futures = new ArrayList<>(); + for (int t = 0; t < threadCount; t++) { + final int threadId = t; + futures.add(executor.submit(() -> { + try { + startLatch.await(); + for (int i = 0; i < iterationsPerThread; i++) { + final Map params = new HashMap<>(); + params.put("x", threadId * 1000 + i); + final Object result = engine.evaluate("return x * 2", params); + if (result == null || !result.equals((threadId * 1000 + i) * 2)) { + errors.incrementAndGet(); + } + } + } catch (final Exception e) { + errors.incrementAndGet(); + } + })); + } + + startLatch.countDown(); + for (final Future future : futures) { + future.get(); + } + + executor.shutdown(); + assertEquals(0, errors.get()); + engine.close(); + } + + /** + * Test concurrent evaluation of different scripts from multiple threads + */ + @Test + public void test_evaluate_concurrentDifferentScripts() throws Exception { + final GroovyEngine engine = new GroovyEngine(); + final int threadCount = 4; + final int scriptsPerThread = 20; + final ExecutorService executor = Executors.newFixedThreadPool(threadCount); + final CountDownLatch startLatch = new CountDownLatch(1); + final AtomicInteger errors = new AtomicInteger(0); + + final List> futures = new ArrayList<>(); + for (int t = 0; t < threadCount; t++) { + final int threadId = t; + futures.add(executor.submit(() -> { + try { + startLatch.await(); + for (int i = 0; i < scriptsPerThread; i++) { + final Map params = new HashMap<>(); + final int expected = threadId * 100 + i; + final Object result = engine.evaluate("return " + expected, params); + if (result == null || !result.equals(expected)) { + errors.incrementAndGet(); + } + } + } catch (final Exception e) { + errors.incrementAndGet(); + } + })); + } + + startLatch.countDown(); + for (final Future future : futures) { + future.get(); + } + + executor.shutdown(); + assertEquals(0, errors.get()); + engine.close(); + } + + // ===== Close Lifecycle Tests ===== + + /** + * Test that close() can be called multiple times without error + */ + @Test + public void test_close_idempotent() { + final GroovyEngine engine = new GroovyEngine(); + engine.evaluate("return 1", new HashMap<>()); + engine.close(); + engine.close(); // second close should not throw + } + + /** + * Test that close() on a fresh engine (no evaluations) works + */ + @Test + public void test_close_noEvaluations() { + final GroovyEngine engine = new GroovyEngine(); + engine.close(); + } + // ===== Edge Cases ===== /**