From be06fba821d20ce516ceb258948e4972cd771520 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Sun, 15 Jul 2018 08:45:41 +0200 Subject: [PATCH] Cleanup Duplication in `PainlessScriptEngine` (#31991) (#32061) * Cleanup Duplication in `PainlessScriptEngine` * Extract duplicate building of compiler settings to method * Remove dead method params + dead constant in `ScriptProcessor` --- .../painless/PainlessScriptEngine.java | 85 ++++++------------- 1 file changed, 26 insertions(+), 59 deletions(-) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java index ae1944c9bd3a9..4560fd85a6589 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java @@ -366,44 +366,7 @@ private void writeNeedsMethods(Class clazz, ClassWriter writer, MainMethodRes } Object compile(Compiler compiler, String scriptName, String source, Map params, Object... args) { - final CompilerSettings compilerSettings; - - if (params.isEmpty()) { - // Use the default settings. - compilerSettings = defaultCompilerSettings; - } else { - // Use custom settings specified by params. - compilerSettings = new CompilerSettings(); - - // Except regexes enabled - this is a node level setting and can't be changed in the request. - compilerSettings.setRegexesEnabled(defaultCompilerSettings.areRegexesEnabled()); - - Map copy = new HashMap<>(params); - - String value = copy.remove(CompilerSettings.MAX_LOOP_COUNTER); - if (value != null) { - compilerSettings.setMaxLoopCounter(Integer.parseInt(value)); - } - - value = copy.remove(CompilerSettings.PICKY); - if (value != null) { - compilerSettings.setPicky(Boolean.parseBoolean(value)); - } - - value = copy.remove(CompilerSettings.INITIAL_CALL_SITE_DEPTH); - if (value != null) { - compilerSettings.setInitialCallSiteDepth(Integer.parseInt(value)); - } - - value = copy.remove(CompilerSettings.REGEX_ENABLED.getKey()); - if (value != null) { - throw new IllegalArgumentException("[painless.regex.enabled] can only be set on node startup."); - } - - if (!copy.isEmpty()) { - throw new IllegalArgumentException("Unrecognized compile-time parameter(s): " + copy); - } - } + final CompilerSettings compilerSettings = buildCompilerSettings(params); // Check we ourselves are not being called by unprivileged code. SpecialPermission.check(); @@ -434,14 +397,33 @@ public Object run() { }, COMPILATION_CONTEXT); // Note that it is safe to catch any of the following errors since Painless is stateless. } catch (OutOfMemoryError | StackOverflowError | VerifyError | Exception e) { - throw convertToScriptException(scriptName == null ? source : scriptName, source, e); + throw convertToScriptException(source, e); } } void compile(Compiler compiler, Loader loader, MainMethodReserved reserved, String scriptName, String source, Map params) { - final CompilerSettings compilerSettings; + final CompilerSettings compilerSettings = buildCompilerSettings(params); + + try { + // Drop all permissions to actually compile the code itself. + AccessController.doPrivileged(new PrivilegedAction() { + @Override + public Void run() { + String name = scriptName == null ? source : scriptName; + compiler.compile(loader, reserved, name, source, compilerSettings); + + return null; + } + }, COMPILATION_CONTEXT); + // Note that it is safe to catch any of the following errors since Painless is stateless. + } catch (OutOfMemoryError | StackOverflowError | VerifyError | Exception e) { + throw convertToScriptException(source, e); + } + } + private CompilerSettings buildCompilerSettings(Map params) { + CompilerSettings compilerSettings; if (params.isEmpty()) { // Use the default settings. compilerSettings = defaultCompilerSettings; @@ -478,25 +460,10 @@ void compile(Compiler compiler, Loader loader, MainMethodReserved reserved, throw new IllegalArgumentException("Unrecognized compile-time parameter(s): " + copy); } } - - try { - // Drop all permissions to actually compile the code itself. - AccessController.doPrivileged(new PrivilegedAction() { - @Override - public Void run() { - String name = scriptName == null ? source : scriptName; - compiler.compile(loader, reserved, name, source, compilerSettings); - - return null; - } - }, COMPILATION_CONTEXT); - // Note that it is safe to catch any of the following errors since Painless is stateless. - } catch (OutOfMemoryError | StackOverflowError | VerifyError | Exception e) { - throw convertToScriptException(scriptName == null ? source : scriptName, source, e); - } + return compilerSettings; } - private ScriptException convertToScriptException(String scriptName, String scriptSource, Throwable t) { + private ScriptException convertToScriptException(String scriptSource, Throwable t) { // create a script stack: this is just the script portion List scriptStack = new ArrayList<>(); for (StackTraceElement element : t.getStackTrace()) { @@ -507,7 +474,7 @@ private ScriptException convertToScriptException(String scriptName, String scrip scriptStack.add("<<< unknown portion of script >>>"); } else { offset--; // offset is 1 based, line numbers must be! - int startOffset = getPreviousStatement(scriptSource, offset); + int startOffset = getPreviousStatement(offset); int endOffset = getNextStatement(scriptSource, offset); StringBuilder snippet = new StringBuilder(); if (startOffset > 0) { @@ -535,7 +502,7 @@ private ScriptException convertToScriptException(String scriptName, String scrip } // very simple heuristic: +/- 25 chars. can be improved later. - private int getPreviousStatement(String scriptSource, int offset) { + private int getPreviousStatement(int offset) { return Math.max(0, offset - 25); }