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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
