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/07/14 18:48:49 UTC
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6204: NIFI-9758 implemented additional Dynamic Properties for JavaMail session
exceptionfactory commented on code in PR #6204:
URL: https://github.com/apache/nifi/pull/6204#discussion_r921462808
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -305,6 +315,18 @@ protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
return properties;
}
+ @Override
+ protected PropertyDescriptor getSupportedDynamicPropertyDescriptor(final String propertyDescriptorName) {
+ return new PropertyDescriptor.Builder()
+ .name(propertyDescriptorName)
+ .description("Mail properties to be set to the JavaMail Session")
Review Comment:
```suggestion
.description("SMTP property passed to the Mail Session")
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -79,10 +82,17 @@
@Tags({"email", "put", "notify", "smtp"})
@InputRequirement(Requirement.INPUT_REQUIRED)
@CapabilityDescription("Sends an e-mail to configured recipients for each incoming FlowFile")
+@DynamicProperty(name = "mail._____",
+ value = "Value for a specific property to be set in the JavaMail Session object",
+ description = "The values specified in this dynamic property will be set in the JavaMail Session object. " +
Review Comment:
```suggestion
description = "Dynamic property names that will be passed to the Mail Session. " +
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -79,10 +82,17 @@
@Tags({"email", "put", "notify", "smtp"})
@InputRequirement(Requirement.INPUT_REQUIRED)
@CapabilityDescription("Sends an e-mail to configured recipients for each incoming FlowFile")
Review Comment:
The annotation `@SupportsSensitiveDynamicProperties` should be added to allow optional property encryption for properties such as Proxy passwords.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -464,6 +486,16 @@ private Properties getMailPropertiesFromFlowFile(final ProcessContext context, f
}
}
+ for (final PropertyDescriptor descriptor : context.getProperties().keySet()) {
+ if (descriptor.isDynamic()) {
+ final String flowFileValue = context.getProperty(descriptor).evaluateAttributeExpressions(flowFile).getValue();
+ // Nullable values are not allowed, so filter out
+ if (null != flowFileValue) {
+ properties.setProperty(descriptor.getName(), flowFileValue);
Review Comment:
Recommend renaming `flowFileValue` to `mailPropertyValue`:
```suggestion
final String mailPropertyValue = context.getProperty(descriptor).evaluateAttributeExpressions(flowFile).getValue();
// Nullable values are not allowed, so filter out
if (mailPropertyValue != null) {
properties.setProperty(descriptor.getName(), mailPropertyValue);
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestPutEmail.java:
##########
@@ -299,4 +299,51 @@ public void testOutgoingMessageWithFlowfileContent() throws Exception {
assertEquals("anotherbcc@apache.org", message.getRecipients(RecipientType.BCC)[1].toString());
}
+ @Test
+ public void testValidDynamicMailProperties() throws Exception {
+ runner.setProperty(PutEmail.SMTP_HOSTNAME, "smtp-host");
+ runner.setProperty(PutEmail.HEADER_XMAILER, "TestingNiFi");
+ runner.setProperty(PutEmail.FROM, "test@apache.org,from@apache.org");
+ runner.setProperty(PutEmail.MESSAGE, "${body}");
+ runner.setProperty(PutEmail.TO, "recipient@apache.org,another@apache.org");
+ runner.setProperty(PutEmail.CC, "recipientcc@apache.org,anothercc@apache.org");
+ runner.setProperty(PutEmail.BCC, "recipientbcc@apache.org,anotherbcc@apache.org");
+ runner.setProperty(PutEmail.CONTENT_AS_MESSAGE, "${sendContent}");
+
+ runner.setProperty("mail.smtp.timeout", "sample dynamic mail property");
+ runner.setProperty("mail.smtp.writetimeout", "sample dynamic mail property");
+ runner.setProperty("mail.smtp.connectiontimeout", "sample dynamic mail property");
+ runner.setProperty("mail.smtp.auth.xoauth2.disable", "sample dynamic mail property");
Review Comment:
Setting a single property seems sufficient for testing.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -79,10 +82,17 @@
@Tags({"email", "put", "notify", "smtp"})
@InputRequirement(Requirement.INPUT_REQUIRED)
@CapabilityDescription("Sends an e-mail to configured recipients for each incoming FlowFile")
+@DynamicProperty(name = "mail._____",
Review Comment:
Instead of using underscores as a placeholder, recommend using `propertyName`:
```suggestion
@DynamicProperty(name = "mail.propertyName",
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -79,10 +82,17 @@
@Tags({"email", "put", "notify", "smtp"})
@InputRequirement(Requirement.INPUT_REQUIRED)
@CapabilityDescription("Sends an e-mail to configured recipients for each incoming FlowFile")
+@DynamicProperty(name = "mail._____",
+ value = "Value for a specific property to be set in the JavaMail Session object",
+ description = "The values specified in this dynamic property will be set in the JavaMail Session object. " +
+ "Possible properties can be found in: https://javaee.github.io/javamail/docs/api/com/sun/mail/smtp/package-summary.html.",
+ expressionLanguageScope = ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
@SystemResourceConsideration(resource = SystemResource.MEMORY, description = "The entirety of the FlowFile's content (as a String object) "
+ "will be read into memory in case the property to use the flow file content as the email body is set to true.")
public class PutEmail extends AbstractProcessor {
+ private static final Pattern MAIL_PROPERTY_PATTERN = Pattern.compile("^mail\\.([a-z0-9\\.]+)$");
Review Comment:
This could be adjusted to require `mail.smtp` with an optional `s`:
```suggestion
private static final Pattern MAIL_PROPERTY_PATTERN = Pattern.compile("^mail\\.smtps?\\.([a-z0-9\\.]+)$");
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -523,4 +555,57 @@ private InternetAddress[] toInetAddresses(final ProcessContext context, final Fl
protected void send(final Message msg) throws MessagingException {
Transport.send(msg);
}
+
+ /**
+ * Utility function to return a Map of optional {@code mail._____} properties that were set dynamically.
+ * @param context the ProcessContext
+ * @return a Map with optional {@code mail._____} properties
+ */
+ private Map<String, String> getDynamicMailProperties(final ProcessContext context) {
+ final Map<String, String> dynamicMailProperties = new HashMap<>();
+
+ for (final Map.Entry<PropertyDescriptor, String> entry: context.getProperties().entrySet()) {
+ final PropertyDescriptor descriptor = entry.getKey();
+ if (descriptor.isDynamic()) {
+ final String name = descriptor.getName();
+ final Matcher matcher = MAIL_PROPERTY_PATTERN.matcher(name);
+ if (matcher.matches()) {
+ dynamicMailProperties.put(name, entry.getValue());
+ }
+ }
+ }
+
+ return dynamicMailProperties;
+ }
+
+ private static class DynamicMailPropertyValidator implements Validator {
+ @Override
+ public ValidationResult validate(String subject, String input, ValidationContext context) {
+ final Matcher matcher = MAIL_PROPERTY_PATTERN.matcher(subject);
+ if (!matcher.matches()) {
+ return new ValidationResult.Builder()
+ .input(input)
+ .subject(subject)
+ .valid(false)
+ .explanation(String.format("[%s] is an invalid mail._____ property", subject))
Review Comment:
```suggestion
.explanation(String.format("[%s] does not start with mail.smtp", subject))
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -523,4 +555,57 @@ private InternetAddress[] toInetAddresses(final ProcessContext context, final Fl
protected void send(final Message msg) throws MessagingException {
Transport.send(msg);
}
+
+ /**
+ * Utility function to return a Map of optional {@code mail._____} properties that were set dynamically.
+ * @param context the ProcessContext
+ * @return a Map with optional {@code mail._____} properties
+ */
+ private Map<String, String> getDynamicMailProperties(final ProcessContext context) {
+ final Map<String, String> dynamicMailProperties = new HashMap<>();
+
+ for (final Map.Entry<PropertyDescriptor, String> entry: context.getProperties().entrySet()) {
+ final PropertyDescriptor descriptor = entry.getKey();
+ if (descriptor.isDynamic()) {
+ final String name = descriptor.getName();
+ final Matcher matcher = MAIL_PROPERTY_PATTERN.matcher(name);
+ if (matcher.matches()) {
+ dynamicMailProperties.put(name, entry.getValue());
+ }
+ }
+ }
+
+ return dynamicMailProperties;
+ }
+
+ private static class DynamicMailPropertyValidator implements Validator {
+ @Override
+ public ValidationResult validate(String subject, String input, ValidationContext context) {
+ final Matcher matcher = MAIL_PROPERTY_PATTERN.matcher(subject);
+ if (!matcher.matches()) {
+ return new ValidationResult.Builder()
+ .input(input)
+ .subject(subject)
+ .valid(false)
+ .explanation(String.format("[%s] is an invalid mail._____ property", subject))
+ .build();
+ }
+
+ if (propertyToContext.containsKey(subject)) {
+ return new ValidationResult.Builder()
+ .input(input)
+ .subject(subject)
+ .valid(false)
+ .explanation(String.format("[%s] overwrites required existing properties", subject))
+ .build();
+ }
+
+ return new ValidationResult.Builder()
+ .subject(subject)
+ .input(input)
+ .valid(true)
+ .explanation("Valid mail._____ property that does not overwrite existing required properties")
Review Comment:
```suggestion
.explanation("Valid mail.smtp property found")
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -523,4 +555,57 @@ private InternetAddress[] toInetAddresses(final ProcessContext context, final Fl
protected void send(final Message msg) throws MessagingException {
Transport.send(msg);
}
+
+ /**
+ * Utility function to return a Map of optional {@code mail._____} properties that were set dynamically.
+ * @param context the ProcessContext
+ * @return a Map with optional {@code mail._____} properties
+ */
+ private Map<String, String> getDynamicMailProperties(final ProcessContext context) {
+ final Map<String, String> dynamicMailProperties = new HashMap<>();
+
+ for (final Map.Entry<PropertyDescriptor, String> entry: context.getProperties().entrySet()) {
+ final PropertyDescriptor descriptor = entry.getKey();
+ if (descriptor.isDynamic()) {
+ final String name = descriptor.getName();
+ final Matcher matcher = MAIL_PROPERTY_PATTERN.matcher(name);
+ if (matcher.matches()) {
+ dynamicMailProperties.put(name, entry.getValue());
+ }
+ }
+ }
+
+ return dynamicMailProperties;
+ }
Review Comment:
This method does not appear to be used, can it be removed?
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutEmail.java:
##########
@@ -523,4 +555,57 @@ private InternetAddress[] toInetAddresses(final ProcessContext context, final Fl
protected void send(final Message msg) throws MessagingException {
Transport.send(msg);
}
+
+ /**
+ * Utility function to return a Map of optional {@code mail._____} properties that were set dynamically.
+ * @param context the ProcessContext
+ * @return a Map with optional {@code mail._____} properties
+ */
+ private Map<String, String> getDynamicMailProperties(final ProcessContext context) {
+ final Map<String, String> dynamicMailProperties = new HashMap<>();
+
+ for (final Map.Entry<PropertyDescriptor, String> entry: context.getProperties().entrySet()) {
+ final PropertyDescriptor descriptor = entry.getKey();
+ if (descriptor.isDynamic()) {
+ final String name = descriptor.getName();
+ final Matcher matcher = MAIL_PROPERTY_PATTERN.matcher(name);
+ if (matcher.matches()) {
+ dynamicMailProperties.put(name, entry.getValue());
+ }
+ }
+ }
+
+ return dynamicMailProperties;
+ }
+
+ private static class DynamicMailPropertyValidator implements Validator {
+ @Override
+ public ValidationResult validate(String subject, String input, ValidationContext context) {
+ final Matcher matcher = MAIL_PROPERTY_PATTERN.matcher(subject);
+ if (!matcher.matches()) {
+ return new ValidationResult.Builder()
+ .input(input)
+ .subject(subject)
+ .valid(false)
+ .explanation(String.format("[%s] is an invalid mail._____ property", subject))
+ .build();
+ }
+
+ if (propertyToContext.containsKey(subject)) {
+ return new ValidationResult.Builder()
+ .input(input)
+ .subject(subject)
+ .valid(false)
+ .explanation(String.format("[%s] overwrites required existing properties", subject))
Review Comment:
```suggestion
.explanation(String.format("[%s] overwrites standard properties", subject))
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestPutEmail.java:
##########
@@ -299,4 +299,51 @@ public void testOutgoingMessageWithFlowfileContent() throws Exception {
assertEquals("anotherbcc@apache.org", message.getRecipients(RecipientType.BCC)[1].toString());
}
+ @Test
+ public void testValidDynamicMailProperties() throws Exception {
+ runner.setProperty(PutEmail.SMTP_HOSTNAME, "smtp-host");
+ runner.setProperty(PutEmail.HEADER_XMAILER, "TestingNiFi");
+ runner.setProperty(PutEmail.FROM, "test@apache.org,from@apache.org");
+ runner.setProperty(PutEmail.MESSAGE, "${body}");
+ runner.setProperty(PutEmail.TO, "recipient@apache.org,another@apache.org");
+ runner.setProperty(PutEmail.CC, "recipientcc@apache.org,anothercc@apache.org");
+ runner.setProperty(PutEmail.BCC, "recipientbcc@apache.org,anotherbcc@apache.org");
+ runner.setProperty(PutEmail.CONTENT_AS_MESSAGE, "${sendContent}");
Review Comment:
Instead of duplicating all of these setProperty calls, recommend creating a shared method named `setRequiredProperties()` and calling in each test 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