From e0ec9780a5dc61db2f6482f3ea41f83065920f81 Mon Sep 17 00:00:00 2001 From: Matthew Burgess Date: Tue, 6 Sep 2022 17:33:12 -0400 Subject: [PATCH] NIFI-10449 Fixed ScriptedLookupServices reloading - Reload script after disabled and first validation after changes made This closes #6371 Signed-off-by: David Handermann --- .../util/StandardProcessorTestRunner.java | 5 +- .../script/BaseScriptedLookupService.java | 97 ++++++++++++++++++- .../nifi/script/ScriptingComponentHelper.java | 2 +- .../script/TestScriptedLookupService.groovy | 25 +++-- .../groovy/test_simple_lookup_inline.groovy | 4 +- 5 files changed, 114 insertions(+), 19 deletions(-) diff --git a/nifi-mock/src/main/java/org/apache/nifi/util/StandardProcessorTestRunner.java b/nifi-mock/src/main/java/org/apache/nifi/util/StandardProcessorTestRunner.java index 0aca359bdb..b2b0557dad 100644 --- a/nifi-mock/src/main/java/org/apache/nifi/util/StandardProcessorTestRunner.java +++ b/nifi-mock/src/main/java/org/apache/nifi/util/StandardProcessorTestRunner.java @@ -721,7 +721,10 @@ public class StandardProcessorTestRunner implements TestRunner { } try { - ReflectionUtils.invokeMethodsWithAnnotation(OnDisabled.class, service); + // Create a config context to pass into the controller service's OnDisabled method (it will be ignored if the controller service has no arguments) + final MockConfigurationContext configContext = new MockConfigurationContext(service, configuration.getProperties(), context, variableRegistry); + configContext.setValidateExpressions(validateExpressionUsage); + ReflectionUtils.invokeMethodsWithAnnotation(OnDisabled.class, service, configContext); } catch (final Exception e) { e.printStackTrace(); Assertions.fail("Failed to disable Controller Service " + service + " due to " + e); 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 3c34af2ad8..709a93f725 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 @@ -21,7 +21,9 @@ import org.apache.nifi.annotation.lifecycle.OnEnabled; import org.apache.nifi.components.AllowableValue; import org.apache.nifi.components.ConfigurableComponent; import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.components.ValidationContext; import org.apache.nifi.components.ValidationResult; +import org.apache.nifi.components.resource.ResourceReferences; import org.apache.nifi.components.state.StateManager; import org.apache.nifi.controller.ConfigurationContext; import org.apache.nifi.controller.ControllerServiceInitializationContext; @@ -35,6 +37,7 @@ import org.apache.nifi.processor.util.StandardValidators; import org.apache.nifi.script.AbstractScriptedControllerService; import org.apache.nifi.script.ScriptingComponentHelper; import org.apache.nifi.script.ScriptingComponentUtils; +import org.apache.nifi.script.impl.FilteredPropertiesValidationContextAdapter; import javax.script.Invocable; import javax.script.ScriptContext; @@ -46,6 +49,7 @@ import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; @@ -105,7 +109,7 @@ public class BaseScriptedLookupService extends AbstractScriptedControllerService } } catch (final Throwable t) { final ComponentLog logger = getLogger(); - final String message = "Unable to get property descriptors from Processor: " + t; + final String message = "Unable to get property descriptors from LookupService: " + t; logger.error(message); if (logger.isDebugEnabled()) { @@ -148,16 +152,29 @@ public class BaseScriptedLookupService extends AbstractScriptedControllerService */ @Override public void onPropertyModified(final PropertyDescriptor descriptor, final String oldValue, final String newValue) { + validationResults.set(new HashSet<>()); + final ComponentLog logger = getLogger(); - final ConfigurableComponent instance = lookupService.get(); + final LookupService instance = lookupService.get(); if (ScriptingComponentUtils.SCRIPT_FILE.equals(descriptor) || ScriptingComponentUtils.SCRIPT_BODY.equals(descriptor) || ScriptingComponentUtils.MODULES.equals(descriptor) || scriptingComponentHelper.SCRIPT_ENGINE.equals(descriptor)) { + + // Update the ScriptingComponentHelper's value(s) + if (ScriptingComponentUtils.SCRIPT_FILE.equals(descriptor)) { + scriptingComponentHelper.setScriptPath(newValue); + } else if (ScriptingComponentUtils.SCRIPT_BODY.equals(descriptor)) { + scriptingComponentHelper.setScriptBody(newValue); + } else if (scriptingComponentHelper.SCRIPT_ENGINE.equals(descriptor)) { + scriptingComponentHelper.setScriptEngineName(newValue); + } + scriptNeedsReload.set(true); + scriptRunner = null; //reset engine. This happens only when the controller service is disabled, so there won't be any performance impact in run-time. } else if (instance != null) { - // If the script provides a ConfigurableComponent, call its onPropertyModified() method + // If the script provides a LookupService, call its onPropertyModified() method try { instance.onPropertyModified(descriptor, oldValue, newValue); } catch (final Exception e) { @@ -167,6 +184,71 @@ public class BaseScriptedLookupService extends AbstractScriptedControllerService } } + @Override + protected Collection customValidate(ValidationContext context) { + Collection commonValidationResults = super.customValidate(context); + if (!commonValidationResults.isEmpty()) { + return commonValidationResults; + } + + // do not try to build processor/compile/etc until onPropertyModified clear the validation error/s + // and don't print anything into log. + if (!validationResults.get().isEmpty()) { + 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()); + final ResourceReferences resourceReferences = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources(); + scriptingComponentHelper.setModules(resourceReferences); + setup(); + + // Now that the component is validated, we can call validate on the scripted lookup service + final LookupService instance = lookupService.get(); + final Collection currentValidationResults = validationResults.get(); + + // if there was existing validation errors and the processor loaded successfully + if (currentValidationResults.isEmpty() && instance != null) { + try { + // defer to the underlying controller service for validation, without the + // lookup service's properties + final Set innerPropertyDescriptor = new HashSet<>(scriptingComponentHelper.getDescriptors()); + + ValidationContext innerValidationContext = new FilteredPropertiesValidationContextAdapter(context, innerPropertyDescriptor); + final Collection instanceResults = instance.validate(innerValidationContext); + + if (instanceResults != null && instanceResults.size() > 0) { + // return the validation results from the underlying instance + return instanceResults; + } + } catch (final Exception e) { + final ComponentLog logger = getLogger(); + final String message = "Unable to validate the scripted LookupService: " + e; + logger.error(message, e); + + // return a new validation message + final Collection results = new HashSet<>(); + results.add(new ValidationResult.Builder() + .subject("Validation") + .valid(false) + .explanation("An error occurred calling validate in the configured scripted LookupService.") + .input(context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).getValue()) + .build()); + return results; + } + } + + return currentValidationResults; + + } + @Override @OnEnabled public void onEnabled(final ConfigurationContext context) { @@ -226,7 +308,8 @@ public class BaseScriptedLookupService extends AbstractScriptedControllerService } } } else { - throw new ScriptException("No LookupService was defined by the script."); + // This might be due to an error during compilation, log it rather than throwing an exception + getLogger().warn("No LookupService was defined by the script."); } } catch (ScriptException se) { throw new ProcessException("Error executing onDisabled(context) method", se); @@ -235,6 +318,10 @@ public class BaseScriptedLookupService extends AbstractScriptedControllerService } else { throw new ProcessException("Error creating ScriptRunner"); } + + scriptingComponentHelper.stop(); + lookupService.set(null); + scriptRunner = null; } @Override @@ -262,7 +349,7 @@ public class BaseScriptedLookupService extends AbstractScriptedControllerService final Collection results = new HashSet<>(); try { - // Create a single script engine, the Processor object is reused by each task + // Create a single script engine, the LookupService object is reused by each task if (scriptRunner == null) { scriptingComponentHelper.setupScriptRunners(1, scriptBody, getLogger()); scriptRunner = scriptingComponentHelper.scriptRunnerQ.poll(); diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java index b7e5e1730e..a485cd640b 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java @@ -236,7 +236,7 @@ public class ScriptingComponentHelper { } // Get a list of URLs from the configurator (if present), or just convert modules from Strings to URLs - final String[] locations = modules.asLocations().toArray(new String[0]); + final String[] locations = (modules == null) ? new String[0] : modules.asLocations().toArray(new String[0]); final URL[] additionalClasspathURLs = ScriptRunnerFactory.getInstance().getModuleURLsForClasspath(scriptEngineName, locations, log); diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/groovy/org/apache/nifi/lookup/script/TestScriptedLookupService.groovy b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/groovy/org/apache/nifi/lookup/script/TestScriptedLookupService.groovy index 01f4454032..c2a4c5b252 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/groovy/org/apache/nifi/lookup/script/TestScriptedLookupService.groovy +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/groovy/org/apache/nifi/lookup/script/TestScriptedLookupService.groovy @@ -112,19 +112,10 @@ class TestScriptedLookupService { runner.addControllerService("lookupService", scriptedLookupService) runner.setProperty(scriptedLookupService, "Script Engine", "Groovy") runner.setProperty(scriptedLookupService, ScriptingComponentUtils.SCRIPT_BODY, (String) null) - runner.setProperty(scriptedLookupService, ScriptingComponentUtils.SCRIPT_FILE, ALTERNATE_TARGET_PATH.toString()) + runner.setProperty(scriptedLookupService, ScriptingComponentUtils.SCRIPT_FILE, TARGET_PATH.toString()) runner.setProperty(scriptedLookupService, ScriptingComponentUtils.MODULES, (String) null) - // This call to setup should fail loading the script but should mark that the script should be reloaded - scriptedLookupService.setup() - // This prevents the (lookupService == null) check from passing, in order to force the reload from the - // scriptNeedsReload variable specifically - scriptedLookupService.lookupService.set(new MockScriptedLookupService()) - runner.enableControllerService(scriptedLookupService) - MockFlowFile mockFlowFile = new MockFlowFile(1L) - InputStream inStream = new ByteArrayInputStream('Flow file content not used'.bytes) - Optional opt = scriptedLookupService.lookup(['key':'Hello']) assertTrue(opt.present) assertEquals('Hi', opt.get()) @@ -133,6 +124,20 @@ class TestScriptedLookupService { assertEquals('there', opt.get()) opt = scriptedLookupService.lookup(['key':'Not There']) assertFalse(opt.present) + + // Disable and load different script + runner.disableControllerService(scriptedLookupService) + runner.setProperty(scriptedLookupService, ScriptingComponentUtils.SCRIPT_FILE, ALTERNATE_TARGET_PATH.toString()) + runner.enableControllerService(scriptedLookupService) + + opt = scriptedLookupService.lookup(['key':'Hello']) + assertTrue(opt.present) + assertEquals('Goodbye', opt.get()) + opt = scriptedLookupService.lookup(['key':'World']) + assertTrue(opt.present) + assertEquals('Stranger', opt.get()) + opt = scriptedLookupService.lookup(['key':'Not There']) + assertFalse(opt.present) } class MockScriptedLookupService extends ScriptedLookupService implements AccessibleScriptingComponentHelper { diff --git a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/resources/groovy/test_simple_lookup_inline.groovy b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/resources/groovy/test_simple_lookup_inline.groovy index ca10baab3f..81792672f7 100644 --- a/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/resources/groovy/test_simple_lookup_inline.groovy +++ b/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/resources/groovy/test_simple_lookup_inline.groovy @@ -24,8 +24,8 @@ import org.apache.nifi.reporting.InitializationException class SimpleGroovyLookupService implements StringLookupService { def lookupTable = [ - 'Hello': 'Hi', - 'World': 'there' + 'Hello': 'Goodbye', + 'World': 'Stranger' ]