pvillard31 commented on code in PR #11338:
URL: https://github.com/apache/nifi/pull/11338#discussion_r3417633626
##########
nifi-extension-bundles/nifi-jslt-bundle/nifi-jslt-processors/src/main/java/org/apache/nifi/processors/jslt/JSLTTransformJSON.java:
##########
@@ -430,9 +429,9 @@ private String readTransform(final PropertyValue
propertyValue) {
return propertyValue.getValue();
}
try (final BufferedReader reader = new BufferedReader(new
InputStreamReader(resourceReference.read()))) {
- return reader.lines().collect(Collectors.joining());
+ return
reader.lines().collect(Collectors.joining(System.lineSeparator()));
Review Comment:
Did you consider joining the lines with a fixed newline instead of
System.lineSeparator(), so the compiled transform and the modifyContent
provenance detail stay identical across Windows and Linux?
##########
nifi-extension-bundles/nifi-jslt-bundle/nifi-jslt-processors/src/test/java/org/apache/nifi/processors/jslt/TestJSLTTransformJSON.java:
##########
@@ -74,6 +82,87 @@ public void testInvalidJSLTTransform() {
runner.assertNotValid();
}
+ @Test
+ public void testMultipleLetStatementsOnDifferentLinesDoNotCollide() {
+ final String transformWithMultilines = """
+ let id = .value.id
+ let client_name = replace(.value.client_name, "-[^-]+$", ""){
+ "id": $id,
+ "name": $client_name
+ }
+ """;
+
+ runner.setProperty(JSLTTransformJSON.JSLT_TRANSFORM,
transformWithMultilines);
+ runner.setProperty(JSLTTransformJSON.TRANSFORMATION_STRATEGY,
ENTIRE_FLOWFILE.getValue());
+ final String json = """
+ {
+ "value" : {
+ "id" : "jdoe",
+ "client_name": "John"
+ }
+ }
+ """;
+ runner.enqueue(json);
+
+ assertDoesNotThrow(() -> runner.run());
+ runner.assertTransferCount(JSLTTransformJSON.REL_SUCCESS, 1);
+ }
+
+ @Test
+ public void testImprovedInvalidTransformReadMessage() {
+ final Path nonExistentTransformFile =
tempDir.resolve("transform.json");
+ runner.setProperty(JSLTTransformJSON.JSLT_TRANSFORM,
nonExistentTransformFile.toString());
+ runner.setProperty(JSLTTransformJSON.TRANSFORMATION_STRATEGY,
ENTIRE_FLOWFILE.getValue());
+
+ final String json = """
+ {
+ "userId" : "jdoe",
+ "firstName": "John"
+ }
+ """;
+ runner.enqueue(json);
+
+ assertThrows(AssertionFailedError.class, () -> runner.run());
+ }
+
+ @ParameterizedTest
+ @MethodSource("singleLineCommentArgs")
+ public void testJSLTTransformWithSingleLineComment(String transform) {
+ runner.setProperty(JSLTTransformJSON.JSLT_TRANSFORM, transform);
+ runner.setProperty(JSLTTransformJSON.TRANSFORMATION_STRATEGY,
ENTIRE_FLOWFILE.getValue());
+
+ final String json = """
+ {
+ "userId" : "jdoe",
+ "firstName": "John"
+ }
+ """;
+ runner.enqueue(json);
+
+ // TODO Uncomment after NIFI-15982 is accepted and merged and NIFI
uses the next version of nifi-api
Review Comment:
Since NIFI-15982 is already merged and this build uses a nifi-api that
includes it, should this TODO comment be removed, given that nothing below it
is actually commented out?
##########
nifi-extension-bundles/nifi-jslt-bundle/nifi-jslt-processors/src/test/java/org/apache/nifi/processors/jslt/TestJSLTTransformJSON.java:
##########
@@ -74,6 +82,87 @@ public void testInvalidJSLTTransform() {
runner.assertNotValid();
}
+ @Test
+ public void testMultipleLetStatementsOnDifferentLinesDoNotCollide() {
+ final String transformWithMultilines = """
+ let id = .value.id
+ let client_name = replace(.value.client_name, "-[^-]+$", ""){
+ "id": $id,
+ "name": $client_name
+ }
+ """;
+
+ runner.setProperty(JSLTTransformJSON.JSLT_TRANSFORM,
transformWithMultilines);
+ runner.setProperty(JSLTTransformJSON.TRANSFORMATION_STRATEGY,
ENTIRE_FLOWFILE.getValue());
+ final String json = """
+ {
+ "value" : {
+ "id" : "jdoe",
+ "client_name": "John"
+ }
+ }
+ """;
+ runner.enqueue(json);
+
+ assertDoesNotThrow(() -> runner.run());
+ runner.assertTransferCount(JSLTTransformJSON.REL_SUCCESS, 1);
+ }
+
+ @Test
+ public void testImprovedInvalidTransformReadMessage() {
+ final Path nonExistentTransformFile =
tempDir.resolve("transform.json");
+ runner.setProperty(JSLTTransformJSON.JSLT_TRANSFORM,
nonExistentTransformFile.toString());
+ runner.setProperty(JSLTTransformJSON.TRANSFORMATION_STRATEGY,
ENTIRE_FLOWFILE.getValue());
+
+ final String json = """
+ {
+ "userId" : "jdoe",
+ "firstName": "John"
+ }
+ """;
+ runner.enqueue(json);
+
+ assertThrows(AssertionFailedError.class, () -> runner.run());
Review Comment:
This test only checks that an error is thrown. Can it also assert that the
validation explanation or exception message contains the new reason text, so it
would fail if the message change were reverted?
##########
nifi-extension-bundles/nifi-jslt-bundle/nifi-jslt-processors/src/test/java/org/apache/nifi/processors/jslt/TestJSLTTransformJSON.java:
##########
@@ -74,6 +82,87 @@ public void testInvalidJSLTTransform() {
runner.assertNotValid();
}
+ @Test
+ public void testMultipleLetStatementsOnDifferentLinesDoNotCollide() {
+ final String transformWithMultilines = """
+ let id = .value.id
+ let client_name = replace(.value.client_name, "-[^-]+$", ""){
+ "id": $id,
+ "name": $client_name
+ }
+ """;
+
+ runner.setProperty(JSLTTransformJSON.JSLT_TRANSFORM,
transformWithMultilines);
+ runner.setProperty(JSLTTransformJSON.TRANSFORMATION_STRATEGY,
ENTIRE_FLOWFILE.getValue());
+ final String json = """
+ {
+ "value" : {
+ "id" : "jdoe",
+ "client_name": "John"
+ }
+ }
+ """;
+ runner.enqueue(json);
+
+ assertDoesNotThrow(() -> runner.run());
Review Comment:
Is there a reason to wrap runner.run() in assertDoesNotThrow here and in the
parameterized test, or can we call runner.run() directly since the test already
fails on any thrown exception?
--
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]
