NIFI-14953 Validate inlined protobuf schemas #10291
Conversation
There was a problem hiding this comment.
Thanks for this additional validation @lkuchars.
As this requires writing out temporary files, it seems like it could be a bit expensive as written, given that customValidate can be triggered many times. One option is to implement cached checking of the schema so that it only runs the compilation when the Schema Text property changes. Another option is to implement this as a manual verify method, requiring user interaction. It seems better to consider the verify method approach, to avoid complexity around caching, and because this validation only applies to the Schema Text option, but I'm open to considering either approach.
| return compiledSchema; | ||
|
|
||
| } catch (final IllegalStateException e) { | ||
| throw new SchemaCompilationException(e); // Illegal state exception is thrown by the wire library for schema issues |
There was a problem hiding this comment.
A message should be included along with the IllegalStateException cause
|
|
||
| final ValidationResult invalidResult = verifyExactlyOneValidationError(); | ||
|
|
||
| assertTrue(invalidResult.getExplanation().contains("Message name 'test.NonExistentMessage' cannot be found")); |
There was a problem hiding this comment.
Recommend reducing the search string:
| assertTrue(invalidResult.getExplanation().contains("Message name 'test.NonExistentMessage' cannot be found")); | |
| assertTrue(invalidResult.getExplanation().contains("test.NonExistentMessage")); |
| return problems; | ||
| } | ||
|
|
||
| // Compile the schema to validate it's correct. The method is used only for validation purposes. |
There was a problem hiding this comment.
This comment mostly reiterates the method name, and the usage in validation is clear, so recommend removing this comment.
|
|
||
| @Override | ||
| protected Collection<ValidationResult> customValidate(final ValidationContext validationContext) { | ||
| final List<ValidationResult> problems = new ArrayList<>(super.customValidate(validationContext)); |
There was a problem hiding this comment.
Results can also indicate a valid status, so the variable name should be changed.
| final List<ValidationResult> problems = new ArrayList<>(super.customValidate(validationContext)); | |
| final List<ValidationResult> results = new ArrayList<>(super.customValidate(validationContext)); |
| final PropertyValue schemaTextProperty = validationContext.getProperty(SCHEMA_TEXT); | ||
| final String schemaTextValue = schemaTextProperty.getValue(); | ||
|
|
||
| if (validationContext.isExpressionLanguageSupported(SCHEMA_TEXT.getName()) |
There was a problem hiding this comment.
Is the isExpressionLanguageSupported check needed?
|
|
||
| if (validationContext.isExpressionLanguageSupported(SCHEMA_TEXT.getName()) | ||
| && validationContext.isExpressionLanguagePresent(schemaTextValue)) { | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
Creating the results array and then having multiple return statements is not ideal. It would be better to refactor this method to have a single return at the end.
There was a problem hiding this comment.
If we refactor to have a single return, we would need to use nested if-else statements or boolean flags, which would actually make the code less readable and more complex. The early returns here serve a legitimate purpose - they represent different validation scenarios where further validation is either unnecessary or impossible.
| .subject(SCHEMA_TEXT.getDisplayName()) | ||
| .input(schemaTextValue) | ||
| .valid(false) | ||
| .explanation("Schema Text cannot be empty when using \"Use 'Schema Text' Property\" strategy") |
There was a problem hiding this comment.
Recommend avoiding the reference to the other property name:
| .explanation("Schema Text cannot be empty when using \"Use 'Schema Text' Property\" strategy") | |
| .explanation("Schema Text value is missing) |
| } | ||
|
|
||
| try { | ||
| // Try to compile the schema to validate it's valid protobuf format |
There was a problem hiding this comment.
| // Try to compile the schema to validate it's valid protobuf format | |
| // Try to compile the schema to validate protobuf format |
@exceptionfactory thank you for taking your time to look at this. It would be nice to get the validation of the schema without manual intervention. I think the original ProtobufReader worked that way, and it used cache. I think your suggestion with a cache is spot on. |
Thanks for the reply @lkuchars. Using the same |
lkuchars
force-pushed
the
NIFI-14953-add-proto-schema-validation
branch
from
78c0d7a to
1c995f4
Compare
I moved the initialization of the schemaCompiler to the init() override. |
Summary
NIFI-14953
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation