[GitHub] [nifi] mattyb149 commented on a change in pull request #5444: NIFI-9286: Add expression language to JOLT processors and fixes the custom module implementation to use custom jars in the processor

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

mattyb149 commented on a change in pull request #5444:
URL: https://github.com/apache/nifi/pull/5444#discussion_r745094543



##########
File path: nifi-nar-bundles/nifi-jolt-record-bundle/nifi-jolt-record-processors/src/main/java/org/apache/nifi/processors/jolt/record/JoltTransformRecord.java
##########
@@ -151,7 +152,7 @@
             .displayName("Custom Transformation Class Name")
             .description("Fully Qualified Class Name for Custom Transformation")
             .required(false)
-            .expressionLanguageSupported(ExpressionLanguageScope.NONE)
+            .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)

Review comment:
       This property is ignored unless the transformation type is CUSTOMR right? If so, it should have a `.dependsOn()` call in the builder, same goes for Custom Module Directory below.

##########
File path: nifi-nar-bundles/nifi-jolt-record-bundle/nifi-jolt-record-processors/src/test/java/org/apache/nifi/processors/jolt/record/TestJoltTransformRecord.java
##########
@@ -582,6 +582,35 @@ public void testTransformInputCustomTransformationIgnored() throws IOException {
                 new String(transformed.toByteArray()));
     }
 
+    @Test
+    public void testExpressionLanguageJarFile() throws IOException {
+        generateTestData(1, null);
+        final String outputSchemaText = new String(Files.readAllBytes(Paths.get("src/test/resources/TestJoltTransformRecord/defaultrOutputSchema.avsc")));
+        runner.setProperty(writer, SchemaAccessUtils.SCHEMA_ACCESS_STRATEGY, SchemaAccessUtils.SCHEMA_TEXT_PROPERTY);
+        runner.setProperty(writer, SchemaAccessUtils.SCHEMA_TEXT, outputSchemaText);
+        runner.setProperty(writer, "Pretty Print JSON", "true");
+        runner.enableControllerService(writer);
+        final String customJarPath = "src/test/resources/TestJoltTransformRecord/TestCustomJoltTransform.jar";
+        final String spec = new String(Files.readAllBytes(Paths.get("src/test/resources/TestJoltTransformRecord/defaultrSpec.json")));
+        final String customJoltTransform = "TestCustomJoltTransform";
+        runner.setVariable("CUSTOM_JAR", customJarPath);
+        runner.setProperty(JoltTransformRecord.JOLT_SPEC, "${JOLT_SPEC}");
+        runner.setProperty(JoltTransformRecord.MODULES, "${CUSTOM_JAR}");
+        runner.setProperty(JoltTransformRecord.JOLT_TRANSFORM, JoltTransformRecord.DEFAULTR);

Review comment:
       This should be `CUSTOMR` not `DEFAULTR` right? That should also illustrate that the processor should be invalid before running, as customValidate() should (currently) fail since the `JOLT_SPEC` attribute is not available at the time of validation.

##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/JoltTransformJSON.java
##########
@@ -119,7 +121,8 @@
             .description("Comma-separated list of paths to files and/or directories which contain modules containing custom transformations (that are not included on NiFi's classpath).")
             .required(false)
             .identifiesExternalResource(ResourceCardinality.MULTIPLE, ResourceType.FILE, ResourceType.DIRECTORY)
-            .expressionLanguageSupported(ExpressionLanguageScope.NONE)
+            .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)

Review comment:
       See my comment above, I would think these would be set to the same value for both JOLT processors

##########
File path: nifi-nar-bundles/nifi-jolt-record-bundle/nifi-jolt-record-processors/src/main/java/org/apache/nifi/processors/jolt/record/JoltTransformRecord.java
##########
@@ -160,7 +161,7 @@
             .displayName("Custom Module Directory")
             .description("Comma-separated list of paths to files and/or directories which contain modules containing custom transformations (that are not included on NiFi's classpath).")
             .required(false)
-            .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+            .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)

Review comment:
       Your comment on the main page made me think this was going to be returned to VARIABLE_REGISTRY?

##########
File path: nifi-nar-bundles/nifi-jolt-record-bundle/nifi-jolt-record-processors/src/main/java/org/apache/nifi/processors/jolt/record/JoltTransformRecord.java
##########
@@ -234,7 +235,7 @@
     protected Collection<ValidationResult> customValidate(ValidationContext validationContext) {
         final List<ValidationResult> results = new ArrayList<>(super.customValidate(validationContext));
         final String transform = validationContext.getProperty(JOLT_TRANSFORM).getValue();
-        final String customTransform = validationContext.getProperty(CUSTOM_CLASS).getValue();
+        final String customTransform = validationContext.getProperty(CUSTOM_CLASS).evaluateAttributeExpressions().getValue();

Review comment:
       If you change this property to accept FlowFile attributes, you won't be able to use it the way it has been used in customValidate(), instead you'd need to check if Expression Language is present and assume it is valid in that case, then check again at runtime. If you revert it back to Variable Registry, this change can remain.




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