Re: [PR] NIFI-12153 allow comments and confiugrable max string size in JSON Record Readers [nifi]

["exceptionfactory (via GitHub)" <gi...@apache.org>]

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


##########
nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-json-record-utils/src/main/java/org/apache/nifi/json/AbstractJsonRowRecordReader.java:
##########
@@ -51,16 +53,31 @@
 import java.util.function.Supplier;
 
 public abstract class AbstractJsonRowRecordReader implements RecordReader {
+    static final PropertyDescriptor MAX_STRING_LENGTH = new PropertyDescriptor.Builder()
+            .name("max-string-length")

Review Comment:
   Recommend starting out with the same value for name.
   ```suggestion
               .name("Max Strength Length")
   ```



##########
nifi-nar-bundles/nifi-standard-services/nifi-record-serialization-services-bundle/nifi-record-serialization-services/src/test/java/org/apache/nifi/json/TestJsonTreeRowRecordReader.java:
##########
@@ -296,11 +298,34 @@ void testReadOneLinePerJSON() throws IOException, MalformedRecordException {
 
     @Test
     void testReadMultilineJSON() throws IOException, MalformedRecordException {
+        testReadAccountJson("src/test/resources/json/bank-account-multiline.json", false, null);
+    }
+
+    @Test
+    void testReadJSONStringTooLong() {
+        final StreamConstraintsException mre = assertThrows(StreamConstraintsException.class, () ->
+                testReadAccountJson("src/test/resources/json/bank-account-multiline.json", false, StreamReadConstraints.builder().maxStringLength(2).build()));
+        assertEquals("String length (8) exceeds the maximum length (2)", mre.getMessage());

Review Comment:
   Having an assertion for the entire message can make the test brittle. Recommend adjusting it to look for a keyword, in this case `2` to align with the maximum string length.



##########
nifi-nar-bundles/nifi-standard-services/nifi-record-serialization-services-bundle/nifi-record-serialization-services/src/test/java/org/apache/nifi/json/TestJsonTreeRowRecordReader.java:
##########
@@ -296,11 +298,34 @@ void testReadOneLinePerJSON() throws IOException, MalformedRecordException {
 
     @Test
     void testReadMultilineJSON() throws IOException, MalformedRecordException {
+        testReadAccountJson("src/test/resources/json/bank-account-multiline.json", false, null);
+    }
+
+    @Test
+    void testReadJSONStringTooLong() {
+        final StreamConstraintsException mre = assertThrows(StreamConstraintsException.class, () ->
+                testReadAccountJson("src/test/resources/json/bank-account-multiline.json", false, StreamReadConstraints.builder().maxStringLength(2).build()));
+        assertEquals("String length (8) exceeds the maximum length (2)", mre.getMessage());
+    }
+
+    @Test
+    void testReadJSONComments() throws IOException, MalformedRecordException {
+        testReadAccountJson("src/test/resources/json/bank-account-comments.jsonc", true, StreamReadConstraints.builder().maxStringLength(20_000).build());
+    }
+
+    @Test
+    void testReadJSONDisallowComments() {
+        final MalformedRecordException mre = assertThrows(MalformedRecordException.class, () ->
+            testReadAccountJson("src/test/resources/json/bank-account-comments.jsonc", false, StreamReadConstraints.builder().maxStringLength(20_000).build()));
+        assertEquals("Could not parse data as JSON", mre.getMessage());

Review Comment:
   This assertion can be removed, or perhaps adjusted to look for the word `parse` by itself.



##########
nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-json-record-utils/src/main/java/org/apache/nifi/json/AbstractJsonRowRecordReader.java:
##########
@@ -51,16 +53,31 @@
 import java.util.function.Supplier;
 
 public abstract class AbstractJsonRowRecordReader implements RecordReader {
+    static final PropertyDescriptor MAX_STRING_LENGTH = new PropertyDescriptor.Builder()
+            .name("max-string-length")
+            .displayName("Max String Length")
+            .description("The maximum allowed length of a string value when parsing the JSON document")
+            .required(true)
+            .defaultValue("20 MB")
+            .addValidator(StandardValidators.DATA_SIZE_VALIDATOR)
+            .build();
+
+    public static final PropertyDescriptor ALLOW_COMMENTS = new PropertyDescriptor.Builder()
+            .name("json-allow-comments")

Review Comment:
   ```suggestion
               .name("Allow Comments")
   ```



##########
nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-json-record-utils/src/main/java/org/apache/nifi/json/AbstractJsonRowRecordReader.java:
##########
@@ -122,7 +156,15 @@ protected AbstractJsonRowRecordReader(final InputStream in,
         capturedFields = new LinkedHashMap<>();
 
         try {
-            jsonParser = jsonFactory.createParser(in);
+            final ObjectMapper codec = new ObjectMapper();
+            if (allowComments) {
+                codec.enable(JsonParser.Feature.ALLOW_COMMENTS);
+            }
+            if (streamReadConstraints != null) {
+                codec.getFactory().setStreamReadConstraints(streamReadConstraints);
+            }

Review Comment:
   Instead of allowing this to be null, it would be more consistent to to always create a default instance of the constraints, and set 20 MB as the value, aligning with current Jackson 2.15 defaults.



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