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

2021-11-08 Thread GitBox 


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 customValidate(ValidationContext 
validationContext) {
 final List 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

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

2021-10-27 Thread GitBox 


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



##
File path: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/JoltTransformJSON.java
##
@@ -208,7 +209,7 @@
 final ClassLoader customClassLoader;
 
 try {
-if (modulePath != null) {
+if (modulePath != null && 
!validationContext.isExpressionLanguagePresent(modulePath)) {

Review comment:
   Should this check also be added to JoltTransformRecord.customValidate()?




-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

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

2021-10-27 Thread GitBox 


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



##
File path: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/JoltTransformJSON.java
##
@@ -242,7 +251,8 @@
 }
 }
 } catch (final Exception e) {
-getLogger().info("Processor is not valid - " + e.toString());
+//getLogger().info("Processor is not valid - " + e.toString());

Review comment:
   Nitpick to remove dead code

##
File path: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/JoltTransformJSON.java
##
@@ -208,7 +209,7 @@
 final ClassLoader customClassLoader;
 
 try {
-if (modulePath != null) {
+if (modulePath != null && 
!validationContext.isExpressionLanguagePresent(modulePath)) {

Review comment:
   Should this check also be added to JoltTransformRecord.validate()?

##
File path: 
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/JoltTransformJSON.java
##
@@ -306,26 +316,33 @@ public void process(OutputStream out) throws IOException {
 logger.info("Transformed {}", new Object[]{original});
 }
 
-private JoltTransform getTransform(final ProcessContext context, final 
FlowFile flowFile) throws Exception {
+private JoltTransform getTransform(final ProcessContext context, final 
FlowFile flowFile) {
 final Optional specString;
 if (context.getProperty(JOLT_SPEC).isSet()) {
 specString = 
Optional.of(context.getProperty(JOLT_SPEC).evaluateAttributeExpressions(flowFile).getValue());
 } else {
 specString = Optional.empty();
 }
 
-return transformCache.get(specString);
+return transformCache.get(specString, currString -> {
+try {
+return createTransform(context, currString.orElse(null), 
flowFile);
+} catch (Exception e) {
+getLogger().error("Problem getting transform", e);
+}
+return null;
+});
 }
 
 @OnScheduled
 public void setup(final ProcessContext context) {
 int maxTransformsToCache = 
context.getProperty(TRANSFORM_CACHE_SIZE).asInteger();
 transformCache = Caffeine.newBuilder()
 .maximumSize(maxTransformsToCache)
-.build(specString -> createTransform(context, 
specString.orElse(null)));
+.build();
 
 try {
-if (context.getProperty(MODULES).isSet()) {
+if (context.getProperty(MODULES).isSet() && 
!context.getProperty(MODULES).isExpressionLanguagePresent()) {

Review comment:
   Is this necessary? I would think the property would be invalid if there 
was EL present, and if we decide to support EL we would remove this and add 
`evaluateAttributeExpression()`




-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org