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/10/14 11:32:52 UTC
[GitHub] [nifi] sedadgn opened a new pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
sedadgn opened a new pull request #5458:
URL: https://github.com/apache/nifi/pull/5458
This PR introduces 2 new properties for the ConsumeAMQP processor: escape.comma and remove.curly.braces.
And one new property for PublishAMQP: unescape.comma
This allows to configure the processors to use escaping for commas and to consistently not use curly braces in the amqp$header attribute.
The default values ensure backwards compatibility.
<!--
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
_Enables X functionality; fixes bug NIFI-YYYY._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] 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.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] 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:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r738035135
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,27 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ /**
+ * Escape {@code str} by replacing occurrences of {@code charToEscape} with {@code escapeChar+charToEscape}
+ * @param str the input string
+ * @param escapeChar the character used for escaping
+ * @param charToEscape the character that needs to be escaped
+ * @return the escaped string
+ */
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null || str.isEmpty()) {
+ return null;
+ }
+ StringBuilder result = new StringBuilder();
+ for (int i=0; i<str.length(); i++) {
+ char curChar = str.charAt(i);
+ if (curChar == escapeChar || curChar == charToEscape) {
+ // special char
+ result.append(escapeChar);
+ }
+ result.append(curChar);
Review comment:
Ottobackwards said the same thing. But we thought, that we must produce always the original string when we make unescape in publishAMQP. But if you want, we can add the check.
--
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] kevdoran commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r737576297
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,27 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ /**
+ * Escape {@code str} by replacing occurrences of {@code charToEscape} with {@code escapeChar+charToEscape}
+ * @param str the input string
+ * @param escapeChar the character used for escaping
+ * @param charToEscape the character that needs to be escaped
+ * @return the escaped string
+ */
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null || str.isEmpty()) {
+ return null;
+ }
+ StringBuilder result = new StringBuilder();
+ for (int i=0; i<str.length(); i++) {
+ char curChar = str.charAt(i);
+ if (curChar == escapeChar || curChar == charToEscape) {
+ // special char
+ result.append(escapeChar);
+ }
+ result.append(curChar);
Review comment:
Minor, but it would be nice if this utility method were idempotent, meaning that if I pass it a string that contains some already escaped characters, it has not effect on those. In other words, all of these invocations would return the same result:
```
StringUtils.escapeString("key=(value0,value1,value2)", '\\', ',');
StringUtils.escapeString("key=(value0\\,value1,value2)", '\\', ',');
StringUtils.escapeString("key=(value0\\,value1\\,value2)", '\\', ',');
// All of the above return "key=(value\\,value1\\,value2)"
```
--
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] kevdoran commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r784106766
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/processor/util/StandardValidators.java
##########
@@ -980,4 +980,25 @@ public ValidationResult validate(final String subject, final String value, final
return new ValidationResult.Builder().subject(subject).input(value).explanation(reason).valid(reason == null).build();
}
}
+
+ public static final Validator SINGLE_CHAR_VALIDATOR = (subject, input, context) -> {
Review comment:
I've never worked in this file, but at a glance it seems the intent it to keep all the static validators together in one section at the top of the file, so would you mind relocating this?
--
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 change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by 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
[GitHub] [nifi] nandorsoma commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r752041780
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -192,6 +221,54 @@ 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();
+ if(headerString.startsWith("{")){
Review comment:
Thank you for changing this. I hope it's not nitpicking but I think it would be better to check for ending and closing characters together. This way it's much more consistent and you can also spare a substring call by using the original one. So, something like:
```
if (headerString.startsWith("{") && headerString.endsWith("}")) {
return headerString.substring(1, headerString.length() - 1);
}
```
Another option would be to construct the string here like in line 229 without escaping. This way it would be consistent code wise that we are only relying on the Map.toString() in the original/compatible mode.
--
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] sedadgn commented on pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-997644212
> I should be able to do a final round of review on this today or tomorrow, and assuming everything looks good, can merge.
Hi Kevin, have you had a chance to review? I wanted to remind.
Thank 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] kevdoran commented on pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran commented on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-987222571
I should be able to do a final round of review on this today or tomorrow, and assuming everything looks good, can merge.
--
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] sedadgn commented on pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-977871567
> Hi @sedadgn, thanks again for this contribution. I re-reviewed based on the latest approach and have some feedback. If you are able to make another round of updates, I think this is close to being able to merge. Thanks!
Hi @kevdoran, thanks for your feedback. I implemented these feedbacks. Thank 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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r751283372
##########
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:
You are right. I changed the parameter names to ESCAPE_COMMA_VALUE_IN_HEADER and UNESCAPE_COMMA_VALUE_IN_HEADER.
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r736470809
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,20 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
Review comment:
I am unsure how to do that. The method does not replace, just add a character. I added a javadoc to explain the parameters instead. Maybe that helps!
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r740815019
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,27 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ /**
+ * Escape {@code str} by replacing occurrences of {@code charToEscape} with {@code escapeChar+charToEscape}
+ * @param str the input string
+ * @param escapeChar the character used for escaping
+ * @param charToEscape the character that needs to be escaped
+ * @return the escaped string
+ */
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null || str.isEmpty()) {
+ return null;
+ }
+ StringBuilder result = new StringBuilder();
+ for (int i=0; i<str.length(); i++) {
+ char curChar = str.charAt(i);
+ if (curChar == escapeChar || curChar == charToEscape) {
+ // special char
+ result.append(escapeChar);
+ }
+ result.append(curChar);
Review comment:
I moved in ConsumeAmqp.java.Because only this class use the 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 change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r757705556
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java
##########
@@ -240,20 +254,20 @@ private BasicProperties extractAmqpPropertiesFromFlowFile(FlowFile flowFile) {
updateBuilderFromAttribute(flowFile, "userId", builder::userId);
updateBuilderFromAttribute(flowFile, "appId", builder::appId);
updateBuilderFromAttribute(flowFile, "clusterId", builder::clusterId);
- updateBuilderFromAttribute(flowFile, "headers", headers -> builder.headers(validateAMQPHeaderProperty(headers)));
+ updateBuilderFromAttribute(flowFile, "headers", headers -> builder.headers(validateAMQPHeaderProperty(headers,headerSeparator)));
return builder.build();
}
/**
* Will validate if provided amqpPropValue can be converted to a {@link Map}.
- * Should be passed in the format: amqp$headers=key=value,key=value etc.
- *
+ * Should be passed in the format: amqp$headers=key=value
+ * @param splitValue is used to split for property
* @param amqpPropValue the value of the property
* @return {@link Map} if valid otherwise null
*/
- private Map<String, Object> validateAMQPHeaderProperty(String amqpPropValue) {
- String[] strEntries = amqpPropValue.split(",");
+ private Map<String, Object> validateAMQPHeaderProperty(String amqpPropValue,Character splitValue) {
+ String[] strEntries = amqpPropValue.split(Pattern.quote(String.valueOf(splitValue)));
Review comment:
I see, it makes sense. Thanks for including the example, I've learned something new. :)
--
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] kevdoran commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r737593262
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -184,6 +211,26 @@ protected void processResource(final Connection connection, final AMQPConsumer c
return attributes;
}
+ 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 + "=" + StringUtils.escapeString(headers.get(key).toString(), StringUtils.ESCAPE_CHAR, StringUtils.COMMA))
+ .collect(Collectors.joining(", "));
+ } else if (escapeComma) {
+ return headers.keySet().stream()
+ .map(key -> key + "=" + StringUtils.escapeString(headers.get(key).toString(), StringUtils.ESCAPE_CHAR, StringUtils.COMMA))
+ .collect(Collectors.joining(", ", "{", "}"));
+ } else if (removeCurlyBraces) {
+ String headerString = headers.toString();
+ return headerString.substring(1, headerString.length() - 1);
Review comment:
IMO, if `removeCurlyBraces==true`, but the headerString is not wrapped in curlyBraces, it should be a no-op. In other words, if someone enabled `removeCurlyBraces`, but the header string does not contain leading/trailing braces, the header string should be unaltered.
--
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] ottobackwards commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r735700480
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,20 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null) {
Review comment:
This check should be null or empty so that we don't do extra work for empty strings.
pseudo
```
if str is null or empty
return str
```
--
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] ottobackwards commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r735702744
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,20 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null) {
Review comment:
Shouldn't this check for already escaped chars by checking str.charAt(i -1) != escapeChar?
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r740815019
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,27 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ /**
+ * Escape {@code str} by replacing occurrences of {@code charToEscape} with {@code escapeChar+charToEscape}
+ * @param str the input string
+ * @param escapeChar the character used for escaping
+ * @param charToEscape the character that needs to be escaped
+ * @return the escaped string
+ */
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null || str.isEmpty()) {
+ return null;
+ }
+ StringBuilder result = new StringBuilder();
+ for (int i=0; i<str.length(); i++) {
+ char curChar = str.charAt(i);
+ if (curChar == escapeChar || curChar == charToEscape) {
+ // special char
+ result.append(escapeChar);
+ }
+ result.append(curChar);
Review comment:
I moved in ConsumeAmqp.java.Because only this class use the 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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r736481331
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,20 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null) {
Review comment:
The idea was, that escape followed by unescape always produces the original string. E.g. unescape(escape("a,b" )) == "a,b". If the original string is already escaped, then the end result is an escaped string. E.g. unescape(escape("a,\\,b" )) == "a,\\,b". That would not work if we perform the check you suggested.
We can add the check if you want, to. For our specific problems, it would work either way.
--
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] kevdoran commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r737576297
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,27 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ /**
+ * Escape {@code str} by replacing occurrences of {@code charToEscape} with {@code escapeChar+charToEscape}
+ * @param str the input string
+ * @param escapeChar the character used for escaping
+ * @param charToEscape the character that needs to be escaped
+ * @return the escaped string
+ */
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null || str.isEmpty()) {
+ return null;
+ }
+ StringBuilder result = new StringBuilder();
+ for (int i=0; i<str.length(); i++) {
+ char curChar = str.charAt(i);
+ if (curChar == escapeChar || curChar == charToEscape) {
+ // special char
+ result.append(escapeChar);
+ }
+ result.append(curChar);
Review comment:
Minor, but it would be nice if this utility method were idempotent, meaning that if I pass it a string that contains some already escaped characters, it has not effect on those. In other words, all of these invocations would return the same result:
```
StringUtils.escapeString("key=(value0,value1,value2)", '\\', ',');
StringUtils.escapeString("key=(value0\\,value1,value2)", '\\', ',');
StringUtils.escapeString("key=(value0\\,value1\\,value2)", '\\', ',');
```
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r757453945
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java
##########
@@ -240,20 +254,20 @@ private BasicProperties extractAmqpPropertiesFromFlowFile(FlowFile flowFile) {
updateBuilderFromAttribute(flowFile, "userId", builder::userId);
updateBuilderFromAttribute(flowFile, "appId", builder::appId);
updateBuilderFromAttribute(flowFile, "clusterId", builder::clusterId);
- updateBuilderFromAttribute(flowFile, "headers", headers -> builder.headers(validateAMQPHeaderProperty(headers)));
+ updateBuilderFromAttribute(flowFile, "headers", headers -> builder.headers(validateAMQPHeaderProperty(headers,headerSeparator)));
return builder.build();
}
/**
* Will validate if provided amqpPropValue can be converted to a {@link Map}.
- * Should be passed in the format: amqp$headers=key=value,key=value etc.
- *
+ * Should be passed in the format: amqp$headers=key=value
+ * @param splitValue is used to split for property
* @param amqpPropValue the value of the property
* @return {@link Map} if valid otherwise null
*/
- private Map<String, Object> validateAMQPHeaderProperty(String amqpPropValue) {
- String[] strEntries = amqpPropValue.split(",");
+ private Map<String, Object> validateAMQPHeaderProperty(String amqpPropValue,Character splitValue) {
+ String[] strEntries = amqpPropValue.split(Pattern.quote(String.valueOf(splitValue)));
Review comment:
Pattern.quote create the escaped version of the regex representing.
For Example '|'.
Also you can look at this.
https://stackoverflow.com/questions/10796160/splitting-a-java-string-by-the-pipe-symbol-using-split
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/test/java/org/apache/nifi/amqp/processors/ConsumeAMQPTest.java
##########
@@ -161,6 +165,138 @@ public void validateSuccessfulConsumeAndTransferToSuccess() throws Exception {
}
}
+ @Test
+ public void validateHeaderWithValueSeparatorForHeaderParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("foo1","bar,bar");
+ headersMap.put("foo2","bar,bar");
+
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.HEADER_SEPARATOR, "|");
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ String headers = successFF.getAttribute("amqp$headers");
+ Map<String, String> properties = Splitter.on("|").withKeyValueSeparator("=").split(headers.substring(1,headers.length()-1));
+ Assert.assertEquals(headersMap,properties);
+
+ }
+ }
+ @Test
+ public void validateWithNotValidHeaderSeparatorParameter() {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.HEADER_SEPARATOR, "|,");
+ runner.assertNotValid();
+ }
+
+ @Test
+ public void validateHeaderWithRemoveCurlyBracesParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("key1","(bar,bar)");
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.REMOVE_CURLY_BRACES,"True");
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ successFF.assertAttributeEquals("amqp$headers", "key1=(bar,bar)");
+
+ }
+ }
+
+ @Test
+ public void validateHeaderWithRemoveCurlyBracesAndValueSeparatorForHeaderParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("key1","(bar,bar)");
+ headersMap.put("key2","(bar,bar)");
+
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.REMOVE_CURLY_BRACES,"True");
+ runner.setProperty(ConsumeAMQP.HEADER_SEPARATOR,"|");
+
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ String headers = successFF.getAttribute("amqp$headers");
+ Map<String, String> properties = Splitter.on("|").withKeyValueSeparator("=").split(headers);
+ Assert.assertEquals(headersMap,properties);
+
+ }
+ }
+
+ @Test
+ public void validateHeaderWithoutParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("key1","bar");
+ headersMap.put("key2","bar2");
+
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ String headers = successFF.getAttribute("amqp$headers");
+ Map<String, String> properties = Splitter.on(",").withKeyValueSeparator("=").split(headers.substring(1,headers.length()-1));
Review comment:
I can not. Because there are two elements in the Hashmap. I don't know in what order these elements will come.
For HashMap "This class makes no guarantees as to the order of the map; in particular, it does not guarantee that the order will remain constant over time."
--
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 change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r757705648
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/test/java/org/apache/nifi/amqp/processors/ConsumeAMQPTest.java
##########
@@ -161,6 +165,138 @@ public void validateSuccessfulConsumeAndTransferToSuccess() throws Exception {
}
}
+ @Test
+ public void validateHeaderWithValueSeparatorForHeaderParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("foo1","bar,bar");
+ headersMap.put("foo2","bar,bar");
+
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.HEADER_SEPARATOR, "|");
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ String headers = successFF.getAttribute("amqp$headers");
+ Map<String, String> properties = Splitter.on("|").withKeyValueSeparator("=").split(headers.substring(1,headers.length()-1));
+ Assert.assertEquals(headersMap,properties);
+
+ }
+ }
+ @Test
+ public void validateWithNotValidHeaderSeparatorParameter() {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.HEADER_SEPARATOR, "|,");
+ runner.assertNotValid();
+ }
+
+ @Test
+ public void validateHeaderWithRemoveCurlyBracesParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("key1","(bar,bar)");
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.REMOVE_CURLY_BRACES,"True");
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ successFF.assertAttributeEquals("amqp$headers", "key1=(bar,bar)");
+
+ }
+ }
+
+ @Test
+ public void validateHeaderWithRemoveCurlyBracesAndValueSeparatorForHeaderParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("key1","(bar,bar)");
+ headersMap.put("key2","(bar,bar)");
+
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.REMOVE_CURLY_BRACES,"True");
+ runner.setProperty(ConsumeAMQP.HEADER_SEPARATOR,"|");
+
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ String headers = successFF.getAttribute("amqp$headers");
+ Map<String, String> properties = Splitter.on("|").withKeyValueSeparator("=").split(headers);
+ Assert.assertEquals(headersMap,properties);
+
+ }
+ }
+
+ @Test
+ public void validateHeaderWithoutParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("key1","bar");
+ headersMap.put("key2","bar2");
+
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ String headers = successFF.getAttribute("amqp$headers");
+ Map<String, String> properties = Splitter.on(",").withKeyValueSeparator("=").split(headers.substring(1,headers.length()-1));
Review comment:
I see, it makes sense!
--
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 #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-987210197
> > Thanks for working through the feedback @sedadgn, the current version looks good.
> > What do you think @kevdoran and @ottobackwards?
>
> Hello @nandorsoma, @ottobackwards and @kevdoran, Thank you for your feedback and reviews. We need this feature in our project. If everything works for you, can you merge? Thank you
Hey @sedadgn !
Unfortunately I haven't got permission to merge, therefore we need to wait for a committer to do that. Will try to ping someone!
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r736481331
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,20 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null) {
Review comment:
The idea was, that escape followed by unescape always produces the original string. E.g. unescape(escape("a,b" )) == "a,b". If the original string is already escaped, then the end result is an escaped string. E.g. unescape(escape("a,\\,b" )) == "a,\,b". That would not work if we perform the check you suggested.
We can add the check if you want, to. For our specific problems, it would work either way.
--
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] kevdoran commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r755463922
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -28,6 +28,8 @@
public class StringUtils {
public static final String EMPTY = "";
+ //With this regular expression, commas without "\"-escape in string can be found.
+ public static final String REGEX_COMMA_WITHOUT_ESCAPE = "(?<!\\\\),";
Review comment:
Is this still used with the new approach? If not, let's remove it.
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java
##########
@@ -240,20 +254,20 @@ private BasicProperties extractAmqpPropertiesFromFlowFile(FlowFile flowFile) {
updateBuilderFromAttribute(flowFile, "userId", builder::userId);
updateBuilderFromAttribute(flowFile, "appId", builder::appId);
updateBuilderFromAttribute(flowFile, "clusterId", builder::clusterId);
- updateBuilderFromAttribute(flowFile, "headers", headers -> builder.headers(validateAMQPHeaderProperty(headers)));
+ updateBuilderFromAttribute(flowFile, "headers", headers -> builder.headers(validateAMQPHeaderProperty(headers,escapeComma)));
return builder.build();
}
/**
* Will validate if provided amqpPropValue can be converted to a {@link Map}.
- * Should be passed in the format: amqp$headers=key=value,key=value etc.
- *
+ * Should be passed in the format: amqp$headers=key=value
+ * With unescape comma parameter, header values may contain escaped commas. The header value is unescaped and then published.
* @param amqpPropValue the value of the property
* @return {@link Map} if valid otherwise null
*/
Review comment:
It appears this JavaDoc no longer matches the method signature of the updated approach.
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java
##########
@@ -345,4 +344,14 @@ public void handleUnexpectedConnectionDriverException(Connection conn, Throwable
throw new IllegalStateException("Failed to establish connection with AMQP Broker: " + cf.toString(), e);
}
}
+
+ protected Character getValueSeparatorChar(String value,PropertyDescriptor valueSeparatorForHeader) {
+ if (value!=null && value.length()==1) {
+ return value.charAt(0);
+ }
+
+ getLogger().warn("'{}' property evaluated to an invalid value: \"{}\". It must be a single character. The property value will be ignored.", valueSeparatorForHeader.getName(), value);
Review comment:
I'm a bit concerned that an invalid configuration could be hidden until runtime and end up filling the logs with these warnings, as it looks like every AMQP message would result in this warning.
IMO, a better approach would be to write a custom validator to use in the property descriptor.
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java
##########
@@ -224,7 +238,7 @@ private void updateBuilderFromAttribute(final FlowFile flowFile, final String at
* {@link AMQPUtils#validateAMQPPriorityProperty}
* {@link AMQPUtils#validateAMQPTimestampProperty}
*/
- private BasicProperties extractAmqpPropertiesFromFlowFile(FlowFile flowFile) {
+ private BasicProperties extractAmqpPropertiesFromFlowFile(FlowFile flowFile,Character escapeComma) {
Review comment:
Is the `escapeComma` parameter name a leftover from the previous approach? I think this is intended to be called something like `headerSeparator`.
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/pom.xml
##########
@@ -48,6 +50,16 @@ language governing permissions and limitations under the License. -->
<version>1.15.0-SNAPSHOT</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-text</artifactId>
+ <version>${common-text.version}</version>
+ </dependency>
Review comment:
I don't see where this is used. Is this added `commons-text` dependency still needed?
--
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 change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r757035734
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java
##########
@@ -240,20 +254,20 @@ private BasicProperties extractAmqpPropertiesFromFlowFile(FlowFile flowFile) {
updateBuilderFromAttribute(flowFile, "userId", builder::userId);
updateBuilderFromAttribute(flowFile, "appId", builder::appId);
updateBuilderFromAttribute(flowFile, "clusterId", builder::clusterId);
- updateBuilderFromAttribute(flowFile, "headers", headers -> builder.headers(validateAMQPHeaderProperty(headers)));
+ updateBuilderFromAttribute(flowFile, "headers", headers -> builder.headers(validateAMQPHeaderProperty(headers,headerSeparator)));
return builder.build();
}
/**
* Will validate if provided amqpPropValue can be converted to a {@link Map}.
- * Should be passed in the format: amqp$headers=key=value,key=value etc.
- *
+ * Should be passed in the format: amqp$headers=key=value
+ * @param splitValue is used to split for property
* @param amqpPropValue the value of the property
* @return {@link Map} if valid otherwise null
*/
- private Map<String, Object> validateAMQPHeaderProperty(String amqpPropValue) {
- String[] strEntries = amqpPropValue.split(",");
+ private Map<String, Object> validateAMQPHeaderProperty(String amqpPropValue,Character splitValue) {
+ String[] strEntries = amqpPropValue.split(Pattern.quote(String.valueOf(splitValue)));
Review comment:
Maybe I don't know something, but why do you use Pattern.quote here, instead of `amqpPropValue.split(String.valueOf(splitValue))`?
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -99,6 +101,28 @@
.required(true)
.build();
+ public static final PropertyDescriptor HEADER_SEPARATOR = new PropertyDescriptor.Builder()
+ .name("header.separator")
+ .displayName("Header Separator")
+ .description("The character that is used to separate key-value for header in String. The value must only one character. "
+ + "Otherwise it will be skipped and the default header separator(',') will be used."
Review comment:
Are we sure that it will be skipped? I think because of the validator an error message will be shown for the user and you won't be able to start the processor. Same applies for PublishAMQP processor.
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/test/java/org/apache/nifi/amqp/processors/ConsumeAMQPTest.java
##########
@@ -161,6 +165,138 @@ public void validateSuccessfulConsumeAndTransferToSuccess() throws Exception {
}
}
+ @Test
+ public void validateHeaderWithValueSeparatorForHeaderParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("foo1","bar,bar");
+ headersMap.put("foo2","bar,bar");
+
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.HEADER_SEPARATOR, "|");
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ String headers = successFF.getAttribute("amqp$headers");
+ Map<String, String> properties = Splitter.on("|").withKeyValueSeparator("=").split(headers.substring(1,headers.length()-1));
+ Assert.assertEquals(headersMap,properties);
+
+ }
+ }
+ @Test
+ public void validateWithNotValidHeaderSeparatorParameter() {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.HEADER_SEPARATOR, "|,");
+ runner.assertNotValid();
+ }
+
+ @Test
+ public void validateHeaderWithRemoveCurlyBracesParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("key1","(bar,bar)");
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.REMOVE_CURLY_BRACES,"True");
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ successFF.assertAttributeEquals("amqp$headers", "key1=(bar,bar)");
+
+ }
+ }
+
+ @Test
+ public void validateHeaderWithRemoveCurlyBracesAndValueSeparatorForHeaderParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("key1","(bar,bar)");
+ headersMap.put("key2","(bar,bar)");
+
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+ runner.setProperty(ConsumeAMQP.REMOVE_CURLY_BRACES,"True");
+ runner.setProperty(ConsumeAMQP.HEADER_SEPARATOR,"|");
+
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ String headers = successFF.getAttribute("amqp$headers");
+ Map<String, String> properties = Splitter.on("|").withKeyValueSeparator("=").split(headers);
+ Assert.assertEquals(headersMap,properties);
+
+ }
+ }
+
+ @Test
+ public void validateHeaderWithoutParameterConsumeAndTransferToSuccess() throws Exception {
+ final Map<String, List<String>> routingMap = Collections.singletonMap("key1", Arrays.asList("queue1", "queue2"));
+ final Map<String, String> exchangeToRoutingKeymap = Collections.singletonMap("myExchange", "key1");
+ final Map<String, Object> headersMap = new HashMap<>();
+ headersMap.put("key1","bar");
+ headersMap.put("key2","bar2");
+
+ AMQP.BasicProperties.Builder builderBasicProperties = new AMQP.BasicProperties.Builder();
+ builderBasicProperties.headers(headersMap);
+
+ final Connection connection = new TestConnection(exchangeToRoutingKeymap, routingMap);
+
+ try (AMQPPublisher sender = new AMQPPublisher(connection, mock(ComponentLog.class))) {
+ sender.publish("hello".getBytes(), builderBasicProperties.build(), "key1", "myExchange");
+
+ ConsumeAMQP proc = new LocalConsumeAMQP(connection);
+ TestRunner runner = initTestRunner(proc);
+
+ runner.run();
+ final MockFlowFile successFF = runner.getFlowFilesForRelationship(PublishAMQP.REL_SUCCESS).get(0);
+ assertNotNull(successFF);
+ successFF.assertAttributeEquals("amqp$routingKey", "key1");
+ successFF.assertAttributeEquals("amqp$exchange", "myExchange");
+ String headers = successFF.getAttribute("amqp$headers");
+ Map<String, String> properties = Splitter.on(",").withKeyValueSeparator("=").split(headers.substring(1,headers.length()-1));
Review comment:
I think you can hardcode the expectation here, it would be more visible what is the expected result. I understand that if get back the initial map, then the header is valid but in the case of that processor the output is a String therefore you can assert that. What do you think?
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -99,6 +101,28 @@
.required(true)
.build();
+ public static final PropertyDescriptor HEADER_SEPARATOR = new PropertyDescriptor.Builder()
+ .name("header.separator")
+ .displayName("Header Separator")
+ .description("The character that is used to separate key-value for header in String. The value must only one character. "
+ + "Otherwise it will be skipped and the default header separator(',') will be used."
+ + "The value of this parameter must be same to the value of parameter in PublishAMQP, when you use the PublishAMQP processor" )
Review comment:
I think this is true in one case from several use cases and because of that maybe it could cause confusion therefore I'd remove that sentence. What do you think? Same applies for PublishAMQP processor.
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/pom.xml
##########
@@ -21,6 +21,8 @@ language governing permissions and limitations under the License. -->
<properties>
<amqp-client.version>5.8.0</amqp-client.version>
+ <common-text.version>1.8</common-text.version>
Review comment:
Is common-text is used somewhere? I don't find it.
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/processor/util/StandardValidators.java
##########
@@ -980,4 +980,35 @@ public ValidationResult validate(final String subject, final String value, final
return new ValidationResult.Builder().subject(subject).input(value).explanation(reason).valid(reason == null).build();
}
}
+
+ public static final Validator SINGLE_CHAR_VALIDATOR = (subject, input, context) -> {
+ if (input == null) {
+ return new ValidationResult.Builder()
+ .input(input)
+ .subject(subject)
+ .valid(false)
+ .explanation("Input is null for this property")
+ .build();
+ }
+
+ if (input.isEmpty()) {
Review comment:
I think you can remove that if, because when the input is empty it will produce the same message that you generate in case of `input.length() != 1`.
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -192,6 +219,21 @@ private void addAttribute(final Map<String, String> attributes, final String att
attributes.put(attributeName, value.toString());
}
+ private String buildHeaders(Map<String, Object> headers, boolean removeCurlyBraces,Character valueSeparatorForHeaders) {
+ if (headers == null) {
+ return null;
+ }
+ String headerString = Joiner.on(valueSeparatorForHeaders).withKeyValueSeparator("=").join(headers);
+
+ if (!removeCurlyBraces) {
Review comment:
I like this approach, much more explicit. Tbh on the first review I didn't even understand how the curly braces come into play, then I realized that it was appended by Map.toString(). (I think that was a mistake in the initial implementation)
--
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] sedadgn commented on pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-975207345
> Hey @sedadgn! I've found one thing among the new changes after that I think we can wrap the things up.
>
> Another thing. During the 2. review, an alternative solution came to my mind. Couldn't we spare the escaping part by making the separator itself configurable? That one feels a bit more robust for me. There is an example for that: https://nifi.apache.org/docs/nifi-docs/components/nifi-docs/components/org.apache.nifi/nifi-poi-nar/1.9.0/org.apache.nifi.processors.poi.ConvertExcelToCSVProcessor/index.html I understand that the PR has been open for more than a month now so I don't ask you to change the current behavior - unless you strongly agree -, just wanted to share that idea.
Hello @nandorsoma
I think the solution is better. Strongly I agree :). I will implement. But I will need some time.
Thank 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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r751279398
##########
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:
Our scenario is as follows:
our publisher -> nifi consumer -> transform-> nifi publisher -> our consumer
so now it can work with different publishers. It can also work on its own when no changes are made to the newly added parameters. But when the ESCAPE_COMMA_VALUE_IN_HEADER parameter in ConsumeAMQP is true, the UNESCAPE_COMMA_VALUE_IN_HEADER parameter in PublishAMQP must also be true to get the comma value as it is entered.
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r751282543
##########
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:
in PublishAMQP According to this parameter, I unescape the escaped value from ConsumeAMQP. I changed the description. I hope it is more understandable.
--
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] sedadgn edited a comment on pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn edited a comment on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-997644212
> I should be able to do a final round of review on this today or tomorrow, and assuming everything looks good, can merge.
Hi Kevin @kevdoran , have you had a chance to review? I wanted to remind.
Thank 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 a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r752041780
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -192,6 +221,54 @@ 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();
+ if(headerString.startsWith("{")){
Review comment:
Thank you for changing this. I hope it's not nitpicking but I think it would be better to check for ending and closing characters together. This way it's much more consistent and you can also spare a substring call by using the original one. So, something like:
```
if (headerString.startsWith("{") && headerString.endsWith("}")) {
return headerString.substring(1, headerString.length() - 1);
```
Another option would be to construct the string here like in line 229 without escaping. This way it would be consistent code wise that we are only relying on the Map.toString() in the original/compatible mode.
--
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] kevdoran closed pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran closed pull request #5458:
URL: https://github.com/apache/nifi/pull/5458
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r784726662
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/processor/util/StandardValidators.java
##########
@@ -980,4 +980,25 @@ public ValidationResult validate(final String subject, final String value, final
return new ValidationResult.Builder().subject(subject).input(value).explanation(reason).valid(reason == null).build();
}
}
+
+ public static final Validator SINGLE_CHAR_VALIDATOR = (subject, input, context) -> {
Review comment:
I moved the 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] kevdoran commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r737593262
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -184,6 +211,26 @@ protected void processResource(final Connection connection, final AMQPConsumer c
return attributes;
}
+ 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 + "=" + StringUtils.escapeString(headers.get(key).toString(), StringUtils.ESCAPE_CHAR, StringUtils.COMMA))
+ .collect(Collectors.joining(", "));
+ } else if (escapeComma) {
+ return headers.keySet().stream()
+ .map(key -> key + "=" + StringUtils.escapeString(headers.get(key).toString(), StringUtils.ESCAPE_CHAR, StringUtils.COMMA))
+ .collect(Collectors.joining(", ", "{", "}"));
+ } else if (removeCurlyBraces) {
+ String headerString = headers.toString();
+ return headerString.substring(1, headerString.length() - 1);
Review comment:
IMO, if `removeCurlyBraces==true`, but the headerString is not wrapped in curlyBraces, it should be a no-op. In other words, if someone enabled `removeCurlyBraces`, but the header value string does not contain leading/trailing braces, the header value string should be unaltered.
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r736481331
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,20 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null) {
Review comment:
The idea was, that escape followed by unescape always produces the original string. E.g. unescape(escape("a,b" )) == "a,b". If the original string is already escaped, then the end result is an escaped string. E.g. unescape(escape("a,\,b" )) == "a,\,b". That would not work if we perform the check you suggested.
We can add the check if you want, to. For our specific problems, it would work either way.
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r736476239
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -99,6 +102,28 @@
.required(true)
.build();
Review comment:
The comma is special, because it is used as the separator (for join in ConsumeAMQP and for split in PublishAMQP).
Curly braces were added by ConsumeAMQP as first and last character.
No other characters have special meaning.
--
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] ottobackwards commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r735704390
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,20 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null) {
Review comment:
There should be a new test for this function in the stringutils test
--
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] ottobackwards commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r735708591
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java
##########
@@ -99,6 +102,28 @@
.required(true)
.build();
Review comment:
Is there a part of the specification that can be referenced about chars that *should* be replaced? Are these the only two, or the only two that you've seen?
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r740815019
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,27 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ /**
+ * Escape {@code str} by replacing occurrences of {@code charToEscape} with {@code escapeChar+charToEscape}
+ * @param str the input string
+ * @param escapeChar the character used for escaping
+ * @param charToEscape the character that needs to be escaped
+ * @return the escaped string
+ */
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null || str.isEmpty()) {
+ return null;
+ }
+ StringBuilder result = new StringBuilder();
+ for (int i=0; i<str.length(); i++) {
+ char curChar = str.charAt(i);
+ if (curChar == escapeChar || curChar == charToEscape) {
+ // special char
+ result.append(escapeChar);
+ }
+ result.append(curChar);
Review comment:
I moved in ConsumeAmqp.java.Because only this class use the 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] sedadgn commented on pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-971618703
> Hey @sedadgn!
>
> Thank you for your contribution. I've left few comments in the code, though I have a general question too.
>
> I tried to search what kind of headers could fit into the headers property. Is there a common list of them or something like that. Unfortunately I didn't find any. The closest information was that developers often misuse that property, because by design you should store information about the message itself in that field. So a negative example is that if you would like to publish information about a http request, the http header should be in the message, but developers often put that info into the headers property. With that in mind I wondering what could be a real value set of that field and how likely you would have commas in the values. Isn't it an edge case that normally would be better to discourage?
Hi @nandorsoma,
Thank you for your quick response.
For our scenario, the comma is in the object id. So this is information about the message itself for us.
--
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] kevdoran commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r737576297
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,27 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ /**
+ * Escape {@code str} by replacing occurrences of {@code charToEscape} with {@code escapeChar+charToEscape}
+ * @param str the input string
+ * @param escapeChar the character used for escaping
+ * @param charToEscape the character that needs to be escaped
+ * @return the escaped string
+ */
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null || str.isEmpty()) {
+ return null;
+ }
+ StringBuilder result = new StringBuilder();
+ for (int i=0; i<str.length(); i++) {
+ char curChar = str.charAt(i);
+ if (curChar == escapeChar || curChar == charToEscape) {
+ // special char
+ result.append(escapeChar);
+ }
+ result.append(curChar);
Review comment:
Minor, but it would be nice if this utility method were idempotent, meaning that if I pass it a string that contains some already escaped characters, it has not effect on those, in order words, all of these invocations would return the same result:
```
StringUtils.escapeString("key=(value0,value1,value2)", '\\', ',');
StringUtils.escapeString("key=(value0\\,value1,value2)", '\\', ',');
StringUtils.escapeString("key=(value0\\,value1\\,value2)", '\\', ',');
```
--
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] sedadgn commented on pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-976232500
> Hello @nandorsoma
Thank you for your suggestion. I implemented your suggestion. Can you review again.
--
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] sedadgn commented on pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-1012994358
> I'm not convinced that adding guava is necessary just for the split/join use case (I was ok with the previous logic, although perhaps it could be refactored out into a dedicated joinMap/splitMap helper method), but if you do want to stick with guava, it looks to me like most other NARs that bundle it call that out in the NOTICE file in the `nifi-*-bundle/nifi-*-nar/src/main/resources/META-INF/` directory, so that would probably be required in this case as well.
>
> Everything else looks good to me aside from one minor comment about code organization in StandardValidators.
Hello @kevdoran ,
Thank you very much for your review.
I removed guava and added 2 methods for splitMap/joinMap.
--
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] sedadgn commented on pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#issuecomment-986475046
> Thanks for working through the feedback @sedadgn, the current version looks good.
>
> What do you think @kevdoran and @ottobackwards?
Hello @nandorsoma, @ottobackwards and @kevdoran,
Thank you for your feedback and reviews.
We need this feature in our project. If everything works for you, can you merge?
Thank 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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r751285499
##########
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 are right. I added the control for first and last characters
--
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] kevdoran commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r738512134
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,27 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
+ /**
+ * Escape {@code str} by replacing occurrences of {@code charToEscape} with {@code escapeChar+charToEscape}
+ * @param str the input string
+ * @param escapeChar the character used for escaping
+ * @param charToEscape the character that needs to be escaped
+ * @return the escaped string
+ */
+ public static String escapeString(String str, char escapeChar, char charToEscape) {
+ if (str == null || str.isEmpty()) {
+ return null;
+ }
+ StringBuilder result = new StringBuilder();
+ for (int i=0; i<str.length(); i++) {
+ char curChar = str.charAt(i);
+ if (curChar == escapeChar || curChar == charToEscape) {
+ // special char
+ result.append(escapeChar);
+ }
+ result.append(curChar);
Review comment:
I see. It seems that semantics of this method are important to ConsumeAMQP and PublishAMQP. It might be better to put this is a package-private class in `org.apache.nifi.amqp.processors` and only use it there. Then the logic would not have to be generalized, so it could be made potentially more robust for this use case, and for maintainability it protect developers from unintentionally modifying this logic in the future in a way that breaks the expectation of PublishAMQP
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java
##########
@@ -99,6 +101,16 @@
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+ static final PropertyDescriptor UNESCAPE_COMMA = new PropertyDescriptor.Builder()
+ .name("unescape.comma")
Review comment:
The property naming convention here should be consistent with other properties in this processor. So with existing properties named `Exchange Name` and `Routing Key`, I would expect this to be `Unescape Comma`.
--
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] ottobackwards commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r735707010
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/StringUtils.java
##########
@@ -510,4 +514,20 @@ public static String toTitleCase(String input) {
.map(word -> Character.toTitleCase(word.charAt(0)) + word.substring(1))
.collect(Collectors.joining(" "));
}
+
Review comment:
string replace uses "old" , "new" semantics, maybe the parameter order should match that
--
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] sedadgn commented on a change in pull request #5458: NIFI-7865 amqp$header is splitted in the wrong way for "," and "}"
Posted by GitBox <gi...@apache.org>.
sedadgn commented on a change in pull request #5458:
URL: https://github.com/apache/nifi/pull/5458#discussion_r757455310
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/pom.xml
##########
@@ -21,6 +21,8 @@ language governing permissions and limitations under the License. -->
<properties>
<amqp-client.version>5.8.0</amqp-client.version>
+ <common-text.version>1.8</common-text.version>
Review comment:
> Hey @sedadgn! Thank you for implementing the alternative approach. I've found few things, can you check them? None of them is a big thing so I also think we are really close to merging. One slight thing. There are a lot of inconsistently placed empty lines, missing spaces. They are not considered blocker issues in the community but can I ask you to clean them? I'd appreciate if you could do.
@nandorsoma
Thank you for your review. I wrote few comment. I made few changing according your comment.
Best Regards
--
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