Scripting: File scripts cache key to include language to prevent conflicts

The file scripts cache key should include the language of the script to prevent conflicts between scripts with same name but different extension (hence lang). Note that script engines can register multiple acronyms that may be used as lang at execution time (e.g. javascript and js are synonyms). We then need to make sure that the same script gets loaded no matter which of the acronyms is used at execution time. The problem didn't exist before this change ad the lang was simply ignored, while now we take it into account.

This change has also some positive effect on inline scripts caching. Up until now, the same script referred to with different acronyms would be compiled and cached multiple times, once per acronym. After this change every script gets compiled and cached only once, as we chose internally the acronym used as part of the cache key, no matter which one the user provides.

Closes #10033

src/main/java/org/elasticsearch/node/Node.java

@@ -380,7 +380,11 @@ public class Node implements Releasable {
         }
 
         stopWatch.stop().start("script");
-        injector.getInstance(ScriptService.class).close();
+        try {
+            injector.getInstance(ScriptService.class).close();
+        } catch(IOException e) {
+            logger.warn("ScriptService close failed", e);
+        }
 
         stopWatch.stop().start("thread_pool");
         // TODO this should really use ThreadPool.terminate()

src/main/java/org/elasticsearch/script/ScriptEngineService.java

@@ -22,12 +22,13 @@ package org.elasticsearch.script;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.search.lookup.SearchLookup;
 
+import java.io.Closeable;
 import java.util.Map;
 
 /**
  *
  */
-public interface ScriptEngineService {
+public interface ScriptEngineService extends Closeable {
 
     String[] types();
 
@@ -45,8 +46,6 @@ public interface ScriptEngineService {
 
     Object unwrap(Object value);
 
-    void close();
-
     /**
      * Handler method called when a script is removed from the Guava cache.
      *

src/main/java/org/elasticsearch/script/ScriptService.java

@@ -25,6 +25,7 @@ import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.RemovalListener;
 import com.google.common.cache.RemovalNotification;
 import com.google.common.collect.ImmutableMap;
+import org.apache.lucene.util.IOUtils;
 import org.elasticsearch.ElasticsearchIllegalArgumentException;
 import org.elasticsearch.ElasticsearchIllegalStateException;
 import org.elasticsearch.action.ActionListener;
@@ -65,6 +66,7 @@ import org.elasticsearch.watcher.FileChangesListener;
 import org.elasticsearch.watcher.FileWatcher;
 import org.elasticsearch.watcher.ResourceWatcherService;
 
+import java.io.Closeable;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.nio.file.Files;
@@ -78,7 +80,7 @@ import java.util.concurrent.TimeUnit;
 /**
  *
  */
-public class ScriptService extends AbstractComponent {
+public class ScriptService extends AbstractComponent implements Closeable {
 
     public static final String DEFAULT_SCRIPTING_LANGUAGE_SETTING = "script.default_lang";
     public static final String DISABLE_DYNAMIC_SCRIPTING_SETTING = "script.disable_dynamic";
@@ -91,9 +93,11 @@ public class ScriptService extends AbstractComponent {
 
     private final String defaultLang;
 
-    private final ImmutableMap<String, ScriptEngineService> scriptEngines;
+    private final Set<ScriptEngineService> scriptEngines;
+    private final ImmutableMap<String, ScriptEngineService> scriptEnginesByLang;
+    private final ImmutableMap<String, ScriptEngineService> scriptEnginesByExt;
 
-    private final ConcurrentMap<String, CompiledScript> staticCache = ConcurrentCollections.newConcurrentMap();
+    private final ConcurrentMap<CacheKey, CompiledScript> staticCache = ConcurrentCollections.newConcurrentMap();
 
     private final Cache<CacheKey, CompiledScript> cache;
     private final Path scriptsDirectory;
@@ -144,6 +148,7 @@ public class ScriptService extends AbstractComponent {
                          ResourceWatcherService resourceWatcherService, NodeSettingsService nodeSettingsService) throws IOException {
         super(settings);
 
+        this.scriptEngines = scriptEngines;
         int cacheMaxSize = settings.getAsInt(SCRIPT_CACHE_SIZE_SETTING, 100);
         TimeValue cacheExpire = settings.getAsTime(SCRIPT_CACHE_EXPIRE_SETTING, null);
         logger.debug("using script cache with max_size [{}], expire [{}]", cacheMaxSize, cacheExpire);
@@ -161,13 +166,18 @@ public class ScriptService extends AbstractComponent {
         cacheBuilder.removalListener(new ScriptCacheRemovalListener());
         this.cache = cacheBuilder.build();
 
-        ImmutableMap.Builder<String, ScriptEngineService> builder = ImmutableMap.builder();
+        ImmutableMap.Builder<String, ScriptEngineService> enginesByLangBuilder = ImmutableMap.builder();
+        ImmutableMap.Builder<String, ScriptEngineService> enginesByExtBuilder = ImmutableMap.builder();
         for (ScriptEngineService scriptEngine : scriptEngines) {
             for (String type : scriptEngine.types()) {
-                builder.put(type, scriptEngine);
+                enginesByLangBuilder.put(type, scriptEngine);
+            }
+            for (String ext : scriptEngine.extensions()) {
+                enginesByExtBuilder.put(ext, scriptEngine);
             }
         }
-        this.scriptEngines = builder.build();
+        this.scriptEnginesByLang = enginesByLangBuilder.build();
+        this.scriptEnginesByExt = enginesByExtBuilder.build();
 
         // add file watcher for static scripts
         scriptsDirectory = env.configFile().resolve("scripts");
@@ -193,10 +203,9 @@ public class ScriptService extends AbstractComponent {
         this.client = client;
     }
 
-    public void close() {
+    @Override
-        for (ScriptEngineService engineService : scriptEngines.values()) {
+    public void close() throws IOException {
-            engineService.close();
+        IOUtils.close(scriptEngines);
-        }
     }
 
     /**
@@ -214,14 +223,22 @@ public class ScriptService extends AbstractComponent {
         this.fileWatcher.clearState();
     }
 
-    private ScriptEngineService getScriptEngineService(String lang) {
+    private ScriptEngineService getScriptEngineServiceForLang(String lang) {
-        ScriptEngineService scriptEngineService = scriptEngines.get(lang);
+        ScriptEngineService scriptEngineService = scriptEnginesByLang.get(lang);
         if (scriptEngineService == null) {
             throw new ElasticsearchIllegalArgumentException("script_lang not supported [" + lang + "]");
         }
         return scriptEngineService;
     }
 
+    private ScriptEngineService getScriptEngineServiceForFileExt(String fileExtension) {
+        ScriptEngineService scriptEngineService = scriptEnginesByExt.get(fileExtension);
+        if (scriptEngineService == null) {
+            throw new ElasticsearchIllegalArgumentException("script file extension not supported [" + fileExtension + "]");
+        }
+        return scriptEngineService;
+    }
+
     /**
      * Compiles a script straight-away, or returns the previously compiled and cached script, without checking if it can be executed based on settings.
      */
@@ -233,15 +250,17 @@ public class ScriptService extends AbstractComponent {
             logger.trace("Compiling lang: [{}] type: [{}] script: {}", lang, scriptType, script);
         }
 
+        ScriptEngineService scriptEngineService = getScriptEngineServiceForLang(lang);
+        CacheKey cacheKey = newCacheKey(scriptEngineService, script);
+
         if (scriptType == ScriptType.FILE) {
-            CompiledScript compiled = staticCache.get(script); //On disk scripts will be loaded into the staticCache by the listener
+            CompiledScript compiled = staticCache.get(cacheKey); //On disk scripts will be loaded into the staticCache by the listener
             if (compiled == null) {
                 throw new ElasticsearchIllegalArgumentException("Unable to find on disk script " + script);
             }
             return compiled;
         }
 
-        ScriptEngineService scriptEngineService = getScriptEngineService(lang);
         verifyDynamicScripting(lang, scriptEngineService);
 
         if (scriptType == ScriptType.INDEXED) {
@@ -252,7 +271,6 @@ public class ScriptService extends AbstractComponent {
             script = getScriptFromIndex(client, indexedScript.lang, indexedScript.id);
         }
 
-        CacheKey cacheKey = new CacheKey(lang, script);
         CompiledScript compiled = cache.getIfPresent(cacheKey);
         if (compiled == null) {
             //Either an un-cached inline script or an indexed script
@@ -282,8 +300,8 @@ public class ScriptService extends AbstractComponent {
     private String validateScriptLanguage(String scriptLang) {
         if (scriptLang == null) {
             scriptLang = defaultLang;
-        } else if (!scriptEngines.containsKey(scriptLang)) {
+        } else if (scriptEnginesByLang.containsKey(scriptLang) == false) {
-            throw new ElasticsearchIllegalArgumentException("script_lang not supported ["+scriptLang+"]");
+            throw new ElasticsearchIllegalArgumentException("script_lang not supported [" + scriptLang + "]");
         }
         return scriptLang;
     }
@@ -382,7 +400,7 @@ public class ScriptService extends AbstractComponent {
      * Executes a previously compiled script provided as an argument
      */
     public ExecutableScript executable(CompiledScript compiledScript, Map<String, Object> vars) {
-        return scriptEngines.get(compiledScript.lang()).executable(compiledScript.compiled(), vars);
+        return getScriptEngineServiceForLang(compiledScript.lang()).executable(compiledScript.compiled(), vars);
     }
 
     /**
@@ -390,7 +408,7 @@ public class ScriptService extends AbstractComponent {
      */
     public SearchScript search(SearchLookup lookup, String lang, String script, ScriptType scriptType, @Nullable Map<String, Object> vars) {
         CompiledScript compiledScript = compile(lang, script, scriptType);
-        return scriptEngines.get(compiledScript.lang()).search(compiledScript.compiled(), lookup, vars);
+        return getScriptEngineServiceForLang(compiledScript.lang()).search(compiledScript.compiled(), lookup, vars);
     }
 
     private boolean dynamicScriptEnabled(String lang, ScriptEngineService scriptEngineService) {
@@ -418,7 +436,7 @@ public class ScriptService extends AbstractComponent {
             if (logger.isDebugEnabled()) {
                 logger.debug("notifying script services of script removal due to: [{}]", notification.getCause());
             }
-            for (ScriptEngineService service : scriptEngines.values()) {
+            for (ScriptEngineService service : scriptEngines) {
                 try {
                     service.scriptRemoved(notification.getValue());
                 } catch (Exception e) {
@@ -451,27 +469,20 @@ public class ScriptService extends AbstractComponent {
             }
             Tuple<String, String> scriptNameExt = scriptNameExt(file);
             if (scriptNameExt != null) {
-                boolean found = false;
+                ScriptEngineService engineService = getScriptEngineServiceForFileExt(scriptNameExt.v2());
-                for (ScriptEngineService engineService : scriptEngines.values()) {
+                if (engineService == null) {
-                    for (String s : engineService.extensions()) {
-                        if (s.equals(scriptNameExt.v2())) {
-                            found = true;
-                            try {
-                                logger.info("compiling script file [{}]", file.toAbsolutePath());
-                                String script = Streams.copyToString(new InputStreamReader(Files.newInputStream(file), Charsets.UTF_8));
-                                staticCache.put(scriptNameExt.v1(), new CompiledScript(engineService.types()[0], engineService.compile(script)));
-                            } catch (Throwable e) {
-                                logger.warn("failed to load/compile script [{}]", e, scriptNameExt.v1());
-                            }
-                            break;
-                        }
-                    }
-                    if (found) {
-                        break;
-                    }
-                }
-                if (!found) {
                     logger.warn("no script engine found for [{}]", scriptNameExt.v2());
+                } else {
+                    try {
+                        logger.info("compiling script file [{}]", file.toAbsolutePath());
+                        try(InputStreamReader reader = new InputStreamReader(Files.newInputStream(file), Charsets.UTF_8)) {
+                            String script = Streams.copyToString(reader);
+                            CacheKey cacheKey = newCacheKey(engineService, scriptNameExt.v1());
+                            staticCache.put(cacheKey, new CompiledScript(engineService.types()[0], engineService.compile(script)));
+                        }
+                    } catch (Throwable e) {
+                        logger.warn("failed to load/compile script [{}]", e, scriptNameExt.v1());
+                    }
                 }
             }
         }
@@ -485,8 +496,10 @@ public class ScriptService extends AbstractComponent {
         public void onFileDeleted(Path file) {
             Tuple<String, String> scriptNameExt = scriptNameExt(file);
             if (scriptNameExt != null) {
+                ScriptEngineService engineService = getScriptEngineServiceForFileExt(scriptNameExt.v2());
+                assert engineService != null;
                 logger.info("removing script file [{}]", file.toAbsolutePath());
-                staticCache.remove(scriptNameExt.v1());
+                staticCache.remove(newCacheKey(engineService, scriptNameExt.v1()));
             }
         }
 
@@ -549,6 +562,10 @@ public class ScriptService extends AbstractComponent {
         }
     }
 
+    private static CacheKey newCacheKey(ScriptEngineService engineService, String script) {
+        return new CacheKey(engineService.types()[0], script);
+    }
+
     private static class CacheKey {
         public final String lang;
         public final String script;
@@ -599,7 +616,7 @@ public class ScriptService extends AbstractComponent {
     private class ApplySettings implements NodeSettingsService.Listener {
         @Override
         public void onRefreshSettings(Settings settings) {
-            GroovyScriptEngineService engine = (GroovyScriptEngineService) ScriptService.this.scriptEngines.get("groovy");
+            GroovyScriptEngineService engine = (GroovyScriptEngineService) ScriptService.this.scriptEnginesByLang.get(GroovyScriptEngineService.NAME);
             if (engine != null) {
                 String[] patches = settings.getAsArray(GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH, Strings.EMPTY_ARRAY);
                 boolean blacklistChanged = engine.addToBlacklist(patches);

src/test/java/org/elasticsearch/script/OnDiskScriptTests.java

@@ -44,27 +44,45 @@ public class OnDiskScriptTests extends ElasticsearchIntegrationTest {
                 .put("path.conf", this.getResourcePath("config")).build();
     }
 
-
     @Test
     public void testFieldOnDiskScript()  throws ExecutionException, InterruptedException {
 
-        List<IndexRequestBuilder> builders = new ArrayList();
+        List<IndexRequestBuilder> builders = new ArrayList<>();
         builders.add(client().prepareIndex("test", "scriptTest", "1").setSource("{\"theField\":\"foo\"}"));
         builders.add(client().prepareIndex("test", "scriptTest", "2").setSource("{\"theField\":\"foo 2\"}"));
         builders.add(client().prepareIndex("test", "scriptTest", "3").setSource("{\"theField\":\"foo 3\"}"));
         builders.add(client().prepareIndex("test", "scriptTest", "4").setSource("{\"theField\":\"foo 4\"}"));
         builders.add(client().prepareIndex("test", "scriptTest", "5").setSource("{\"theField\":\"bar\"}"));
 
-        indexRandom(true,builders);
+        indexRandom(true, builders);
 
         String query = "{ \"query\" : { \"match_all\": {}} , \"script_fields\" : { \"test1\" : { \"script_file\" : \"script1\" }, \"test2\" : { \"script_file\" : \"script2\", \"params\":{\"factor\":3}  }}, size:1}";
         SearchResponse searchResponse = client().prepareSearch().setSource(query).setIndices("test").setTypes("scriptTest").get();
-        assertHitCount(searchResponse,5);
+        assertHitCount(searchResponse, 5);
         assertTrue(searchResponse.getHits().hits().length == 1);
         SearchHit sh = searchResponse.getHits().getAt(0);
         assertThat((Integer)sh.field("test1").getValue(), equalTo(2));
         assertThat((Integer)sh.field("test2").getValue(), equalTo(6));
     }
 
+    @Test
+    public void testOnDiskScriptsSameNameDifferentLang()  throws ExecutionException, InterruptedException {
 
+        List<IndexRequestBuilder> builders = new ArrayList<>();
+        builders.add(client().prepareIndex("test", "scriptTest", "1").setSource("{\"theField\":\"foo\"}"));
+        builders.add(client().prepareIndex("test", "scriptTest", "2").setSource("{\"theField\":\"foo 2\"}"));
+        builders.add(client().prepareIndex("test", "scriptTest", "3").setSource("{\"theField\":\"foo 3\"}"));
+        builders.add(client().prepareIndex("test", "scriptTest", "4").setSource("{\"theField\":\"foo 4\"}"));
+        builders.add(client().prepareIndex("test", "scriptTest", "5").setSource("{\"theField\":\"bar\"}"));
+
+        indexRandom(true, builders);
+
+        String query = "{ \"query\" : { \"match_all\": {}} , \"script_fields\" : { \"test1\" : { \"script_file\" : \"script1\" }, \"test2\" : { \"script_file\" : \"script1\", \"lang\":\"expression\"  }}, size:1}";
+        SearchResponse searchResponse = client().prepareSearch().setSource(query).setIndices("test").setTypes("scriptTest").get();
+        assertHitCount(searchResponse, 5);
+        assertTrue(searchResponse.getHits().hits().length == 1);
+        SearchHit sh = searchResponse.getHits().getAt(0);
+        assertThat((Integer)sh.field("test1").getValue(), equalTo(2));
+        assertThat((Double)sh.field("test2").getValue(), equalTo(10d));
+    }
 }

src/test/java/org/elasticsearch/script/ScriptServiceTests.java

@@ -18,16 +18,19 @@
  */
 package org.elasticsearch.script;
 
-import com.carrotsearch.ant.tasks.junit4.dependencies.com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSet;
 import org.elasticsearch.ElasticsearchIllegalArgumentException;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.io.Streams;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.node.settings.NodeSettingsService;
+import org.elasticsearch.script.expression.ExpressionScriptEngineService;
+import org.elasticsearch.script.groovy.GroovyScriptEngineService;
 import org.elasticsearch.search.lookup.SearchLookup;
 import org.elasticsearch.test.ElasticsearchTestCase;
 import org.elasticsearch.watcher.ResourceWatcherService;
+import org.junit.Before;
 import org.junit.Test;
 
 import java.io.IOException;
@@ -36,37 +39,42 @@ import java.nio.file.Path;
 import java.util.Map;
 
 import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
-import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.*;
-import static org.hamcrest.Matchers.equalTo;
 
 /**
  *
  */
 public class ScriptServiceTests extends ElasticsearchTestCase {
 
-    @Test
+    private ResourceWatcherService resourceWatcherService;
-    public void testScriptsWithoutExtensions() throws IOException {
+    private ScriptService scriptService;
-        Path homeFolder = newTempDirPath();
+    private Path scriptsFilePath;
+
+    @Before
+    public void setup() throws IOException {
         Path genericConfigFolder = newTempDirPath();
 
         Settings settings = settingsBuilder()
                 .put("path.conf", genericConfigFolder)
-                .put("path.home", homeFolder)
                 .build();
         Environment environment = new Environment(settings);
 
-        ResourceWatcherService resourceWatcherService = new ResourceWatcherService(settings, null);
+        resourceWatcherService = new ResourceWatcherService(settings, null);
 
         logger.info("--> setup script service");
-        ScriptService scriptService = new ScriptService(settings, environment,
+        scriptService = new ScriptService(settings, environment,
-                ImmutableSet.of(new TestEngineService()), resourceWatcherService, new NodeSettingsService(settings));
+                ImmutableSet.of(new TestEngineService(), new GroovyScriptEngineService(settings), new ExpressionScriptEngineService(settings)),
-        Path scriptsFile = genericConfigFolder.resolve("scripts");
+                resourceWatcherService, new NodeSettingsService(settings));
-        Files.createDirectories(scriptsFile);
+        scriptsFilePath = genericConfigFolder.resolve("scripts");
-        resourceWatcherService.notifyNow();
+        Files.createDirectories(scriptsFilePath);
+    }
+
+    @Test
+    public void testScriptsWithoutExtensions() throws IOException {
 
         logger.info("--> setup two test files one with extension and another without");
-        Path testFileNoExt = scriptsFile.resolve("test_no_ext");
+        Path testFileNoExt = scriptsFilePath.resolve("test_no_ext");
-        Path testFileWithExt = scriptsFile.resolve("test_script.tst");
+        Path testFileWithExt = scriptsFilePath.resolve("test_script.tst");
         Streams.copy("test_file_no_ext".getBytes("UTF-8"), Files.newOutputStream(testFileNoExt));
         Streams.copy("test_file".getBytes("UTF-8"), Files.newOutputStream(testFileWithExt));
         resourceWatcherService.notifyNow();
@@ -89,11 +97,43 @@ public class ScriptServiceTests extends ElasticsearchTestCase {
         }
     }
 
+    @Test
+    public void testScriptsSameNameDifferentLanguage() throws IOException {
+        Path testFileNoExt = scriptsFilePath.resolve("script.groovy");
+        Path testFileWithExt = scriptsFilePath.resolve("script.expression");
+        Streams.copy("10".getBytes("UTF-8"), Files.newOutputStream(testFileNoExt));
+        Streams.copy("20".getBytes("UTF-8"), Files.newOutputStream(testFileWithExt));
+        resourceWatcherService.notifyNow();
+
+        CompiledScript groovyScript = scriptService.compile(GroovyScriptEngineService.NAME, "script", ScriptService.ScriptType.FILE);
+        assertThat(groovyScript.lang(), equalTo(GroovyScriptEngineService.NAME));
+        CompiledScript expressionScript = scriptService.compile(ExpressionScriptEngineService.NAME, "script", ScriptService.ScriptType.FILE);
+        assertThat(expressionScript.lang(), equalTo(ExpressionScriptEngineService.NAME));
+    }
+
+    @Test
+    public void testInlineScriptCompiledOnceMultipleLangAcronyms() throws IOException {
+        CompiledScript compiledScript1 = scriptService.compile("test", "test_script", ScriptService.ScriptType.INLINE);
+        CompiledScript compiledScript2 = scriptService.compile("test2", "test_script", ScriptService.ScriptType.INLINE);
+        assertThat(compiledScript1, sameInstance(compiledScript2));
+    }
+
+    @Test
+    public void testFileScriptCompiledOnceMultipleLangAcronyms() throws IOException {
+        Path testFileWithExt = scriptsFilePath.resolve("test_script.tst");
+        Streams.copy("test_file".getBytes("UTF-8"), Files.newOutputStream(testFileWithExt));
+        resourceWatcherService.notifyNow();
+
+        CompiledScript compiledScript1 = scriptService.compile("test", "test_script", ScriptService.ScriptType.FILE);
+        CompiledScript compiledScript2 = scriptService.compile("test2", "test_script", ScriptService.ScriptType.FILE);
+        assertThat(compiledScript1, sameInstance(compiledScript2));
+    }
+
     public static class TestEngineService implements ScriptEngineService {
 
         @Override
         public String[] types() {
-            return new String[] {"test"};
+            return new String[] {"test", "test2"};
         }
 
         @Override
@@ -103,7 +143,7 @@ public class ScriptServiceTests extends ElasticsearchTestCase {
 
         @Override
         public boolean sandboxed() {
-            return false;
+            return true;
         }
 
         @Override

src/test/resources/org/elasticsearch/script/config/scripts/script1.expression

@@ -0,0 +1 @@
+10