[GitHub] [nifi] nandorsoma commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"

[GitBox <gi...@apache.org>]

nandorsoma commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r750791129



##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -192,6 +220,48 @@ private void addAttribute(final Map<String, String> attributes, final String att
         attributes.put(attributeName, value.toString());
     }
 
+    private String buildHeaders(Map<String, Object> headers, boolean escapeComma, boolean removeCurlyBraces) {
+        if (headers == null) {
+            return null;
+        }
+        if (escapeComma && removeCurlyBraces) {
+            return headers.keySet().stream()
+                    .map(key -> key + "=" + escapeString(headers.get(key).toString()))
+                    .collect(Collectors.joining(", "));
+        } else if (escapeComma) {
+            return headers.keySet().stream()
+                    .map(key -> key + "=" + escapeString(headers.get(key).toString()))
+                    .collect(Collectors.joining(", ", "{", "}"));
+        } else if (removeCurlyBraces) {
+            String headerString = headers.toString();
+            return headerString.substring(1, headerString.length() - 1);

Review comment:
       You could make this part a bit fault tolerant by removing the first and last characters only if they are really curly braces. What do you think?

##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java
##########
@@ -99,6 +101,15 @@
             .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
             .build();
 
+    static final PropertyDescriptor UNESCAPE_COMMA = new PropertyDescriptor.Builder()

Review comment:
       I was a bit confused about the name of the property. The goal is to escape a comma in the header so the processor won't threat that comma in the header as a separator. Wouldn't IGNORE_ESCAPED_COMMA_IN_HEADER reflect that intention better?

##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -192,6 +220,48 @@ private void addAttribute(final Map<String, String> attributes, final String att
         attributes.put(attributeName, value.toString());
     }
 
+    private String buildHeaders(Map<String, Object> headers, boolean escapeComma, boolean removeCurlyBraces) {
+        if (headers == null) {
+            return null;
+        }
+        if (escapeComma && removeCurlyBraces) {
+            return headers.keySet().stream()
+                    .map(key -> key + "=" + escapeString(headers.get(key).toString()))

Review comment:
       On the publish side, based on the property, the intention is to allow comma in the header. It doesn't specify that we want to allow it in the key or in the value. Now I'm able to put an escaped comma into the key on the Publisher side, but here only the key will be escaped.
   So, while on the publisher side the property looks like that:
   ```
   amqp$headers=foo\,foo=(bar\,bar)
   ```
   After processing in the consumer's success flowFile it will look like that:
   ```
   amqp$headers=foo,foo=(bar\,bar)
   ```
   I think you either need to specify that escaped comma is only allowed in the value or you need to escape the key here. I prefer the first one, because comma in the key is undesirable I think.

##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -99,6 +103,28 @@
         .required(true)
         .build();
 
+    static final PropertyDescriptor ESCAPE_COMMA = new PropertyDescriptor.Builder()
+        .name("escape.comma")
+        .displayName("Escape Comma")
+        .description("When there is a comma in the header itself, with the help of this parameter, the header's own commas are escaped. "
+             + "When this parameter is selected true, the Unescape Comma parameter in PublishAMQP processor must be set to true ")

Review comment:
       Based on the second sentence for the first sight it seems like this feature is only working between NiFi publishers and consumers. What happens when NiFi consumes a message that is produced by a different publisher? Is it a valid scenario?




-- 
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