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 2021/06/14 07:19:51 UTC
[GitHub] [nifi] ChrisSamo632 opened a new pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
ChrisSamo632 opened a new pull request #5147:
URL: https://github.com/apache/nifi/pull/5147
…perties of SSLContextService
<!--
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.
-->
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
Support Expression Language for the Truststore/Keystore properties of SSLContextService.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [x] Have you verified that the full build is successful on JDK 11?
- [ ] ~If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?~
- [ ] ~If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?~
- [ ] ~If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?~
- [ ] ~If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?~
### For documentation related changes:
- [ ] ~Have you ensured that format looks appropriate for the output in which it is rendered?~
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654805376
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
Review comment:
Sounds good, I was mainly asking whether the method signature could be changed to just `evaluateProperties(PropertyContext context)` based on how the method was being called.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654694783
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
Review comment:
Is there a reason for passing the Map of properties as a separate parameter from the PropertyContext as opposed to just passing the PropertyContext and then calling getProperties() in this method?
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -582,6 +598,6 @@ public String toString() {
allowableValues.add(new AllowableValue(supportedProtocol, supportedProtocol, description));
}
- return allowableValues.toArray(new AllowableValue[allowableValues.size()]);
+ return allowableValues.toArray(new AllowableValue[0]);
Review comment:
Is there a reason for changing the AllowableValue array size to 0 here?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r655350853
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
Review comment:
@markobean yes, see https://github.com/apache/nifi/pull/5147/commits/a27fc7955076749e3db49c80deef0f0b7d038377#diff-d1b1db3b2c0d67997b262529940d794f98ac6a485c6a21316f7af1b935d95ffaR204 (commit 2)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654943433
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
+ for (final PropertyDescriptor pd : properties.keySet()) {
+ if (pd.isExpressionLanguageSupported() && context.getProperty(pd).isSet()) {
+ properties.put(pd, context.getProperty(pd).evaluateAttributeExpressions().getValue());
Review comment:
Fixed with code similar to what you suggested (I tried using Java Streams initially but the `Collectors.toMap` doesn't like `null` values, so instead I went with a straight up "old school" for` loop)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654943460
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
Review comment:
API simplified
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654755602
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
+ for (final PropertyDescriptor pd : properties.keySet()) {
+ if (pd.isExpressionLanguageSupported() && context.getProperty(pd).isSet()) {
Review comment:
My thinking was that this use of the loop was more future proof than explicitly checking individual properties (doing so would mean more updates in future if anyone wanted to make other properties EL enabled)
But per my reply to @exceptionfactory below, changing the `if` may allow a slightly simpler api of using the `PropertyContext`, so I'll consider both those comments together when I next look at the code
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654943323
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
+ for (final PropertyDescriptor pd : properties.keySet()) {
+ if (pd.isExpressionLanguageSupported() && context.getProperty(pd).isSet()) {
Review comment:
I had a look at this, but decided not to make the change (unless you feel strongly about this) - it would mean adding code similar to the following:
```java
private final static List<PropertyDescriptor> EL_PROPERTIES = Collections.unmodifiableList(Arrays.asList(KEYSTORE, TRUSTSTORE));
```
Although actually would need to be an overridable method so that `StandardRestrictedSSLContextService` could potentially provide different values in the future.
Doing this would introduce a new Collection/method for developers to maintain in the future if they added more Properties with EL support (or changed existing properties to allow EL) - as they're already specifying that as part of the `PropertyDescriptor`, it seems unnecessary (and less maintainable) to have another set of properties to maintain in one or more classes (same argument for putting things directly in the `if`, e.g. `if (KEYSTORE.equals(pd) || TRUSTSTORE.equals(pd)) { ... }` - I'd say that was even less maintainable as now the developer needs to know to look further through the code to understand what needs updating.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#issuecomment-865086534
I like the simplified method signature. Also, re-tested, and the UnsupportedOperationException is now resolved.
Implemented a Post/ListenHTTP pair using both StandardSSLContextService and StandardRestrictedSSLContextService, each with EL specified for keystore and truststore files. (ListenHTTP has ability to use only StandardRestrictedSSLContextService.) FlowFiles transferred successfully.
Looks good now! Thanks for the update.
+1
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654755348
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -582,6 +598,6 @@ public String toString() {
allowableValues.add(new AllowableValue(supportedProtocol, supportedProtocol, description));
}
- return allowableValues.toArray(new AllowableValue[allowableValues.size()]);
+ return allowableValues.toArray(new AllowableValue[0]);
Review comment:
Performance and memory
The java api here simply needs to know what object type to use for the array being constructed, it doesn't use the array that's created for the parameter in the `toArray` method)... so simply using 0 here means we save the call to `.size` and memory of allocating an unnecessarily large array that's unused
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5147:
URL: https://github.com/apache/nifi/pull/5147
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654805015
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -582,6 +598,6 @@ public String toString() {
allowableValues.add(new AllowableValue(supportedProtocol, supportedProtocol, description));
}
- return allowableValues.toArray(new AllowableValue[allowableValues.size()]);
+ return allowableValues.toArray(new AllowableValue[0]);
Review comment:
Thanks for the helpful reply!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654755140
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
Review comment:
The `PropertyContext#getProperties` uses the property names as keys and not `PropertyDescriptor`s whereas I needed access to the latter to check whether they supported expression language
When I look at @markobean suggestion (changing the if from an expression language check to one based on explicit property names), I'll consider whether the `PropertyContext` could then be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#issuecomment-865086534
I like the simplified method signature. Also, re-tested, and the UnsupportedOperationException is now resolved.
Implemented a Post/ListenHTTP pair using both StandardSSLContextService and StandardRestrictedSSLContextService, each with EL specified for keystore and truststore files. (ListenHTTP has ability to use only StandardRestrictedSSLContextService.) FlowFiles transferred successfully.
Looks good now! Thanks for the update.
+1
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654612219
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
+ for (final PropertyDescriptor pd : properties.keySet()) {
+ if (pd.isExpressionLanguageSupported() && context.getProperty(pd).isSet()) {
+ properties.put(pd, context.getProperty(pd).evaluateAttributeExpressions().getValue());
Review comment:
@markobean I figured it would be something like that. Have been on holiday this past week, will hopefully get to look at this sometime soon once back
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5147:
URL: https://github.com/apache/nifi/pull/5147
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
markobean commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r655346488
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
Review comment:
Chris- you mention "API simplified" implying you updated the signature to be `evaluateProperties(PropertyContext context)`, but I don't see that update. Have you pushed your latest? Thx.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
markobean commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654587700
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
+ for (final PropertyDescriptor pd : properties.keySet()) {
+ if (pd.isExpressionLanguageSupported() && context.getProperty(pd).isSet()) {
+ properties.put(pd, context.getProperty(pd).evaluateAttributeExpressions().getValue());
Review comment:
Chris,
I looked into this a little more, and discovered the error is occurring due to an attempt to modify an unmodifiable collection (map) here.
I was able to avoid the UnsupportedOperationException by creating a new HashMap for the evaluated properties, and return it from this method. I haven't fully thought through if this is a concern with effectively side-stepping the "unmodifiable" intent of the map.
private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
Map<PropertyDescriptor, String> evaluatedProperties = new HashMap<>();
for (final PropertyDescriptor pd : properties.keySet()) {
if (pd.isExpressionLanguageSupported() && context.getProperty(pd).isSet()) {
evaluatedProperties.put(pd, context.getProperty(pd).evaluateAttributeExpressions().getValue());
} else {
evaluatedProperties.put(pd, context.getProperty(pd).getValue());
}
}
return evaluatedProperties;
}
-Mark
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r655350853
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
Review comment:
@markobean yes, see https://github.com/apache/nifi/pull/5147/commits/a27fc7955076749e3db49c80deef0f0b7d038377#diff-d1b1db3b2c0d67997b262529940d794f98ac6a485c6a21316f7af1b935d95ffaR204 (commit 2)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r654987437
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
+ for (final PropertyDescriptor pd : properties.keySet()) {
+ if (pd.isExpressionLanguageSupported() && context.getProperty(pd).isSet()) {
+ properties.put(pd, context.getProperty(pd).evaluateAttributeExpressions().getValue());
Review comment:
As to the modifiability of the Map - these properties are only used internally within the controller service and (currently) are treated as read-only by all the methods that use them (so far as I can see)
So we could (maybe should) make them unmodifiable after EL has been evaluated (way to do although I forgot to include in my latest commit) - can do this if you think best
The underlying properties (descriptors & values) should not be affected by the nature of the Map used herein
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a change in pull request #5147: NIFI-8683 support Expression Language for the Truststore/Keystore pro…
Posted by GitBox <gi...@apache.org>.
markobean commented on a change in pull request #5147:
URL: https://github.com/apache/nifi/pull/5147#discussion_r655346488
##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java
##########
@@ -185,14 +191,24 @@ public void onPropertyModified(PropertyDescriptor descriptor, String oldValue, S
}
}
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.KEYSTORE));
- results.addAll(validateStore(validationContext.getProperties(), KeystoreValidationGroup.TRUSTSTORE));
+ final Map<PropertyDescriptor, String> properties = evaluateProperties(validationContext.getProperties(), validationContext);
+ results.addAll(validateStore(properties, KeystoreValidationGroup.KEYSTORE));
+ results.addAll(validateStore(properties, KeystoreValidationGroup.TRUSTSTORE));
isValidated = results.isEmpty();
return results;
}
+ private Map<PropertyDescriptor, String> evaluateProperties(final Map<PropertyDescriptor, String> properties, final PropertyContext context) {
Review comment:
Chris- you mention "API simplified" implying you updated the signature to be `evaluateProperties(PropertyContext context)`, but I don't see that update. Have you pushed your latest? Thx.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org