-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider #7313
Conversation
Will review... |
...-jms-processors/src/main/java/org/apache/nifi/jms/cf/JndiJmsConnectionFactoryProperties.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @exceptionfactory! The approach of customization via system properties looks good.
I added a few improvement comments. The most important one is the single vs. multiple disallowed context factories. Pls. see them below.
...-jms-processors/src/main/java/org/apache/nifi/jms/cf/JndiJmsConnectionFactoryProperties.java
Outdated
Show resolved
Hide resolved
...-jms-processors/src/main/java/org/apache/nifi/jms/cf/JndiJmsConnectionFactoryProperties.java
Outdated
Show resolved
Hide resolved
...esources/docs/org.apache.nifi.jms.cf.JndiJmsConnectionFactoryProvider/additionalDetails.html
Outdated
Show resolved
Hide resolved
...esources/docs/org.apache.nifi.jms.cf.JndiJmsConnectionFactoryProvider/additionalDetails.html
Outdated
Show resolved
Hide resolved
...ms-processors/src/test/java/org/apache/nifi/jms/cf/JndiJmsConnectionFactoryProviderTest.java
Outdated
Show resolved
Hide resolved
Thanks for the additional feedback @turcsanyip! On further consideration, the URL Scheme validation seems sufficient, so I removed the Context Factory class validation and associated documentation. I also adjusted the System property evaluation approach so that the unit tests could evaluate both scenarios. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @exceptionfactory!
+1 LGTM
Merging to main and support/1.x.
This closes #7313. Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
Thanks for implementing a validator. Because these are very useful. Unfortunately, this currently prevents the use of salace and nifi. the solace messaging protocol (SMF) uses a url format like Unfortunately the validator fails with |
Thanks for the feedback @schnells. As this pull request is closed, it would be helpful to create an Apache NiFi Jira issue for further discussion. Adding As an alternative for now, the Provider supports a Java System property that can be used to set the allowed URLs. In this scenario, the following line can be added to
|
Thanks @exceptionfactory for the quick response! I will absolutely have a look into the java system property. |
This closes apache#7313. Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org> (cherry picked from commit b042eb0)
Summary
NIFI-11614 Improves property validation for the
JndiJmsConnectionFactoryProvider
Controller Service with a set of allowed URL schemes and disallowed Context Factory classes.The current implementation only checks for a configured value with standard properties, but the new validation narrows the scope to provide better validation prior to enabling the provider.
The initial set of allowed URL schemes is based on the ActiveMQ Using JMS documentation. The new URL validation narrows the scope of potential values, which should be noted in migration guidance, providing a clearer set of supported values
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation