exceptionfactory commented on code in PR #10291:
URL: https://github.com/apache/nifi/pull/10291#discussion_r2352569234
##########
nifi-extension-bundles/nifi-protobuf-bundle/nifi-protobuf-services/src/main/java/org/apache/nifi/services/protobuf/ProtobufSchemaCompiler.java:
##########
@@ -129,7 +129,8 @@ private Schema compileSchemaDefinition(final
SchemaDefinition schemaDefinition)
final Schema compiledSchema = createAndLoadSchema(tempDir);
logger.debug("Successfully compiled schema for identifier:
{}", schemaDefinition.getIdentifier());
return compiledSchema;
-
+ } catch (final IllegalStateException e) {
+ throw new SchemaCompilationException(e); // Illegal state
exception is thrown by the wire library for schema issues
Review Comment:
A message should be included along with the `IllegalStateException` cause
##########
nifi-extension-bundles/nifi-protobuf-bundle/nifi-protobuf-services/src/main/java/org/apache/nifi/services/protobuf/StandardProtobufReader.java:
##########
@@ -170,6 +174,84 @@ protected PropertyDescriptor buildSchemaTextProperty() {
return PROTOBUF_SCHEMA_TEXT;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(final
ValidationContext validationContext) {
+ final List<ValidationResult> problems = new
ArrayList<>(super.customValidate(validationContext));
+ final String schemaAccessStrategyValue =
validationContext.getProperty(SCHEMA_ACCESS_STRATEGY).getValue();
+
+ // Only validate when using Schema Text property strategy
+ if (SCHEMA_TEXT_PROPERTY.getValue().equals(schemaAccessStrategyValue))
{
+ final PropertyValue schemaTextProperty =
validationContext.getProperty(SCHEMA_TEXT);
+ final String schemaTextValue = schemaTextProperty.getValue();
+
+ if
(validationContext.isExpressionLanguageSupported(SCHEMA_TEXT.getName())
+ &&
validationContext.isExpressionLanguagePresent(schemaTextValue)) {
+ return Collections.emptyList();
Review 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.
##########
nifi-extension-bundles/nifi-protobuf-bundle/nifi-protobuf-services/src/main/java/org/apache/nifi/services/protobuf/StandardProtobufReader.java:
##########
@@ -170,6 +174,84 @@ protected PropertyDescriptor buildSchemaTextProperty() {
return PROTOBUF_SCHEMA_TEXT;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(final
ValidationContext validationContext) {
+ final List<ValidationResult> problems = new
ArrayList<>(super.customValidate(validationContext));
+ final String schemaAccessStrategyValue =
validationContext.getProperty(SCHEMA_ACCESS_STRATEGY).getValue();
+
+ // Only validate when using Schema Text property strategy
+ if (SCHEMA_TEXT_PROPERTY.getValue().equals(schemaAccessStrategyValue))
{
+ final PropertyValue schemaTextProperty =
validationContext.getProperty(SCHEMA_TEXT);
+ final String schemaTextValue = schemaTextProperty.getValue();
+
+ if
(validationContext.isExpressionLanguageSupported(SCHEMA_TEXT.getName())
+ &&
validationContext.isExpressionLanguagePresent(schemaTextValue)) {
+ return Collections.emptyList();
+ }
+
+ if (schemaTextValue == null || schemaTextValue.isBlank()) {
+ problems.add(new ValidationResult.Builder()
+ .subject(SCHEMA_TEXT.getDisplayName())
+ .input(schemaTextValue)
+ .valid(false)
+ .explanation("Schema Text cannot be empty when using \"Use
'Schema Text' Property\" strategy")
Review Comment:
Recommend avoiding the reference to the other property name:
```suggestion
.explanation("Schema Text value is missing)
```
##########
nifi-extension-bundles/nifi-protobuf-bundle/nifi-protobuf-services/src/main/java/org/apache/nifi/services/protobuf/StandardProtobufReader.java:
##########
@@ -170,6 +174,84 @@ protected PropertyDescriptor buildSchemaTextProperty() {
return PROTOBUF_SCHEMA_TEXT;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(final
ValidationContext validationContext) {
+ final List<ValidationResult> problems = new
ArrayList<>(super.customValidate(validationContext));
+ final String schemaAccessStrategyValue =
validationContext.getProperty(SCHEMA_ACCESS_STRATEGY).getValue();
+
+ // Only validate when using Schema Text property strategy
+ if (SCHEMA_TEXT_PROPERTY.getValue().equals(schemaAccessStrategyValue))
{
+ final PropertyValue schemaTextProperty =
validationContext.getProperty(SCHEMA_TEXT);
+ final String schemaTextValue = schemaTextProperty.getValue();
+
+ if
(validationContext.isExpressionLanguageSupported(SCHEMA_TEXT.getName())
+ &&
validationContext.isExpressionLanguagePresent(schemaTextValue)) {
+ return Collections.emptyList();
+ }
+
+ if (schemaTextValue == null || schemaTextValue.isBlank()) {
+ problems.add(new ValidationResult.Builder()
+ .subject(SCHEMA_TEXT.getDisplayName())
+ .input(schemaTextValue)
+ .valid(false)
+ .explanation("Schema Text cannot be empty when using \"Use
'Schema Text' Property\" strategy")
+ .build());
+ return problems;
+ }
+
+ try {
+ // Try to compile the schema to validate it's valid protobuf
format
Review Comment:
```suggestion
// Try to compile the schema to validate protobuf format
```
##########
nifi-extension-bundles/nifi-protobuf-bundle/nifi-protobuf-services/src/main/java/org/apache/nifi/services/protobuf/StandardProtobufReader.java:
##########
@@ -170,6 +174,84 @@ protected PropertyDescriptor buildSchemaTextProperty() {
return PROTOBUF_SCHEMA_TEXT;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(final
ValidationContext validationContext) {
+ final List<ValidationResult> problems = new
ArrayList<>(super.customValidate(validationContext));
+ final String schemaAccessStrategyValue =
validationContext.getProperty(SCHEMA_ACCESS_STRATEGY).getValue();
+
+ // Only validate when using Schema Text property strategy
+ if (SCHEMA_TEXT_PROPERTY.getValue().equals(schemaAccessStrategyValue))
{
+ final PropertyValue schemaTextProperty =
validationContext.getProperty(SCHEMA_TEXT);
+ final String schemaTextValue = schemaTextProperty.getValue();
+
+ if
(validationContext.isExpressionLanguageSupported(SCHEMA_TEXT.getName())
+ &&
validationContext.isExpressionLanguagePresent(schemaTextValue)) {
+ return Collections.emptyList();
+ }
+
+ if (schemaTextValue == null || schemaTextValue.isBlank()) {
+ problems.add(new ValidationResult.Builder()
+ .subject(SCHEMA_TEXT.getDisplayName())
+ .input(schemaTextValue)
+ .valid(false)
+ .explanation("Schema Text cannot be empty when using \"Use
'Schema Text' Property\" strategy")
+ .build());
+ return problems;
+ }
+
+ try {
+ // Try to compile the schema to validate it's valid protobuf
format
+ final String hash = sha256Hex(schemaTextValue);
+ final SchemaIdentifier schemaIdentifier =
SchemaIdentifier.builder()
+ .name(hash + PROTO_EXTENSION)
+ .build();
+ final Schema compiledSchema =
validateSchemaCompiles(schemaIdentifier, schemaTextValue);
+
+ // Validate Message Name if using MESSAGE_NAME_PROPERTY
strategy
+ final String messageNameStrategy =
validationContext.getProperty(MESSAGE_NAME_RESOLUTION_STRATEGY).getValue();
+ if
(MESSAGE_NAME_PROPERTY.getValue().equals(messageNameStrategy)) {
+ final PropertyValue messageNameProperty =
validationContext.getProperty(MESSAGE_NAME);
+ final String messageNameValue =
messageNameProperty.getValue();
+
+ if
(validationContext.isExpressionLanguageSupported(MESSAGE_NAME.getName())
+ &&
validationContext.isExpressionLanguagePresent(messageNameValue)) {
+ return problems;
+ }
+
+ if (messageNameValue != null &&
!messageNameValue.isBlank()) {
+ // Check if the message name exists in the compiled
schema
+ if (compiledSchema.getType(messageNameValue) == null) {
+ problems.add(new ValidationResult.Builder()
+ .subject(MESSAGE_NAME.getDisplayName())
+ .input(messageNameValue)
+ .valid(false)
+ .explanation(String.format("Message name '%s'
cannot be found in the provided protobuf schema", messageNameValue))
+ .build());
+ }
+ }
+ }
+
+ } catch (final SchemaCompilationException e) {
+ problems.add(new ValidationResult.Builder()
+ .subject(SCHEMA_TEXT.getDisplayName())
+ .input(schemaTextValue)
+ .valid(false)
+ .explanation("Invalid protobuf schema format: " +
e.getMessage())
+ .build());
+ }
+ }
+
+ return problems;
+ }
+
+ // Compile the schema to validate it's correct. The method is used only
for validation purposes.
Review Comment:
This comment mostly reiterates the method name, and the usage in validation
is clear, so recommend removing this comment.
##########
nifi-extension-bundles/nifi-protobuf-bundle/nifi-protobuf-services/src/main/java/org/apache/nifi/services/protobuf/StandardProtobufReader.java:
##########
@@ -170,6 +174,84 @@ protected PropertyDescriptor buildSchemaTextProperty() {
return PROTOBUF_SCHEMA_TEXT;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(final
ValidationContext validationContext) {
+ final List<ValidationResult> problems = new
ArrayList<>(super.customValidate(validationContext));
Review Comment:
Results can also indicate a valid status, so the variable name should be
changed.
```suggestion
final List<ValidationResult> results = new
ArrayList<>(super.customValidate(validationContext));
```
##########
nifi-extension-bundles/nifi-protobuf-bundle/nifi-protobuf-services/src/test/java/org/apache/nifi/services/protobuf/TestStandardProtobufReaderPropertyValidation.java:
##########
@@ -84,6 +84,62 @@ void testValidWhenSchemaTextPropertyRemoved() { // default
value of ${proto.sche
enableAllControllerServices();
runner.assertValid(standardProtobufReader);
}
+
+ @Test
+ void testInvalidSchemaTextFormat() {
+ runner.setProperty(standardProtobufReader, SCHEMA_TEXT, "invalid
protobuf schema");
+ final ValidationResult invalidResult =
verifyExactlyOneValidationError();
+
+ assertTrue(invalidResult.getExplanation().contains("Invalid
protobuf schema format"));
+ }
+
+ @Test
+ void testInvalidMessageNameInSchema() {
+ final String validProtobufSchema = """
+ syntax = "proto3";
+ package test;
+ message Person {
+ string name = 1;
+ int32 age = 2;
+ }
+ """;
+
+ runner.setProperty(standardProtobufReader, SCHEMA_TEXT,
validProtobufSchema);
+ runner.setProperty(standardProtobufReader,
MESSAGE_NAME_RESOLUTION_STRATEGY, MESSAGE_NAME_PROPERTY);
+ runner.setProperty(standardProtobufReader, MESSAGE_NAME,
"test.NonExistentMessage");
+
+ final ValidationResult invalidResult =
verifyExactlyOneValidationError();
+
+ assertTrue(invalidResult.getExplanation().contains("Message name
'test.NonExistentMessage' cannot be found"));
Review Comment:
Recommend reducing the search string:
```suggestion
assertTrue(invalidResult.getExplanation().contains("test.NonExistentMessage"));
```
##########
nifi-extension-bundles/nifi-protobuf-bundle/nifi-protobuf-services/src/main/java/org/apache/nifi/services/protobuf/StandardProtobufReader.java:
##########
@@ -170,6 +174,84 @@ protected PropertyDescriptor buildSchemaTextProperty() {
return PROTOBUF_SCHEMA_TEXT;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(final
ValidationContext validationContext) {
+ final List<ValidationResult> problems = new
ArrayList<>(super.customValidate(validationContext));
+ final String schemaAccessStrategyValue =
validationContext.getProperty(SCHEMA_ACCESS_STRATEGY).getValue();
+
+ // Only validate when using Schema Text property strategy
+ if (SCHEMA_TEXT_PROPERTY.getValue().equals(schemaAccessStrategyValue))
{
+ final PropertyValue schemaTextProperty =
validationContext.getProperty(SCHEMA_TEXT);
+ final String schemaTextValue = schemaTextProperty.getValue();
+
+ if
(validationContext.isExpressionLanguageSupported(SCHEMA_TEXT.getName())
Review Comment:
Is the `isExpressionLanguageSupported` check needed?
--
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]
