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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
