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