You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/15 00:26:36 UTC
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Lehel44 commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1022159847
##########
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.
Review Comment:
```suggestion
* If this method is called multiple times with different dependencies, a relationship is established so that the original Property is only relevant if all dependencies are satisfied.
```
##########
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.
Review Comment:
The naming "this Property" and "given property" is not trivial. I suggest renaming it to "dependent" or "original" property" and "conditional property" even if it's not consistent with other Javadoc parts. If this is agreeable, it'd apply to the remaining javadoc, too.
```suggestion
* Establishes a relationship between the original Property and the conditional property by declaring that the original Property is only relevant if the conditional Property value equals to one of the given
* {@link DescribedValue} arguments.
```
##########
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.
+ *
+ * If the given property is not relevant (because its dependencies are not satisfied), this property is also considered not to be valid.
+ *
+ * @param property the property that must be set in order for this property to become relevant
Review Comment:
```suggestion
* @param property the property that must be set in order for this property to become relevant
```
```suggestion
* @param property the conditional property
```
##########
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.
+ *
+ * If the given property is not relevant (because its dependencies are not satisfied), this property is also considered not to be valid.
+ *
+ * @param property the property that must be set in order for this property to become relevant
+ * @param firstDependentValue the first value for the given property for which this Property is relevant
Review Comment:
Please update the javadoc in case of renaming the parameters below.
##########
nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/notification/http/HttpNotificationService.java:
##########
@@ -127,14 +124,6 @@ public class HttpNotificationService extends AbstractNotificationService {
.sensitive(true)
.build();
- public static final PropertyDescriptor SSL_ALGORITHM = new PropertyDescriptor.Builder()
Review Comment:
Was this intentional?
##########
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.
+ *
+ * If the given property is not relevant (because its dependencies are not satisfied), this property is also considered not to be valid.
Review Comment:
This one is difficult to understand. AFAIK "relevant" means that a property's dependencies are satisfied and it's displayed on the UI. If any of the dependent properties are not relevant, for example:
`DummyProperty.
.dependsOn(dependentProperty1, DescribedValue1)
`
where `dependentProperty1` is also not relevant for some reason, does it make DummyProperty **not relevant** or **invalid**?
##########
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:
```suggestion
* If this property is NOT considered relevant (it is dependent on a property whose value is not specified or whose value does not match one of the given
* Described Values), it will not be displayed in the component's configuration on the User Interface. Additionally, the value of this property will not be validated.
* That is, even if the value of this property is invalid but it depends on Property Foo, which also does not have a value set, the component
* will still be valid since the value of this property is irrelevant.
```
##########
nifi-api/src/main/java/org/apache/nifi/components/PropertyDescriptor.java:
##########
@@ -338,6 +337,23 @@ public Builder defaultValue(final String value) {
return this;
}
+ /**
+ * Specifies the initial value and the default value that will be used
+ * if the user does not specify a value. When {@link #build()} is
Review Comment:
```suggestion
* if no value is specified by the user. When {@link #build()} is
```
##########
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.
+ *
+ * If the given property is not relevant (because its dependencies are not satisfied), this property is also considered not to be valid.
+ *
+ * @param property the property that must be set in order for this property to become relevant
+ * @param firstDependentValue the first value for the given property for which this Property is relevant
+ * @param additionalDependentValues any other values for the given property for which this Property is relevant
+ * @return the builder
+ */
+ public Builder dependsOn(final PropertyDescriptor property, final DescribedValue firstDependentValue, final DescribedValue... additionalDependentValues) {
Review Comment:
I think the original Property is dependent on the conditional properties, I'd rename them to `firstConditionalValue` and `additionalConditionalValues`.
##########
nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/notification/http/HttpNotificationService.java:
##########
@@ -92,15 +92,13 @@ public class HttpNotificationService extends AbstractNotificationService {
public static final PropertyDescriptor PROP_TRUSTSTORE_PASSWORD = new PropertyDescriptor.Builder()
.name("Truststore Password")
.description("The password for the Truststore")
- .defaultValue(null)
Review Comment:
Does removing these null default values come from any of the current changes?
--
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