You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "exceptionfactory (via GitHub)" <gi...@apache.org> on 2023/05/23 15:26:17 UTC

[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7231: [NIFI-2964] Added ability for AttributeToJSON to handle nested JSON when either outputting to a flow file or an attribute.

exceptionfactory commented on code in PR #7231:
URL: https://github.com/apache/nifi/pull/7231#discussion_r1202529486


##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AttributesToJSON.java:
##########
@@ -240,24 +281,51 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
             return;
         }
 
-        final Map<String, String> atrList = buildAttributesMapForFlowFile(original, attributes, attributesToRemove, nullValueForEmptyString, pattern);
+        final Map<String, Object> atrList = buildAttributesMapForFlowFile(original, attributes, attributesToRemove, nullValueForEmptyString, pattern);
 
         try {
+            Map<String, Object> asJson = getAsJson(atrList);
             if (destinationContent) {
                 FlowFile conFlowfile = session.write(original, (in, out) -> {
                     try (OutputStream outputStream = new BufferedOutputStream(out)) {
-                        outputStream.write(objectMapper.writeValueAsBytes(atrList));
+                        outputStream.write(objectMapper.writeValueAsBytes(asJson));
                     }
                 });
                 conFlowfile = session.putAttribute(conFlowfile, CoreAttributes.MIME_TYPE.key(), APPLICATION_JSON);
                 session.transfer(conFlowfile, REL_SUCCESS);
             } else {
-                FlowFile atFlowfile = session.putAttribute(original, JSON_ATTRIBUTE_NAME, objectMapper.writeValueAsString(atrList));
+                FlowFile atFlowfile = session.putAttribute(original, JSON_ATTRIBUTE_NAME, objectMapper.writeValueAsString(asJson));
                 session.transfer(atFlowfile, REL_SUCCESS);
             }
         } catch (JsonProcessingException e) {
             getLogger().error(e.getMessage());
             session.transfer(original, REL_FAILURE);
         }
     }
+
+    private Map<String, Object> getAsJson(Map<String, Object> atrList) throws JsonProcessingException {
+        if(JsonHandlingStrategy.ESCAPED_STRING.equals(jsonHandlingStrategy)) {
+            return atrList;
+        }
+
+        Map<String, Object> asJson = new HashMap<>();

Review Comment:
   Recommend using `LinkedHashMap` to preserve key ordering.
   ```suggestion
           Map<String, Object> asJson = new LinkedHashMap<>();
   ```



##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AttributesToJSON.java:
##########
@@ -240,24 +281,51 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
             return;
         }
 
-        final Map<String, String> atrList = buildAttributesMapForFlowFile(original, attributes, attributesToRemove, nullValueForEmptyString, pattern);
+        final Map<String, Object> atrList = buildAttributesMapForFlowFile(original, attributes, attributesToRemove, nullValueForEmptyString, pattern);
 
         try {
+            Map<String, Object> asJson = getAsJson(atrList);
             if (destinationContent) {
                 FlowFile conFlowfile = session.write(original, (in, out) -> {
                     try (OutputStream outputStream = new BufferedOutputStream(out)) {
-                        outputStream.write(objectMapper.writeValueAsBytes(atrList));
+                        outputStream.write(objectMapper.writeValueAsBytes(asJson));
                     }
                 });
                 conFlowfile = session.putAttribute(conFlowfile, CoreAttributes.MIME_TYPE.key(), APPLICATION_JSON);
                 session.transfer(conFlowfile, REL_SUCCESS);
             } else {
-                FlowFile atFlowfile = session.putAttribute(original, JSON_ATTRIBUTE_NAME, objectMapper.writeValueAsString(atrList));
+                FlowFile atFlowfile = session.putAttribute(original, JSON_ATTRIBUTE_NAME, objectMapper.writeValueAsString(asJson));
                 session.transfer(atFlowfile, REL_SUCCESS);
             }
         } catch (JsonProcessingException e) {
             getLogger().error(e.getMessage());
             session.transfer(original, REL_FAILURE);
         }
     }
+
+    private Map<String, Object> getAsJson(Map<String, Object> atrList) throws JsonProcessingException {
+        if(JsonHandlingStrategy.ESCAPED_STRING.equals(jsonHandlingStrategy)) {

Review Comment:
   This can be simplified:
   ```suggestion
           if (JsonHandlingStrategy.ESCAPED_STRING == jsonHandlingStrategy) {
   ```



##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AttributesToJSON.java:
##########
@@ -122,6 +151,15 @@ public class AttributesToJSON extends AbstractProcessor {
             .defaultValue("false")
             .build();
 
+    public static final PropertyDescriptor JSON_HANDLING_STRATEGY = new PropertyDescriptor.Builder()
+            .name("Json Handling Strategy")
+            .displayName("Json Handling Strategy")
+            .description("Strategy to use for handling attributes which contain nested JSON.")
+            .required(true)
+            .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)

Review Comment:
   Recommend leaving this as `NONE` since it can also be parameterized using Parameter Contexts, instead of the Variable Registry or environment variables, which are not preferred.



##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AttributesToJSON.java:
##########
@@ -240,24 +281,51 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
             return;
         }
 
-        final Map<String, String> atrList = buildAttributesMapForFlowFile(original, attributes, attributesToRemove, nullValueForEmptyString, pattern);
+        final Map<String, Object> atrList = buildAttributesMapForFlowFile(original, attributes, attributesToRemove, nullValueForEmptyString, pattern);
 
         try {
+            Map<String, Object> asJson = getAsJson(atrList);
             if (destinationContent) {
                 FlowFile conFlowfile = session.write(original, (in, out) -> {
                     try (OutputStream outputStream = new BufferedOutputStream(out)) {
-                        outputStream.write(objectMapper.writeValueAsBytes(atrList));
+                        outputStream.write(objectMapper.writeValueAsBytes(asJson));
                     }
                 });
                 conFlowfile = session.putAttribute(conFlowfile, CoreAttributes.MIME_TYPE.key(), APPLICATION_JSON);
                 session.transfer(conFlowfile, REL_SUCCESS);
             } else {
-                FlowFile atFlowfile = session.putAttribute(original, JSON_ATTRIBUTE_NAME, objectMapper.writeValueAsString(atrList));
+                FlowFile atFlowfile = session.putAttribute(original, JSON_ATTRIBUTE_NAME, objectMapper.writeValueAsString(asJson));
                 session.transfer(atFlowfile, REL_SUCCESS);
             }
         } catch (JsonProcessingException e) {
             getLogger().error(e.getMessage());
             session.transfer(original, REL_FAILURE);
         }
     }
+
+    private Map<String, Object> getAsJson(Map<String, Object> atrList) throws JsonProcessingException {

Review Comment:
   Recommend adjusting the argument name. The method name should also be adjusted to more accurately reflect the fact that the input Map may not be changed.
   ```suggestion
       private Map<String, Object> getFormattedAttributes(final Map<String, Object> flowFileAttributes) throws JsonProcessingException {
   ```



##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AttributesToJSON.java:
##########
@@ -59,9 +60,37 @@
 @Tags({"json", "attributes", "flowfile"})
 @InputRequirement(InputRequirement.Requirement.INPUT_REQUIRED)
 @CapabilityDescription("Generates a JSON representation of the input FlowFile Attributes. The resulting JSON " +
-        "can be written to either a new Attribute 'JSONAttributes' or written to the FlowFile as content.")
+        "can be written to either a new Attribute 'JSONAttributes' or written to the FlowFile as content. Attributes " +
+        " which contain nested JSON objects can either be handled as JSON or as escaped JSON depending on the strategy chosen.")
 @WritesAttribute(attribute = "JSONAttributes", description = "JSON representation of Attributes")
 public class AttributesToJSON extends AbstractProcessor {
+    public enum JsonHandlingStrategy implements DescribedValue {
+        ESCAPED_STRING("Escapes any nested JSON as a string", "Escaped String"),
+        NESTED_OBJECT("Handles nested JSON as JSON", "Nested Object");
+
+        private final String description;
+        private final String displayName;
+
+        JsonHandlingStrategy(String description, String displayName) {

Review Comment:
   As a general convention, recommend placing the display name before the description.
   ```suggestion
           ESCAPED_STRING("Escaped String", "Escapes JSON attribute values to strings"),
           NESTED_OBJECT("Nested Object", "Handles JSON attribute values as nested structured objects");
   
           private final String description;
           private final String displayName;
   
           JsonHandlingStrategy(String displayName, String description) {
   ```



##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AttributesToJSON.java:
##########
@@ -122,6 +151,15 @@ public class AttributesToJSON extends AbstractProcessor {
             .defaultValue("false")
             .build();
 
+    public static final PropertyDescriptor JSON_HANDLING_STRATEGY = new PropertyDescriptor.Builder()
+            .name("Json Handling Strategy")
+            .displayName("Json Handling Strategy")

Review Comment:
   `JSON` should be all-caps:
   ```suggestion
               .name("JSON Handling Strategy")
               .displayName("JSON Handling Strategy")
   ```



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