EndzeitBegins commented on code in PR #9516:
URL: https://github.com/apache/nifi/pull/9516#discussion_r1841075621
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GenerateFlowFile.java:
##########
@@ -111,8 +116,19 @@ public class GenerateFlowFile extends AbstractProcessor {
+ "per batch of generated FlowFiles")
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .dependsOn(DATA_FORMAT, DATA_FORMAT_TEXT)
+ .dependsOn(UNIQUE_FLOWFILES, "false")
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+ public static final PropertyDescriptor CUSTOM_CONTENT = new
PropertyDescriptor.Builder()
+ .name("Custom Content")
+ .description("Path to a file. If specified, this file will be used
as content of the generated FlowFiles and the File Size will be ignored.")
Review Comment:
Does it make sense to declare the `FILE_SIZE` with `.dependsOn(DATA_FORMAT,
DATA_FORMAT_BINARY, DATA_FORMAT_TEXT)`, Instead of documenting that the `File
Size` will be ignored here?
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GenerateFlowFile.java:
##########
@@ -111,8 +116,19 @@ public class GenerateFlowFile extends AbstractProcessor {
+ "per batch of generated FlowFiles")
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .dependsOn(DATA_FORMAT, DATA_FORMAT_TEXT)
+ .dependsOn(UNIQUE_FLOWFILES, "false")
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+ public static final PropertyDescriptor CUSTOM_CONTENT = new
PropertyDescriptor.Builder()
+ .name("Custom Content")
+ .description("Path to a file. If specified, this file will be used
as content of the generated FlowFiles and the File Size will be ignored.")
+ .required(false)
Review Comment:
I think this should be `.required(true)` as this only takes effect In case
the `dependsOn` conditions are met, in which case we want this property to be
set, right?
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GenerateFlowFile.java:
##########
@@ -183,12 +200,22 @@ protected Collection<ValidationResult>
customValidate(ValidationContext validati
final List<ValidationResult> results = new ArrayList<>(1);
final boolean isUnique =
validationContext.getProperty(UNIQUE_FLOWFILES).asBoolean();
final boolean isText =
validationContext.getProperty(DATA_FORMAT).getValue().equals(DATA_FORMAT_TEXT);
+ final boolean isExternal =
validationContext.getProperty(DATA_FORMAT).getValue().equals(DATA_FORMAT_EXTERNAL_FILE);
final boolean isCustom =
validationContext.getProperty(CUSTOM_TEXT).isSet();
+ final boolean isCustomContent =
validationContext.getProperty(CUSTOM_CONTENT).isSet();
if (isCustom && (isUnique || !isText)) {
results.add(new ValidationResult.Builder().subject("Custom
Text").valid(false).explanation("If Custom Text is set, then Data Format must
be "
+ "text and Unique FlowFiles must be false.").build());
}
+ if (isCustomContent && (isUnique || !isExternal)) {
+ results.add(new ValidationResult.Builder().subject("Custom
Content").valid(false).explanation("If Custom Content is set, then Data Format
must be "
+ + "External File and Unique FlowFiles must be
false.").build());
+ }
+ if (isCustomContent && isCustom) {
Review Comment:
Do we need this validation explicitly? The `dependsOn` conditionals should
result in only on of `Custom Text` or `Custom Content` being available to the
user I assume?
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GenerateFlowFile.java:
##########
@@ -111,8 +116,19 @@ public class GenerateFlowFile extends AbstractProcessor {
+ "per batch of generated FlowFiles")
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .dependsOn(DATA_FORMAT, DATA_FORMAT_TEXT)
+ .dependsOn(UNIQUE_FLOWFILES, "false")
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+ public static final PropertyDescriptor CUSTOM_CONTENT = new
PropertyDescriptor.Builder()
+ .name("Custom Content")
Review Comment:
Does it make sense to name this rather "File Content" to make it easier to
differentiate from the existing "Custom Text"?
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GenerateFlowFile.java:
##########
@@ -111,8 +116,19 @@ public class GenerateFlowFile extends AbstractProcessor {
+ "per batch of generated FlowFiles")
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .dependsOn(DATA_FORMAT, DATA_FORMAT_TEXT)
+ .dependsOn(UNIQUE_FLOWFILES, "false")
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+ public static final PropertyDescriptor CUSTOM_CONTENT = new
PropertyDescriptor.Builder()
+ .name("Custom Content")
+ .description("Path to a file. If specified, this file will be used
as content of the generated FlowFiles and the File Size will be ignored.")
+ .required(false)
+ .identifiesExternalResource(ResourceCardinality.SINGLE,
ResourceType.FILE, ResourceType.URL, ResourceType.TEXT)
Review Comment:
Doesn't this warrant declaring the processor to require `READ_FILESYSTEM`
permission? Is that a problem in regards to backwards compatibility?
--
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]
