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/11 02:59:20 UTC
[GitHub] [nifi] nandorsoma opened a new pull request, #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
nandorsoma opened a new pull request, #6650:
URL: https://github.com/apache/nifi/pull/6650
<!-- 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. -->
# Summary
[NIFI-10775](https://issues.apache.org/jira/browse/NIFI-10775)
This PR adds support for DescribedValue in the `dependsOn()` and in the `defaultValue()` builder methods. Unfortunately, the new signature defaultValue(null) doesn’t work. Only if you cast it. Because of that, I’ve removed the redundant null defaultValue assignments. However, at this point, I’m unsure what brings more value, and I cannot decide it at the moment. Sometimes it is useful to indicate that the default value is null. On the other hand, reducing redundancy and getting rid of unnecessary getValues() contribute to clean code. A workaround could be to add `defaultNull()` to the Builder() that can be used instead of `defaultValue(null)`. That could be useful in the case of `sensitive(boolean)` too.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [x] Build completed using `mvn clean install -P contrib-check`
- [x] JDK 8
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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
[GitHub] [nifi] asfgit closed pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
URL: https://github.com/apache/nifi/pull/6650
--
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
Re: [PR] NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder [nifi]
Posted by "nandorsoma (via GitHub)" <gi...@apache.org>.
nandorsoma commented on PR #6650:
URL: https://github.com/apache/nifi/pull/6650#issuecomment-1856580431
> Hey @nandorsoma, just stumbled upon this PR and wanted to let you know, that #8102 introduced the `defaultValue` method you proposed here originally.
>
>
>
> It will be available in the upcoming major release version 2.x, as we can make the breaking change introduced by the method overload then.
Thanks for letting me know! GJ!
--
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
[GitHub] [nifi] turcsanyip commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1021809680
##########
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:
I misunderstood the javadoc, no change needed.
Please ignore my comment.
--
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
[GitHub] [nifi] nandorsoma commented on pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on PR #6650:
URL: https://github.com/apache/nifi/pull/6650#issuecomment-1315126161
Hey @Lehel44!
Thank you for your review! I've answered your questions inline, which were related to functionality. About the Javadoc... I mainly used the original Javadoc and removed a part that was unrelated to this case. Because of that, I'd leave it as it is for now if it suits you.
--
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
[GitHub] [nifi] nandorsoma commented on pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on PR #6650:
URL: https://github.com/apache/nifi/pull/6650#issuecomment-1314419042
Thanks for the review @turcsanyip! I've addressed your comments, please see my latest commit.
--
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
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1022157647
##########
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". 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.
```
--
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
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [nifi] turcsanyip commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1021588923
##########
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
+ * called, if Allowable Values have been set (see
+ * {@link #allowableValues(Class)}) and this value is not
+ * one of those Allowable Values and Exception will be thrown.
Review Comment:
I think there is a typo in the original comment (where this was copied from):
```suggestion
* called, if Allowable Values have been set (see
* {@link #allowableValues(Class)}) and this value is not
* one of those Allowable Values, an Exception will be thrown.
```
--
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
[GitHub] [nifi] nandorsoma commented on pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on PR #6650:
URL: https://github.com/apache/nifi/pull/6650#issuecomment-1315579187
Thank you for your review @exceptionfactory! You are right, I didn't think about custom components. I've removed the new defaultValue method.
--
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
[GitHub] [nifi] nandorsoma commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1022615073
##########
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:
Yes, it was not used.
--
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
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1022178121
##########
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.
--
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
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
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 this Property is only relevant if all dependencies are satisfied.
```
--
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
[GitHub] [nifi] exceptionfactory commented on pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6650:
URL: https://github.com/apache/nifi/pull/6650#issuecomment-1315434084
Thanks for putting together this improvement @nandorsoma! Following the note in the summary, introducing the new `defaultValue(DescribedValue)` method signature would create potential compatibility problems for custom components that currently use `defaultValue(null)` in a `PropertyDescriptor.Builder`. As this is part of `nifi-api`, compatibility should be maintained, so it would be best to remove that addition and back out the removal of `defaultValue(null)` from the various components.
--
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
[GitHub] [nifi] turcsanyip commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by 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
[GitHub] [nifi] nandorsoma commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1022623523
##########
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:
Yes, I mentioned it in the summary.
--
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
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1022157647
##########
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". 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.
```
--
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
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6650: NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on code in PR #6650:
URL: https://github.com/apache/nifi/pull/6650#discussion_r1022180833
##########
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`.
--
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
Re: [PR] NIFI-10775 Improve support for DescribedValue in PropertyDescriptor.Builder [nifi]
Posted by "EndzeitBegins (via GitHub)" <gi...@apache.org>.
EndzeitBegins commented on PR #6650:
URL: https://github.com/apache/nifi/pull/6650#issuecomment-1856573392
Hey @nandorsoma, just stumbled upon this PR and wanted to let you know, that #8102 introduced the `defaultValue` method you proposed here originally.
It will be available in the upcoming major release version 2.x, as we can make the breaking change introduced by the method overload then.
--
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