From 632fbcca7d7e028c4526ab2b643c6b83cfadef99 Mon Sep 17 00:00:00 2001 From: Matthew Burgess Date: Thu, 24 Jun 2021 11:09:58 -0400 Subject: [PATCH] NIFI-8730: Invalidate (don't evaluate) empty script in scripting components Signed-off-by: Pierre Villard This closes #5184. --- .../script/BaseScriptedLookupService.java | 5 ++- .../script/InvokeScriptedProcessor.java | 31 +++++++++++++------ .../script/ScriptedTransformRecord.java | 2 +- .../sink/script/ScriptedRecordSink.java | 5 ++- .../engine/script/ScriptedRulesEngine.java | 5 ++- .../script/ScriptedActionHandler.java | 5 ++- .../AbstractScriptedControllerService.java | 4 +-- .../script/TestInvokeJavascript.java | 15 +++++++-- .../test/resources/groovy/test_reader.groovy | 2 +- 9 files changed, 45 insertions(+), 29 deletions(-) diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/lookup/script/BaseScriptedLookupService.java b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/lookup/script/BaseScriptedLookupService.java index 9c7a4b4bbe..623796eb4b 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/lookup/script/BaseScriptedLookupService.java +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/lookup/script/BaseScriptedLookupService.java @@ -241,11 +241,10 @@ public class BaseScriptedLookupService extends AbstractScriptedControllerService public void setup() { if (scriptNeedsReload.get() || lookupService.get() == null) { if (ScriptingComponentHelper.isFile(scriptingComponentHelper.getScriptPath())) { - reloadScriptFile(scriptingComponentHelper.getScriptPath()); + scriptNeedsReload.set(reloadScriptFile(scriptingComponentHelper.getScriptPath())); } else { - reloadScriptBody(scriptingComponentHelper.getScriptBody()); + scriptNeedsReload.set(reloadScriptBody(scriptingComponentHelper.getScriptBody())); } - scriptNeedsReload.set(false); } } diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/InvokeScriptedProcessor.java b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/InvokeScriptedProcessor.java index 681c798afe..48f12ce9d2 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/InvokeScriptedProcessor.java +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/InvokeScriptedProcessor.java @@ -215,11 +215,10 @@ public class InvokeScriptedProcessor extends AbstractSessionFactoryProcessor { public void setup() { if (scriptNeedsReload.get() || processor.get() == null) { if (ScriptingComponentHelper.isFile(scriptingComponentHelper.getScriptPath())) { - reloadScriptFile(scriptingComponentHelper.getScriptPath()); + scriptNeedsReload.set(reloadScriptFile(scriptingComponentHelper.getScriptPath())); } else { - reloadScriptBody(scriptingComponentHelper.getScriptBody()); + scriptNeedsReload.set(reloadScriptBody(scriptingComponentHelper.getScriptBody())); } - scriptNeedsReload.set(false); } } @@ -326,6 +325,10 @@ public class InvokeScriptedProcessor extends AbstractSessionFactoryProcessor { * @return Whether the script was successfully reloaded */ private boolean reloadScript(final String scriptBody) { + if (StringUtils.isEmpty(scriptBody)) { + return true; + } + // note we are starting here with a fresh listing of validation // results since we are (re)loading a new/updated script. any // existing validation results are not relevant @@ -431,15 +434,14 @@ public class InvokeScriptedProcessor extends AbstractSessionFactoryProcessor { // store the updated validation results validationResults.set(results); - // return whether there was any issues loading the configured script - return results.isEmpty(); + // return whether there were any issues loading the configured script + return !results.isEmpty(); } /** * Invokes the validate() routine provided by the script, allowing for - * custom validation code. This method assumes there is a valid Processor - * defined in the script and it has been loaded by the - * InvokeScriptedProcessor processor + * custom validation code, if there is a valid Processor + * defined in the script and it has been loaded by the processor * * @param context The validation context to be passed into the custom * validate method @@ -460,6 +462,12 @@ public class InvokeScriptedProcessor extends AbstractSessionFactoryProcessor { return validationResults.get(); } + Collection scriptingComponentHelperResults = scriptingComponentHelper.customValidate(context); + if (scriptingComponentHelperResults != null && !scriptingComponentHelperResults.isEmpty()) { + validationResults.set(scriptingComponentHelperResults); + return scriptingComponentHelperResults; + } + scriptingComponentHelper.setScriptEngineName(context.getProperty(scriptingComponentHelper.SCRIPT_ENGINE).getValue()); scriptingComponentHelper.setScriptPath(context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).evaluateAttributeExpressions().getValue()); scriptingComponentHelper.setScriptBody(context.getProperty(ScriptingComponentUtils.SCRIPT_BODY).getValue()); @@ -476,7 +484,7 @@ public class InvokeScriptedProcessor extends AbstractSessionFactoryProcessor { try { // defer to the underlying processor for validation, without the // invokescriptedprocessor properties - final Set innerPropertyDescriptor = new HashSet(scriptingComponentHelper.getDescriptors()); + final Set innerPropertyDescriptor = new HashSet<>(scriptingComponentHelper.getDescriptors()); ValidationContext innerValidationContext = new FilteredPropertiesValidationContextAdapter(context, innerPropertyDescriptor); final Collection instanceResults = instance.validate(innerValidationContext); @@ -568,7 +576,10 @@ public class InvokeScriptedProcessor extends AbstractSessionFactoryProcessor { @OnStopped public void stop(ProcessContext context) { - invokeScriptedProcessorMethod("onStopped", context); + // If the script needs to be reloaded at this point, it is because it was empty + if (!scriptNeedsReload.get()) { + invokeScriptedProcessorMethod("onStopped", context); + } scriptingComponentHelper.stop(); processor.set(null); scriptRunner = null; diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/ScriptedTransformRecord.java b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/ScriptedTransformRecord.java index efe54de175..370f3f2089 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/ScriptedTransformRecord.java +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/ScriptedTransformRecord.java @@ -168,7 +168,7 @@ public class ScriptedTransformRecord extends AbstractProcessor implements Search @OnScheduled public void setup(final ProcessContext context) throws IOException { if (!scriptingComponentHelper.isInitialized.get()) { - scriptingComponentHelper.createResources(); + scriptingComponentHelper.createResources(false); } scriptingComponentHelper.setupVariables(context); diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/record/sink/script/ScriptedRecordSink.java b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/record/sink/script/ScriptedRecordSink.java index 652965fd65..f17cb0e95f 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/record/sink/script/ScriptedRecordSink.java +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/record/sink/script/ScriptedRecordSink.java @@ -107,11 +107,10 @@ public class ScriptedRecordSink extends AbstractScriptedControllerService implem public void setup() { if (scriptNeedsReload.get() || recordSink.get() == null) { if (ScriptingComponentHelper.isFile(scriptingComponentHelper.getScriptPath())) { - reloadScriptFile(scriptingComponentHelper.getScriptPath()); + scriptNeedsReload.set(reloadScriptFile(scriptingComponentHelper.getScriptPath())); } else { - reloadScriptBody(scriptingComponentHelper.getScriptBody()); + scriptNeedsReload.set(reloadScriptBody(scriptingComponentHelper.getScriptBody())); } - scriptNeedsReload.set(false); } } diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/rules/engine/script/ScriptedRulesEngine.java b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/rules/engine/script/ScriptedRulesEngine.java index 7d09509bed..5239761c25 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/rules/engine/script/ScriptedRulesEngine.java +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/rules/engine/script/ScriptedRulesEngine.java @@ -78,11 +78,10 @@ public class ScriptedRulesEngine extends AbstractScriptedControllerService imple public void setup() { if (scriptNeedsReload.get() || rulesEngine.get() == null) { if (ScriptingComponentHelper.isFile(scriptingComponentHelper.getScriptPath())) { - reloadScriptFile(scriptingComponentHelper.getScriptPath()); + scriptNeedsReload.set(reloadScriptFile(scriptingComponentHelper.getScriptPath())); } else { - reloadScriptBody(scriptingComponentHelper.getScriptBody()); + scriptNeedsReload.set(reloadScriptBody(scriptingComponentHelper.getScriptBody())); } - scriptNeedsReload.set(false); } } diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/rules/handlers/script/ScriptedActionHandler.java b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/rules/handlers/script/ScriptedActionHandler.java index 2b48bf624c..f81ade4ffb 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/rules/handlers/script/ScriptedActionHandler.java +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/rules/handlers/script/ScriptedActionHandler.java @@ -80,11 +80,10 @@ public class ScriptedActionHandler extends AbstractScriptedControllerService imp public void setup() { if (scriptNeedsReload.get() || actionHandler.get() == null) { if (ScriptingComponentHelper.isFile(scriptingComponentHelper.getScriptPath())) { - reloadScriptFile(scriptingComponentHelper.getScriptPath()); + scriptNeedsReload.set(reloadScriptFile(scriptingComponentHelper.getScriptPath())); } else { - reloadScriptBody(scriptingComponentHelper.getScriptBody()); + scriptNeedsReload.set(reloadScriptBody(scriptingComponentHelper.getScriptBody())); } - scriptNeedsReload.set(false); } } diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/AbstractScriptedControllerService.java b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/AbstractScriptedControllerService.java index 35ea374482..8c8a391183 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/AbstractScriptedControllerService.java +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/AbstractScriptedControllerService.java @@ -119,7 +119,7 @@ public abstract class AbstractScriptedControllerService extends AbstractControll @Override protected Collection customValidate(ValidationContext validationContext) { - Collection commonValidationResults = super.customValidate(validationContext); + Collection commonValidationResults = new ArrayList<>(super.customValidate(validationContext)); commonValidationResults.addAll(scriptingComponentHelper.customValidate(validationContext)); if (!commonValidationResults.isEmpty()) { @@ -173,7 +173,7 @@ public abstract class AbstractScriptedControllerService extends AbstractControll validationResults.set(results); // return whether there was any issues loading the configured script - return results.isEmpty(); + return !results.isEmpty(); } /** diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestInvokeJavascript.java b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestInvokeJavascript.java index f6500c6205..9118f5a2e6 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestInvokeJavascript.java +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestInvokeJavascript.java @@ -52,7 +52,6 @@ public class TestInvokeJavascript extends BaseScriptTest { */ @Test public void testReadFlowFileContentAndStoreInFlowFileAttribute() throws Exception { - runner.setValidateExpressionUsage(false); runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "ECMAScript"); runner.setProperty(ScriptingComponentUtils.SCRIPT_FILE, "target/test/resources/javascript/test_reader.js"); runner.setProperty(ScriptingComponentUtils.MODULES, "target/test/resources/javascript"); @@ -146,7 +145,6 @@ public class TestInvokeJavascript extends BaseScriptTest { @Test(expected = AssertionError.class) public void testInvokeScriptCausesException() throws Exception { final TestRunner runner = TestRunners.newTestRunner(new InvokeScriptedProcessor()); - runner.setValidateExpressionUsage(false); runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "ECMAScript"); runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY, getFileContentsAsString( TEST_RESOURCE_LOCATION + "javascript/testInvokeScriptCausesException.js") @@ -164,7 +162,6 @@ public class TestInvokeJavascript extends BaseScriptTest { */ @Test public void testScriptRoutesToFailure() throws Exception { - runner.setValidateExpressionUsage(false); runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "ECMAScript"); runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY, getFileContentsAsString( TEST_RESOURCE_LOCATION + "javascript/testScriptRoutesToFailure.js") @@ -177,4 +174,16 @@ public class TestInvokeJavascript extends BaseScriptTest { final List result = runner.getFlowFilesForRelationship("FAILURE"); assertFalse(result.isEmpty()); } + + /** + * Tests an empty script with Nashorn (which throws an NPE if it is loaded), this test verifies an empty script is not attempted to be loaded. + * + * @throws Exception Any error encountered while testing + */ + @Test + public void testEmptyScript() throws Exception { + runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "ECMAScript"); + runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY, ""); + runner.assertNotValid(); + } } diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/resources/groovy/test_reader.groovy b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/resources/groovy/test_reader.groovy index 03395a2c8f..6e3dfb1867 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/resources/groovy/test_reader.groovy +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/resources/groovy/test_reader.groovy @@ -57,7 +57,7 @@ class GroovyProcessor implements Processor { @Override void onTrigger(ProcessContext context, ProcessSessionFactory sessionFactory) throws ProcessException { def session = sessionFactory.createSession() - def flowFile = session.get(); + def flowFile = session.get() if (flowFile == null) { return }