gresockj commented on a change in pull request #5288:
URL: https://github.com/apache/nifi/pull/5288#discussion_r685219160
##########
File path:
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/s3/AbstractS3Processor.java
##########
@@ -333,4 +338,47 @@ protected final CannedAccessControlList
createCannedACL(final ProcessContext con
return cannedAcl;
}
+
+ @Override
+ public List<ConfigVerificationResult> verify(final ProcessContext context,
final ComponentLog logger, final Map<String, String> attributes) {
+ final AmazonS3Client client = createClient(context,
getCredentials(context), createConfiguration(context));
+ initializeRegionAndEndpoint(context, client);
+
+ final List<ConfigVerificationResult> results = new ArrayList<>();
+ final String bucketName =
context.getProperty(BUCKET).evaluateAttributeExpressions(attributes).getValue();
+
+ if (bucketName == null || bucketName.trim().isEmpty()) {
+ results.add(new ConfigVerificationResult.Builder()
+ .verificationStepName("Perform Listing")
+ .outcome(Outcome.FAILED)
+ .explanation("Bucket Name must be specified")
+ .build());
+
+ return results;
+ }
+
+ // Attempt to perform a listing of objects in the S3 bucket
+ try {
+ final ObjectListing listing = client.listObjects(bucketName);
Review comment:
Good thought, but I don't think we can have this check be
`listObjects(bucketName)` in case there are a lot of objects, and because not
all S3 processors should require the `s3:ListBucket` permission.
In fact, I propose you add an abstract method like `ConfigVerificationResult
verifyAccess(AmazonS3Client client)` to specifically check the permission
required by that processor. That way, ListS3 can check `listObjects()` just to
verify that the operation can be performed.
Also, I'd recommend using `listObjects(bucketName, "prefixthatdoesntexist")`
or some variant, so as not to actually list the entire bucket, since this will
still check if the configured account has access to that operation. I don't
think the bucket count is necessary for verification.
##########
File path:
nifi-api/src/main/java/org/apache/nifi/controller/VerifiableControllerService.java
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.controller;
+
+import org.apache.nifi.logging.ComponentLog;
+import org.apache.nifi.components.ConfigVerificationResult;
+
+import java.util.List;
+import java.util.Map;
+
+/**
+ * <p>
+ * Any Controller Service that implements this interface will be provided
the opportunity to verify
+ * a given configuration of the Controller Service. This allows the
Controller Service to provide meaningful feedback
+ * to users when configuring the dataflow.
+ * </p>
+ *
+ * <p>
+ * Generally speaking, verification differs from validation in that
validation is expected to be very
+ * quick and run often. If a Controller Service is not valid, it cannot be
started. However, verification may be
+ * more expensive or time-consuming to complete. For example, validation
may ensure that a username is
+ * provided for connecting to an external service but should not perform
any sort of network connection
+ * in order to verify that the username is accurate. Verification, on the
other hand, may create resources
+ * such as network connections, may be more expensive to complete, and may
be run only when a user invokes
+ * the action (though verification may later occur at other stages, such
as when starting a component).
+ * </p>
+ *
+ * <p>
+ * Verification is allowed to be run only when a Controller Service is
fully disabled.
+ * Therefore, any initialization logic that may need to be performed
+ * before the Controller Service is triggered may also be required for
verification. However, the framework is not responsible
+ * for triggering the Lifecycle management stages, such as @OnScheduled
before triggering the verification. Such
+ * methods should be handled by the {@link #verify(ConfigurationContext,
ComponentLog, Map)} itself.
+ * The {@link #verify(ConfigurationContext, ComponentLog, Map)} method
will only be called if the configuration is valid according to the
+ * validation rules (i.e., all Property Descriptors' validators and
customValidate methods have indicated that the configuration is valid).
+ * </p>
+ */
+public interface VerifiableControllerService {
+
+ /**
+ * Verifies that the configuration defined by the given ProcessContext is
valid.
Review comment:
Looks like a copy/paste error
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/service/StandardConfigurationContext.java
##########
@@ -74,9 +82,58 @@ public StandardConfigurationContext(final ComponentNode
component, final Control
}
}
+ public StandardConfigurationContext(final ComponentNode component, final
Map<String, String> propertyOverrides, final String annotationDataOverride,
final ParameterLookup parameterLookup,
+ final ControllerServiceLookup
serviceLookup, final String schedulingPeriod, final VariableRegistry
variableRegistry) {
+ this.component = component;
+ this.serviceLookup = serviceLookup;
+ this.schedulingPeriod = schedulingPeriod;
+ this.variableRegistry = variableRegistry;
+ this.annotationData = annotationDataOverride;
+
+ if (schedulingPeriod == null) {
+ schedulingNanos = null;
+ } else {
+ if
(FormatUtils.TIME_DURATION_PATTERN.matcher(schedulingPeriod).matches()) {
+ schedulingNanos =
FormatUtils.getTimeDuration(schedulingPeriod, TimeUnit.NANOSECONDS);
+ } else {
+ schedulingNanos = null;
+ }
+ }
+
Review comment:
It seems like we could reuse some of this by calling the original
constructor from here.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/reporting/AbstractReportingTaskNode.java
##########
@@ -315,4 +329,100 @@ public String getProcessGroupIdentifier() {
public ParameterLookup getParameterLookup() {
return ParameterLookup.EMPTY;
}
+
+ @Override
+ public void verifyCanPerformVerification() {
+ if (isRunning()) {
+ throw new IllegalStateException("Cannot perform verification
because Reporting Task is not fully stopped");
+ }
+ }
+
+ @Override
+ public List<ConfigVerificationResult> verifyConfiguration(final
ConfigurationContext context, final ComponentLog logger, final ExtensionManager
extensionManager) {
+ final List<ConfigVerificationResult> results = new ArrayList<>();
+
+ try {
+ verifyCanPerformVerification();
+
+ final long startNanos = System.nanoTime();
+
+ final Map<PropertyDescriptor, PropertyConfiguration>
descriptorToConfigMap = new LinkedHashMap<>();
+ for (final Map.Entry<PropertyDescriptor, String> entry :
context.getProperties().entrySet()) {
+ final PropertyDescriptor descriptor = entry.getKey();
+ final String rawValue = entry.getValue();
+ final String propertyValue = rawValue == null ?
descriptor.getDefaultValue() : rawValue;
+
+ final PropertyConfiguration propertyConfiguration = new
PropertyConfiguration(propertyValue, null, Collections.emptyList());
+ descriptorToConfigMap.put(descriptor, propertyConfiguration);
+ }
+
+ final ValidationContext validationContext =
getValidationContextFactory().newValidationContext(descriptorToConfigMap,
context.getAnnotationData(),
+ getProcessGroupIdentifier(), getIdentifier(), null, false);
+
+ final ValidationState validationState =
performValidation(validationContext);
+ final ValidationStatus validationStatus =
validationState.getStatus();
+
+ if (validationStatus == ValidationStatus.INVALID) {
+ for (final ValidationResult result :
validationState.getValidationErrors()) {
+ if (result.isValid()) {
+ continue;
+ }
+
+ results.add(new ConfigVerificationResult.Builder()
+ .outcome(Outcome.FAILED)
+ .explanation("Reporting Task is invalid: " +
result.toString())
+ .verificationStepName("Perform Validation")
+ .build());
+ }
+
+ if (results.isEmpty()) {
+ results.add(new ConfigVerificationResult.Builder()
+ .outcome(Outcome.FAILED)
+ .explanation("Reporting Task is invalid but provided
no Validation Results to indicate why")
+ .verificationStepName("Perform Validation")
+ .build());
+ }
+
+ logger.debug("{} is not valid with the given configuration.
Will not attempt to perform any additional verification of configuration.
Validation took {}. Reason not valid: {}",
Review comment:
Could we extract a method that does this initial validation check on the
context properties? Since both `ProcessContext` and `ConfigurationContext` can
return `Map<String, PropertyDescriptor>`, and this initial validation is needed
in `AbstractReportingTaskNode`, `StandardControllerServiceNode`, and
`StandardProcessorNode`, it seems like it would allow some pretty good reuse.
##########
File path:
nifi-api/src/main/java/org/apache/nifi/processor/VerifiableProcessor.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.processor;
+
+import org.apache.nifi.components.ConfigVerificationResult;
+import org.apache.nifi.logging.ComponentLog;
+
+import java.util.List;
+import java.util.Map;
+
+/**
+ * <p>
+ * Any Processor that implements this interface will be provided the
opportunity to verify
+ * a given configuration of the Processor. This allows the Processor to
provide meaningful feedback
+ * to users when configuring the dataflow.
+ * </p>
+ *
+ * <p>
+ * Generally speaking, verification differs from validation in that
validation is expected to be very
+ * quick and run often. If a Processor is not valid, it cannot be started.
However, verification may be
+ * more expensive or time-consuming to complete. For example, validation
may ensure that a username is
+ * provided for connecting to an external service but should not perform
any sort of network connection
+ * in order to verify that the username is accurate. Verification, on the
other hand, may create resources
+ * such as network connections, may be more expensive to complete, and may
be run only when a user invokes
+ * the action (though verification may later occur at other stages, such
as when starting a component).
+ * </p>
+ *
+ * <p>
+ * Verification is allowed to be run only when a Processor is fully
stopped. I.e., it has no active threads
+ * and currently has a state of STOPPED. Therefore, any initialization
logic that may need to be performed
+ * before the Processor is triggered may also be required for
verification. However, the framework is not responsible
+ * for triggering the Lifecycle management stages, such as @OnScheduled
before triggering the verification. Such
+ * methods should be handled by the {@link #verify(ProcessContext,
ComponentLog, Map)} itself.
+ * The {@link #verify(ProcessContext, ComponentLog, Map)} method will only
be called if the configuration is valid according to the
+ * validation rules (i.e., all Property Descriptors' validators and
customValidate methods have indicated that the configuration is valid).
+ * </p>
+ */
+public interface VerifiableProcessor {
+
+ /**
+ * Verifies that the configuration defined by the given ProcessContext is
valid.
+ * @param context the ProcessContext that contains the necessary
configuration
+ * @param verificationLogger a logger that can be used during
verification. While the typical logger can be used, doing so may result
+ * in producing bulletins, which can be confusing.
Review comment:
Missing a @param here
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/reporting/AbstractReportingTaskNode.java
##########
@@ -315,4 +329,100 @@ public String getProcessGroupIdentifier() {
public ParameterLookup getParameterLookup() {
return ParameterLookup.EMPTY;
}
+
+ @Override
+ public void verifyCanPerformVerification() {
+ if (isRunning()) {
+ throw new IllegalStateException("Cannot perform verification
because Reporting Task is not fully stopped");
+ }
+ }
+
+ @Override
+ public List<ConfigVerificationResult> verifyConfiguration(final
ConfigurationContext context, final ComponentLog logger, final ExtensionManager
extensionManager) {
+ final List<ConfigVerificationResult> results = new ArrayList<>();
+
+ try {
+ verifyCanPerformVerification();
+
+ final long startNanos = System.nanoTime();
+
+ final Map<PropertyDescriptor, PropertyConfiguration>
descriptorToConfigMap = new LinkedHashMap<>();
+ for (final Map.Entry<PropertyDescriptor, String> entry :
context.getProperties().entrySet()) {
+ final PropertyDescriptor descriptor = entry.getKey();
+ final String rawValue = entry.getValue();
+ final String propertyValue = rawValue == null ?
descriptor.getDefaultValue() : rawValue;
+
+ final PropertyConfiguration propertyConfiguration = new
PropertyConfiguration(propertyValue, null, Collections.emptyList());
+ descriptorToConfigMap.put(descriptor, propertyConfiguration);
+ }
+
+ final ValidationContext validationContext =
getValidationContextFactory().newValidationContext(descriptorToConfigMap,
context.getAnnotationData(),
+ getProcessGroupIdentifier(), getIdentifier(), null, false);
+
+ final ValidationState validationState =
performValidation(validationContext);
+ final ValidationStatus validationStatus =
validationState.getStatus();
+
+ if (validationStatus == ValidationStatus.INVALID) {
+ for (final ValidationResult result :
validationState.getValidationErrors()) {
+ if (result.isValid()) {
+ continue;
+ }
+
+ results.add(new ConfigVerificationResult.Builder()
+ .outcome(Outcome.FAILED)
+ .explanation("Reporting Task is invalid: " +
result.toString())
+ .verificationStepName("Perform Validation")
Review comment:
Constant here?
##########
File path:
nifi-commons/nifi-expression-language/src/main/java/org/apache/nifi/attribute/expression/language/StandardPreparedQuery.java
##########
@@ -77,6 +77,53 @@ public boolean isExpressionLanguagePresent() {
return false;
}
+ @Override
+ public Set<String> getExplicitlyReferencedAttributes() {
+ final Set<String> variables = new HashSet<>();
Review comment:
Minor nit: shall we call this attributes to match the method name?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java
##########
@@ -1062,6 +1067,94 @@ public int getActiveThreadCount() {
return nonLoopConnections;
}
+ @Override
+ public List<ConfigVerificationResult> verifyConfiguration(final
ProcessContext context, final ComponentLog logger, final Map<String, String>
attributes, final ExtensionManager extensionManager) {
+ final List<ConfigVerificationResult> results = new ArrayList<>();
+
+ try {
+ verifyCanPerformVerification();
+
+ final long startNanos = System.nanoTime();
+
+ final Map<PropertyDescriptor, PropertyConfiguration>
descriptorToConfigMap = new LinkedHashMap<>();
+ for (final Map.Entry<PropertyDescriptor, String> entry :
context.getProperties().entrySet()) {
+ final PropertyDescriptor descriptor = entry.getKey();
+ final String rawValue = entry.getValue();
+ final String propertyValue = rawValue == null ?
descriptor.getDefaultValue() : rawValue;
+
+ final PropertyConfiguration propertyConfiguration = new
PropertyConfiguration(propertyValue, null, Collections.emptyList());
+ descriptorToConfigMap.put(descriptor, propertyConfiguration);
+ }
+
+ final ValidationContext validationContext =
getValidationContextFactory().newValidationContext(descriptorToConfigMap,
context.getAnnotationData(),
+ getProcessGroupIdentifier(), getIdentifier(),
getProcessGroup().getParameterContext(), false);
+
+ final ValidationState validationState =
performValidation(validationContext);
+ final ValidationStatus validationStatus =
validationState.getStatus();
+
+ if (validationStatus == ValidationStatus.INVALID) {
+ for (final ValidationResult result :
validationState.getValidationErrors()) {
+ if (result.isValid()) {
+ continue;
+ }
+
+ results.add(new ConfigVerificationResult.Builder()
+ .outcome(Outcome.FAILED)
+ .explanation("Processor is invalid: " +
result.toString())
+ .verificationStepName("Perform Validation")
Review comment:
What do you think about having constant for this?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/processor/StandardProcessContext.java
##########
@@ -85,6 +90,51 @@ public StandardProcessContext(final ProcessorNode
processorNode, final Controlle
}
}
+ public StandardProcessContext(final ProcessorNode processorNode, final
Map<String, String> propertiesOverride, final String annotationDataOverride,
final ParameterLookup parameterLookup,
+ final ControllerServiceProvider
controllerServiceProvider, final PropertyEncryptor propertyEncryptor,
+ final StateManager stateManager, final
TaskTermination taskTermination, final NodeTypeProvider nodeTypeProvider) {
+ this.procNode = processorNode;
+ this.controllerServiceProvider = controllerServiceProvider;
+ this.propertyEncryptor = propertyEncryptor;
+ this.stateManager = stateManager;
+ this.taskTermination = taskTermination;
+ this.nodeTypeProvider = nodeTypeProvider;
Review comment:
Can this constructor be refactored to call the existing constructor? I
notice preparedQueries would be different, but perhaps we could create a method
that initializes the preparedQueries, given a `Map<PropertyDescriptor, String>`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
