markap14 commented on code in PR #6115: URL: https://github.com/apache/nifi/pull/6115#discussion_r902915510
########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java: ########## @@ -1209,33 +1194,112 @@ protected Collection<ValidationResult> computeValidationErrors(final ValidationC } @Override - protected List<ValidationResult> validateConfig() { + public List<ValidationResult> validateConfig() { final List<ValidationResult> results = new ArrayList<>(); final ParameterContext parameterContext = getParameterContext(); - if (parameterContext == null && !this.parameterReferences.isEmpty()) { + if (parameterContext == null && !this.configurationParameterReferences.isEmpty()) { results.add(new ValidationResult.Builder() - .subject(getName()) + .subject(RUN_SCHEDULE) + .input("Parameter Context") .valid(false) .explanation("Processor configuration references one or more Parameters but no Parameter Context is currently set on the Process Group.") .build()); } else { - for (final ParameterReference paramRef : parameterReferences) { - if (!parameterContext.getParameter(paramRef.getParameterName()).isPresent() ) { + for (final ParameterReference paramRef : configurationParameterReferences) { + final Optional<Parameter> parameterRef = parameterContext.getParameter(paramRef.getParameterName()); + if (!parameterRef.isPresent() ) { results.add(new ValidationResult.Builder() - .subject(getName()) + .subject(RUN_SCHEDULE) + .input(paramRef.getParameterName()) .valid(false) .explanation("Processor configuration references Parameter '" + paramRef.getParameterName() + "' but the currently selected Parameter Context does not have a Parameter with that name") .build()); + } else { + final ParameterDescriptor parameterDescriptor = parameterRef.get().getDescriptor(); + if (parameterDescriptor.isSensitive()) { + results.add(new ValidationResult.Builder() + .subject(RUN_SCHEDULE) + .input(parameterDescriptor.getName()) + .valid(false) + .explanation("Processor configuration cannot reference sensitive parameters") + .build()); + } } } - } + final String schedulingPeriod = getSchedulingPeriod(); + final String evaluatedSchedulingPeriod = evaluateParameter(schedulingPeriod); + + if (evaluatedSchedulingPeriod != null) { Review Comment: This should never be `null`, should it? ########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java: ########## @@ -1969,17 +2033,12 @@ public void setMaxBackoffPeriod(String maxBackoffPeriod) { } @Override - public String evaluateSchedulingPeriodFromParameter(final String schedulingPeriod) { + public String evaluateParameter(final String parameter) { Review Comment: I don't think the argument here is really a parameter is it? It's some value that may or may not contain parameters. Perhaps should call it `evaluateParameters` and call the argument `value`? ########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java: ########## @@ -1084,12 +1092,29 @@ public void onParametersModified(final Map<String, ParameterUpdate> updatedParam } // If this component is affected by the Parameter change, we need to re-validate + componentAffected |= isConfigurationParameterModified(updatedParameters); if (componentAffected) { logger.debug("Configuration of {} changed due to an update to Parameter Context. Resetting validation state", this); resetValidationState(); } } + protected void increaseReferenceCounts(final String parameterName) { + parameterReferenceCounts.merge(parameterName, 1, (a, b) -> a == -1 ? null : a + b); + } + + protected void decreaseReferenceCounts(final String parameterName) { + parameterReferenceCounts.merge(parameterName, -1, (a, b) -> a == 1 ? null : a + b); + } + + /** + * Verifies whether any referenced configuration parameter has been updated and its effective value has changed. + * If yes, the component needs to be re-validated. + * @param updatedParameters updated parameters + * @return true if the component requires re-validation Review Comment: With a method named `isConfigurationParameterModified`, it should not return `true` to indicate that the component requires validation. That's simply a side-effect. It should return `true` if any of the given parameters are used in its configuration. ########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java: ########## @@ -1084,12 +1092,29 @@ public void onParametersModified(final Map<String, ParameterUpdate> updatedParam } // If this component is affected by the Parameter change, we need to re-validate + componentAffected |= isConfigurationParameterModified(updatedParameters); if (componentAffected) { logger.debug("Configuration of {} changed due to an update to Parameter Context. Resetting validation state", this); resetValidationState(); } } + protected void increaseReferenceCounts(final String parameterName) { + parameterReferenceCounts.merge(parameterName, 1, (a, b) -> a == -1 ? null : a + b); + } + + protected void decreaseReferenceCounts(final String parameterName) { Review Comment: Given that this is always changing the value by `1`, it probably is best to call these `incrementReferenceCounts` and `decrementReferenceCounts` ########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/connectable/Connectable.java: ########## @@ -311,5 +311,5 @@ default boolean isSessionBatchingSupported() { void setMaxBackoffPeriod(String maxBackoffPeriod); - String evaluateSchedulingPeriodFromParameter(String schedulingPeriod); + String evaluateParameter(String parameter); Review Comment: As commented, based on the way this is used, I think the signature should be `String evaluateParameters(String value);` ########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/parameter/StandardParameterReferenceManager.java: ########## @@ -128,6 +128,14 @@ private boolean isComponentReferencing(final ComponentNode componentNode, final } } + if (componentNode instanceof ProcessorNode) { Review Comment: We shouldn't be treating ProcessorNode specially. Perhaps it makes more sense to get rid of this `isComponentReferencing` method from `StandardParameterReferenceManager` and instead add a `isReferencingParameter(String parameterName)` to `ComponentNode`. Then the existing implementation would move to `AbstractComponentNode` and `StandardProcessorNode` can then override the method, call the `super.isReferencingParameter` and if false check its `configurationParameterReferences` ########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java: ########## @@ -1462,6 +1459,25 @@ private Set<AffectedComponentEntity> getComponentsAffectedByParameterContextUpda return dtoFactory.createAffectedComponentEntities(affectedComponents, revisionManager); } + private void getAffectedComponentsByParameterUpdate(final ParameterContextDTO parameterContextDto, final Set<String> updatedParameterNames, final Set<ComponentNode> affectedComponents, Review Comment: We should not have a getter that doesn't return a value. Is the intent to populate the provided `affectedComponents` set? If so, perhaps that should be returned instead. ########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java: ########## @@ -1084,12 +1092,29 @@ public void onParametersModified(final Map<String, ParameterUpdate> updatedParam } // If this component is affected by the Parameter change, we need to re-validate + componentAffected |= isConfigurationParameterModified(updatedParameters); if (componentAffected) { logger.debug("Configuration of {} changed due to an update to Parameter Context. Resetting validation state", this); resetValidationState(); } } + protected void increaseReferenceCounts(final String parameterName) { + parameterReferenceCounts.merge(parameterName, 1, (a, b) -> a == -1 ? null : a + b); + } + + protected void decreaseReferenceCounts(final String parameterName) { + parameterReferenceCounts.merge(parameterName, -1, (a, b) -> a == 1 ? null : a + b); + } + + /** + * Verifies whether any referenced configuration parameter has been updated and its effective value has changed. + * If yes, the component needs to be re-validated. + * @param updatedParameters updated parameters + * @return true if the component requires re-validation + **/ + public abstract boolean isConfigurationParameterModified(final Map<String, ParameterUpdate> updatedParameters); Review Comment: I'm not sure exactly what a "configuration parameter" is - i presume here we mean a parameter that is used within the configuration, other than properties, such as the Scheduling Period. But not sure that this comes across from the code. Also not entirely sure that this is something that we should distinguish between - parameters that are used by properties and parameters that are used elsewhere. Perhaps this would call for a slight refactoring elsewhere but not sure that we should distinguish between the two. Should we just have a `Set<String> getReferencedParameterNames()` on ComponentNode? Would that give us everything that we need? ########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/ProcessorNode.java: ########## @@ -300,4 +301,12 @@ public abstract CompletableFuture<Void> stop(ProcessScheduler processScheduler, * @param context The ProcessContext associated with the Processor configuration */ public abstract void onConfigurationRestored(ProcessContext context); + + /** + * Returns those parameter references which configures the Processor through Parameters. This is for Processor configuration only, + * Processor Property Parameter references stored and handled separately. + * + * @return the configuration Parameter references for this Processor + */ + public abstract List<ParameterReference> getConfigurationParameterReferences(); Review Comment: Not sure that the naming convention here makes sense. Parameters are always used for configuration. Processor properties are very much configuration. ########## nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java: ########## @@ -1462,6 +1459,25 @@ private Set<AffectedComponentEntity> getComponentsAffectedByParameterContextUpda return dtoFactory.createAffectedComponentEntities(affectedComponents, revisionManager); } + private void getAffectedComponentsByParameterUpdate(final ParameterContextDTO parameterContextDto, final Set<String> updatedParameterNames, final Set<ComponentNode> affectedComponents, Review Comment: Perhaps this should be `getComponentsAffectedBy...` -- 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