[GitHub] [nifi] turcsanyip commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder

[GitBox <gi...@apache.org>]

turcsanyip commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1021639794


##########
nifi-api/src/test/java/org/apache/nifi/components/TestPropertyDescriptor.java:
##########
@@ -79,6 +86,15 @@ void testPropertyDescriptorWithEnumValue() {
                 .map(enumValue -> new AllowableValue(enumValue.name(), enumValue.getDisplayName(), enumValue.getDescription()))
                 .collect(Collectors.toList());
         assertEquals(expectedAllowableValues, propertyDescriptor.getAllowableValues());
+
+        final Set<PropertyDependency> dependencies = propertyDescriptor.getDependencies();
+        assertEquals(1, dependencies.size());
+        final PropertyDependency dependency = dependencies.iterator().next();
+        assertEquals(DEPENDENT_PROPERTY_NAME, dependency.getPropertyName());
+        final Set<String> dependentValues = dependency.getDependentValues();
+        assertEquals(1, dependentValues.size());
+        final String dependentValue = dependentValues.iterator().next();
+        assertEquals(EnumAllowableValue.RED.name(), dependentValue);

Review Comment:
   `getValue()` should be used instead of `name()` (also in the original test).
   The point here is that the default / dependent values match the `getValue()` of `EnumAllowableValue` (which happens to be the `name()` of the enum but it could be something else).
   
   I would also consider creating a separate test case for the new feature(s). It is hard to follow what is being tested exactly (there is a property with allowable values and it depends on another property having the same allowable values).
   
   



##########
nifi-api/src/main/java/org/apache/nifi/components/PropertyDescriptor.java:
##########
@@ -591,6 +606,38 @@ public Builder dependsOn(final PropertyDescriptor property, final String firstDe
             return dependsOn(property, dependentValues);
         }
 
+        /**
+         * Establishes a relationship between this Property and the given property by declaring that this Property is only relevant if the given Property has a value equal to one of the given
+         * {@link DescribedValue} arguments.
+         * If this method is called multiple times, each with a different dependency, then a relationship is established such that this Property is relevant only if all dependencies are satisfied.
+         *
+         * In the case that this property is NOT considered to be relevant (meaning that it depends on a property whose value is not specified, or whose value does not match one of the given
+         * Described Values), the property will not be shown in the component's configuration in the User Interface. Additionally, this property's value will not be considered for
+         * validation. That is, if this property is configured with an invalid value and this property depends on Property Foo, and Property Foo does not have a value set, then the component
+         * will still be valid, because the value of this property is irrelevant.

Review Comment:
   The `meaning that it depends on a property whose value is not specified` part is not relevant here because the method always has at least one dependent value.



-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org