pvillard31 commented on code in PR #11397:
URL: https://github.com/apache/nifi/pull/11397#discussion_r3532191571
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UpdateDatabaseTable.java:
##########
@@ -708,11 +708,22 @@ private TableDefinition getTableDefinition(final
TableSchema tableSchema) {
);
}
- private String enquoteIdentifier(final String identifier, final String
quotedIdentifierString, final boolean quoteIdentifier) {
- if (identifier != null && quoteIdentifier) {
- return quotedIdentifierString + identifier +
quotedIdentifierString;
+ private String enquoteIdentifier(final Statement statement, final String
identifier, final boolean quoteIdentifier) {
+ final String enquoted;
+
+ if (identifier == null) {
+ enquoted = identifier;
+ } else if (quoteIdentifier) {
+ try {
+ enquoted = statement.enquoteIdentifier(identifier,
quoteIdentifier);
+ } catch (final SQLException e) {
+ throw new IllegalArgumentException("Enquote Identifier [%s]
failed".formatted(identifier), e);
Review Comment:
Statement.enquoteIdentifier throws for identifiers longer than 128
characters or with invalid characters, and this wraps it into an
IllegalArgumentException. Since checkAndUpdateTableSchema runs inside the try
that only routes IOException and SQLException to failure, would such an
identifier now cause a rollback instead of routing to REL_FAILURE?
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestUpdateDatabaseTable.java:
##########
@@ -120,7 +120,7 @@ public void testCreateTable() throws Exception {
assertColumnEquals(rs, "id", 1, "INTEGER");
assertColumnEquals(rs, "fractional", 2, "DOUBLE PRECISION");
assertColumnEquals(rs, "code", 3, "INTEGER");
- assertColumnEquals(rs, "newField", 4, "DOUBLE PRECISION");
+ assertColumnEquals(rs, "spaced Field", 4, "DOUBLE PRECISION");
Review Comment:
Would it be worth adding a test for an identifier that makes
enquoteIdentifier throw (for example longer than 128 characters) to lock in the
error routing behavior?
##########
nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UpdateDatabaseTable.java:
##########
@@ -537,14 +537,14 @@ private synchronized OutputMetadataHolder
checkAndUpdateTableSchema(
}
final int dataType =
DataTypeUtils.getSQLTypeValue(recordField.getDataType());
- final String quotedColumnName =
enquoteIdentifier(columnName, quoteString, quoteColumnNames);
+ final String quotedColumnName = enquoteIdentifier(s,
columnName, quoteColumnNames);
columns.add(new ColumnDescription(quotedColumnName,
dataType, required, null, recordField.isNullable()));
getLogger().debug("Adding column {} to table {}",
columnName, tableName);
}
- final String quotedCatalogName =
enquoteIdentifier(catalogName, quoteString, quoteTableName);
- final String quotedSchemaName =
enquoteIdentifier(schemaName, quoteString, quoteTableName);
- final String quotedTableName =
enquoteIdentifier(tableName, quoteString, quoteTableName);
+ final String quotedCatalogName = enquoteIdentifier(s,
catalogName, quoteTableName);
+ final String quotedSchemaName = enquoteIdentifier(s,
schemaName, quoteTableName);
+ final String quotedTableName = enquoteIdentifier(s,
tableName, quoteTableName);
tableSchema = new TableSchema(quotedCatalogName,
quotedSchemaName, quotedTableName, columns, translateFieldNames, normalizer,
primaryKeyColumnNames, quoteString);
Review Comment:
The quote character now comes from the driver's enquoteIdentifier, but
quoteString from getIdentifierQuoteString is still passed to TableSchema. Could
these two produce different quote characters for a driver that does not
override the default enquoteIdentifier?
--
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]
