exceptionfactory commented on code in PR #11012:
URL: https://github.com/apache/nifi/pull/11012#discussion_r3011411900
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestValidateJson.java:
##########
@@ -265,8 +295,15 @@ private void assertValidationErrors(Relationship
relationship, boolean expected)
private static Stream<Arguments> customValidateArgs() {
return Stream.of(
-
Arguments.of(ValidateJson.JsonSchemaStrategy.SCHEMA_NAME_PROPERTY, "requires
that the JSON Schema Registry property be set"),
-
Arguments.of(ValidateJson.JsonSchemaStrategy.SCHEMA_CONTENT_PROPERTY, "requires
that the JSON Schema property be set")
+ Arguments.argumentSet("Require JSON Schema Registry property
to be set", ValidateJson.JsonSchemaStrategy.SCHEMA_NAME_PROPERTY),
+ Arguments.argumentSet("Require JSON Schema property to be
set", ValidateJson.JsonSchemaStrategy.SCHEMA_CONTENT_PROPERTY)
+ );
+ }
+
+ private static Stream<Arguments> multilineJsonArgs() {
+ return Stream.of(
+
Arguments.argumentSet(ValidateJson.InputFormat.FLOW_FILE.getDisplayName(),
ValidateJson.InputFormat.FLOW_FILE.getValue(), true),
Review Comment:
Instead of passing the displayName and value separately, it looks like it
would be cleaner to pass the `InputFormat` itself.
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ValidateJson.java:
##########
@@ -298,27 +339,36 @@ public void onTrigger(final ProcessContext context, final
ProcessSession session
}
}
+ if (schema == null) {
+ getLogger().error("JSON schema not configured for {}", flowFile);
+ session.getProvenanceReporter().route(flowFile, REL_FAILURE);
+ session.transfer(flowFile, REL_FAILURE);
+ return;
+ }
+
+ final InputFormat inputFormat =
context.getProperty(INPUT_FORMAT).asAllowableValue(InputFormat.class);
+ if (inputFormat == InputFormat.FLOW_FILE) {
+ validateFlowFile(session, flowFile);
+ } else {
+ validateJsonLines(session, flowFile);
+ }
+ }
+
+ void validateFlowFile(final ProcessSession session, final FlowFile
flowFile) {
Review Comment:
It looks like both `validate` methods should be `private` instead of
package-private.
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ValidateJson.java:
##########
@@ -298,27 +339,36 @@ public void onTrigger(final ProcessContext context, final
ProcessSession session
}
}
+ if (schema == null) {
+ getLogger().error("JSON schema not configured for {}", flowFile);
+ session.getProvenanceReporter().route(flowFile, REL_FAILURE);
+ session.transfer(flowFile, REL_FAILURE);
+ return;
+ }
Review Comment:
On closer review, the current logic for setting the `schema` seems a bit
awkward, given the use of the `volatile` member variable and different ways of
reading.
I think this needs some rework, given the multiple references in multiple
methods. One simple options for now is to declare `final Schema currentSchema =
schema` in each `validate` method so that the value remains the same during
execution, especially for multiple JSON Lines.
A more complex approach would be to refactor how the schema is loaded and
cached. In the interest of keeping this PR scoped, the simple options seems
better for now.
--
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]
