markap14 commented on code in PR #6057:
URL: https://github.com/apache/nifi/pull/6057#discussion_r877399549
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/NiFiServiceFacade.java:
##########
@@ -616,9 +616,10 @@ Set<DocumentedTypeDTO> getControllerServiceTypes(final
String serviceType, final
*
* @param id id
* @param property property
+ * @param sensitive Sensitive Status
* @return descriptor
*/
- PropertyDescriptorDTO getProcessorPropertyDescriptor(String id, String
property);
+ PropertyDescriptorDTO getProcessorPropertyDescriptor(String id, String
property, boolean sensitive);
Review Comment:
Rather than passing the `sensitive` flag all the way down in these methods,
what do you think of leaving the methods as they are, and then in the Resource
class that calls this method, creating a PropertyDescriptor, if necessary, that
sets the sensitive flag to `true`? Going with that approach, it keeps the
`get<type>PropertyDescriptor` method a little cleaner, as it could be
potentially used elsewhere if need be. Adding the `sensitive` flag seems to
sort of dictate the use case where it would be applicable.
##########
nifi-api/src/main/java/org/apache/nifi/documentation/AbstractDocumentationWriter.java:
##########
@@ -141,6 +142,7 @@ protected void writeBody(final ConfigurableComponent
component, Map<String,Servi
writeTriggerWhenEmpty(processor.getClass().getAnnotation(TriggerWhenEmpty.class));
writeTriggerWhenAnyDestinationAvailable(processor.getClass().getAnnotation(TriggerWhenAnyDestinationAvailable.class));
writeSupportsBatching(processor.getClass().getAnnotation(SupportsBatching.class));
+
writeSupportsSensitiveDynamicProperties(processor.getClass().getAnnotation(SupportsSensitiveDynamicProperties.class));
Review Comment:
This should be done for all `ConfigurableComponent`s, not just `Processor`s.
##########
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/NiFiClientUtil.java:
##########
@@ -768,6 +752,48 @@ public void waitForControllerServiceState(final String
groupId, final String des
}
}
+ public void waitForControllerServiceValidationStatus(final String
controllerServiceId, final String validationStatus) throws NiFiClientException,
IOException {
+ while (true) {
+ final ControllerServiceEntity controllerServiceEntity =
nifiClient.getControllerServicesClient().getControllerService(controllerServiceId);
+ final String currentValidationStatus =
controllerServiceEntity.getStatus().getValidationStatus();
+ if (validationStatus.contentEquals(currentValidationStatus)) {
Review Comment:
Any reason for using `contentEquals` here instead of `equals`? Both are
`String`s so equals() should work.
##########
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/controllerservice/ControllerServiceApiValidationIT.java:
##########
@@ -150,4 +167,59 @@ public void
testMatchingGenericDynamicPropertyControllerService() throws NiFiCli
assertEquals("ENABLED", controllerStatus);
assertEquals("Stopped", processorStatus);
}
+
+ @Test
Review Comment:
This is very minor but i wonder if we should put these tests into a
different class? This one is really intended to verify that Controller Service
API's work properly. I.e., processors can reference service by its interface,
etc., whereas the new ones are around sensitive property validation
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/ComponentNode.java:
##########
@@ -190,6 +191,15 @@ public default void setProperties(Map<String, String>
properties) {
*/
boolean isValidationNecessary();
+ /**
+ * Indicates whether the Component supports sensitive dynamic properties
+ *
+ * @return Support status for Sensitive Dynamic Properties
+ */
+ default boolean isSupportsSensitiveDynamicProperties() {
+ return false;
+ }
+
Review Comment:
I don't think I'd put a default implementation here that returns `false`.
I'd recommend either defining the method in the interface with no
implementation, or maybe implementing with:
```
return
getComponent().getClass().isAnnotationPresent(SupportsSensitiveDynamicProperties.class);
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/audit/ReportingTaskAuditor.java:
##########
@@ -138,13 +142,13 @@ public Object
updateReportingTaskAdvice(ProceedingJoinPoint proceedingJoinPoint,
// create a configuration action accordingly
if (operation != null) {
// clear the value if this property is sensitive
- final PropertyDescriptor propertyDescriptor =
reportingTask.getReportingTask().getPropertyDescriptor(property);
- if (propertyDescriptor != null &&
propertyDescriptor.isSensitive()) {
+ final PropertyDescriptor propertyDescriptor =
reportingTask.getPropertyDescriptor(property);
+ if (propertyDescriptor != null &&
(propertyDescriptor.isSensitive() ||
sensitiveDynamicPropertyNames.contains(property))) {
Review Comment:
Would recommend same thing here - adding a comment as to why we're not just
trusting the PropertyDescriptor's isSenstiive() method
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/audit/ControllerServiceAuditor.java:
##########
@@ -144,13 +148,13 @@ public Object
updateControllerServiceAdvice(ProceedingJoinPoint proceedingJoinPo
// create a configuration action accordingly
if (operation != null) {
// clear the value if this property is sensitive
- final PropertyDescriptor propertyDescriptor =
controllerService.getControllerServiceImplementation().getPropertyDescriptor(property);
- if (propertyDescriptor != null &&
propertyDescriptor.isSensitive()) {
+ final PropertyDescriptor propertyDescriptor =
controllerService.getPropertyDescriptor(property);
+ if (propertyDescriptor != null &&
(propertyDescriptor.isSensitive() ||
sensitiveDynamicPropertyNames.contains(property))) {
Review Comment:
Might be worth adding a comment here that we have to check if isSensitive()
or sensitiveDynamicPropertyNames.contains() because at this point in the stack,
the changes will not have been applied yet. Otherwise, I can see someone
changing this in the future, thinking oh this would be simpler....
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
