mattyb149 commented on a change in pull request #4528:
URL: https://github.com/apache/nifi/pull/4528#discussion_r498888543
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -249,7 +271,7 @@
properties.add(TABLE_NAME);
properties.add(CATALOG_NAME);
properties.add(SCHEMA_NAME);
- properties.add(TRANSLATE_FIELD_NAMES);
Review comment:
Removing a field from the list of supported property descriptors will
cause all existing flows (after upgrade) to mark these processors as Invalid. I
think we should keep this field and mention in the description of Translate
Field Expression that it will be ignored if Translate Field Names is false.
That means the other logic below would have to change slightly as well, to
provide the default expression if Translate Field Names is set to true.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -124,6 +129,18 @@
"Fail on Unmatched Columns",
"A flow will fail if any column in the database that does not have
a field in the JSON document. An error will be logged");
+ public static final Validator TRANSLATE_FIELD_EXPRESSION_VALIDATOR = new
Validator() {
Review comment:
I'm not sure we need a special field validator for this (see my comments
below), I think a NON_EMPTY_EL_VALIDATOR would suffice.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -146,6 +148,18 @@
protected static Set<Relationship> relationships;
+ public static final Validator TRANSLATE_FIELD_EXPRESSION_VALIDATOR = new
Validator() {
Review comment:
All the same comments for ConvertJSONToSQL apply here as well
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -158,12 +175,17 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
- static final PropertyDescriptor TRANSLATE_FIELD_NAMES = new
PropertyDescriptor.Builder()
- .name("Translate Field Names")
- .description("If true, the Processor will attempt to translate
JSON field names into the appropriate column names for the table specified. "
- + "If false, the JSON field names must match the column
names exactly, or the column will not be updated")
- .allowableValues("true", "false")
- .defaultValue("true")
+ static final PropertyDescriptor TRANSLATE_FIELD_EXPRESSION = new
PropertyDescriptor.Builder()
+ .name("put-db-record-translate-expression")
+ .displayName("Translate Field Expression")
+ .description("If set, the Processor will attempt to translate
field names into the appropriate column names for the table specified using "
+ + "the Expression language, and the expression's value
should start with '${colName:'. If not set, the field names must match the
column names exactly, "
Review comment:
We may want to mention here that we are translating both field and
column names in order to match the two, rather than just translating field
names and matching against actual column names. That makes `colName` a bit of
an awkward choice, but I think users would understand what it means (with the
additional doc I just suggested). In fact `columnName` might be even more
helpful.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -158,12 +175,17 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
- static final PropertyDescriptor TRANSLATE_FIELD_NAMES = new
PropertyDescriptor.Builder()
- .name("Translate Field Names")
- .description("If true, the Processor will attempt to translate
JSON field names into the appropriate column names for the table specified. "
- + "If false, the JSON field names must match the column
names exactly, or the column will not be updated")
- .allowableValues("true", "false")
- .defaultValue("true")
+ static final PropertyDescriptor TRANSLATE_FIELD_EXPRESSION = new
PropertyDescriptor.Builder()
+ .name("put-db-record-translate-expression")
+ .displayName("Translate Field Expression")
+ .description("If set, the Processor will attempt to translate
field names into the appropriate column names for the table specified using "
+ + "the Expression language, and the expression's value
should start with '${colName:'. If not set, the field names must match the
column names exactly, "
+ + "or the column will not be updated. Note that the scope
of expression language is 'Variable Registry and FlowFile Attributes', "
+ + "but we would not use them when evaluating.")
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .required(false)
+ .defaultValue("${colName:toUpper():replace('_','')}")
Review comment:
Since you are adding the column name as a `colName` attribute before
evaluating the Expression Language expression, I think requiring the entire
expression's value to start with `${colName` is more strict than it has to be.
Instead we could just document that the `colName` (or `columnName`, see above)
is available for use in any EL expression. I imagine most use cases will see
`colName` as the subject (and indeed it has to be part of the expression or
else it will not match one of the field or column), and most operations can be
done that way (append, prepend, e.g.), but I like to make things as flexible as
possible.
The documentation can include good examples of how to use this the intended
way, but flexibility may allow for support of more use cases.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java
##########
@@ -158,12 +175,17 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
- static final PropertyDescriptor TRANSLATE_FIELD_NAMES = new
PropertyDescriptor.Builder()
- .name("Translate Field Names")
- .description("If true, the Processor will attempt to translate
JSON field names into the appropriate column names for the table specified. "
- + "If false, the JSON field names must match the column
names exactly, or the column will not be updated")
- .allowableValues("true", "false")
- .defaultValue("true")
+ static final PropertyDescriptor TRANSLATE_FIELD_EXPRESSION = new
PropertyDescriptor.Builder()
+ .name("put-db-record-translate-expression")
+ .displayName("Translate Field Expression")
+ .description("If set, the Processor will attempt to translate
field names into the appropriate column names for the table specified using "
+ + "the Expression language, and the expression's value
should start with '${colName:'. If not set, the field names must match the
column names exactly, "
+ + "or the column will not be updated. Note that the scope
of expression language is 'Variable Registry and FlowFile Attributes', "
+ + "but we would not use them when evaluating.")
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
Review comment:
You shouldn't need to set this to FLOWFILE_ATTRIBUTES in order to call
the method that evaluates the expression based on the single `colName`
attribute, you can just call the version that takes a Map and no FlowFile
reference.
Having said that, if you make the expression more flexible (see below), then
you could support Variably Registry and FlowFile attributes in the Translate
Field Expression property, you'd just need to document that `colName` will
contain the field/column name and will override any value that had been
previously set for that attribute.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
