[
https://issues.apache.org/jira/browse/PHOENIX-6560?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17538570#comment-17538570
]
ASF GitHub Bot commented on PHOENIX-6560:
-----------------------------------------
chrajeshbabu commented on code in PR #1429:
URL: https://github.com/apache/phoenix/pull/1429#discussion_r875451392
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/OrphanViewTool.java:
##########
@@ -445,7 +445,11 @@ private void removeLink(PhoenixConnection
phoenixConnection, Key src, Key dst, P
COLUMN_NAME + (dst.getTenantId() == null ? " IS NULL" : " = '"
+ dst.getTenantId() + "'") + " AND " +
COLUMN_FAMILY + " = '" + (dst.getSchemaName() == null ?
dst.getTableName() : dst.getSchemaName() + "." +
dst.getTableName()) + "'";
- phoenixConnection.createStatement().execute(deleteQuery);
+ try {
+ phoenixConnection.prepareStatement(deleteQuery).execute();
+ } catch (SQLException e) {
Review Comment:
Why to handle the SQLException and make it IOException better to through the
SQLException as it is?
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java:
##########
@@ -605,21 +602,27 @@ private long getMaxRebuildAsyncDate(String schemaName,
List<String> disableIndex
for (String index : disableIndexes) {
quotedIndexes.add("'" + index + "'");
}
- try (ResultSet rs = connection.createStatement()
- .executeQuery("SELECT MAX(" + ASYNC_REBUILD_TIMESTAMP +
"),MAX("+INDEX_DISABLE_TIMESTAMP+") FROM " + SYSTEM_CATALOG_NAME + " ("
- + ASYNC_REBUILD_TIMESTAMP + " BIGINT) WHERE " +
TABLE_SCHEM
- + (schemaName != null && schemaName.length() > 0 ?
"='" + schemaName + "'" : " IS NULL")
- + " and " + TABLE_NAME + " IN (" +
StringUtils.join(",", quotedIndexes) + ")")) {
- if (rs.next()) {
- maxRebuilAsyncDate = rs.getLong(1);
- maxDisabledTimeStamp = rs.getLong(2);
- }
- // Do check if table is disabled again after user invoked
async rebuilding during the run of the job
- if (maxRebuilAsyncDate > maxDisabledTimeStamp) {
- return maxRebuilAsyncDate;
- } else {
- throw new RuntimeException(
- "Inconsistent state we have one or more index
tables which are disabled after the async is called!!");
+ try (PreparedStatement stmt = connection.prepareStatement("SELECT
MAX("
+ + ASYNC_REBUILD_TIMESTAMP + "), MAX(" +
INDEX_DISABLE_TIMESTAMP
+ + ") FROM " + SYSTEM_CATALOG_NAME
+ + " (" + ASYNC_REBUILD_TIMESTAMP + " BIGINT) WHERE " +
TABLE_SCHEM
+ + (schemaName != null && schemaName.length() > 0 ? "='"
+ + schemaName + "'" : " IS NULL")
+ + " and " + TABLE_NAME + " IN ("
+ + StringUtils.join(",", quotedIndexes) + ")")) {
Review Comment:
You can use parameter here also.
##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:
##########
@@ -3847,21 +3847,34 @@ protected PhoenixConnection
upgradeSystemCatalogIfRequired(PhoenixConnection met
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_14_0,
PhoenixDatabaseMetaData.TRANSACTION_PROVIDER + " "
+ PTinyint.INSTANCE.getSqlTypeName());
- metaConnection.createStatement().executeUpdate("ALTER TABLE " +
- PhoenixDatabaseMetaData.SYSTEM_CATALOG + " SET " +
- HConstants.VERSIONS + "= " +
props.getInt(DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_MAX_VERSIONS) + ",\n" +
- ColumnFamilyDescriptorBuilder.KEEP_DELETED_CELLS + "=" +
props.getBoolean(DEFAULT_SYSTEM_KEEP_DELETED_CELLS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_KEEP_DELETED_CELLS)
- );
- metaConnection.createStatement().executeUpdate("ALTER TABLE " +
- PhoenixDatabaseMetaData.SYSTEM_FUNCTION + " SET " +
- TableDescriptorBuilder.SPLIT_POLICY + "='" +
SystemFunctionSplitPolicy.class.getName() + "',\n" +
- HConstants.VERSIONS + "= " +
props.getInt(DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_MAX_VERSIONS) + ",\n" +
- ColumnFamilyDescriptorBuilder.KEEP_DELETED_CELLS + "=" +
props.getBoolean(DEFAULT_SYSTEM_KEEP_DELETED_CELLS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_KEEP_DELETED_CELLS)
- );
- metaConnection.createStatement().executeUpdate("ALTER TABLE " +
- PhoenixDatabaseMetaData.SYSTEM_STATS_NAME + " SET " +
- TableDescriptorBuilder.SPLIT_POLICY + "='" +
SystemStatsSplitPolicy.class.getName() +"'"
- );
+ try (PreparedStatement stmt =
metaConnection.prepareStatement("ALTER TABLE "
+ + PhoenixDatabaseMetaData.SYSTEM_CATALOG + " SET "
+ + HConstants.VERSIONS + "= "
+ + props.getInt(DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB,
+ QueryServicesOptions.DEFAULT_SYSTEM_MAX_VERSIONS) + ",\n"
+ + ColumnFamilyDescriptorBuilder.KEEP_DELETED_CELLS + "="
Review Comment:
This can be parameter as well.
##########
phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java:
##########
@@ -903,11 +903,12 @@ public static void
deleteViewIndexSequences(PhoenixConnection connection, PName
throws SQLException {
String schemaName = getViewIndexSequenceSchemaName(name,
isNamespaceMapped);
String sequenceName = getViewIndexSequenceName(name, null,
isNamespaceMapped);
- connection.createStatement().executeUpdate("DELETE FROM "
+ connection.prepareStatement("DELETE FROM "
+ PhoenixDatabaseMetaData.SYSTEM_SEQUENCE
+ " WHERE " + PhoenixDatabaseMetaData.SEQUENCE_SCHEMA
+ (schemaName.length() > 0 ? "='" + schemaName + "'" : " IS
NULL")
- + " AND " + PhoenixDatabaseMetaData.SEQUENCE_NAME + " = '" +
sequenceName + "'" );
+ + " AND " + PhoenixDatabaseMetaData.SEQUENCE_NAME + " = '" +
sequenceName + "'")
Review Comment:
This can be parameter as well.
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/transform/Transform.java:
##########
@@ -488,7 +488,11 @@ public static void doCutover(PhoenixConnection connection,
SystemTransformRecord
connection.setAutoCommit(false);
List<TableInfo> viewsToUpdateCache = new ArrayList<>();
try {
- connection.createStatement().execute(changeTable);
+ try {
+ connection.prepareStatement(changeTable).execute();
+ } catch (SQLException e) {
+ throw new SQLException("Error during cutover via change
table");
Review Comment:
This is overriding exact error reported from the query better need not
handle SQLException and through the same Exception.
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java:
##########
@@ -26,10 +26,7 @@
import static
org.apache.phoenix.mapreduce.index.IndexVerificationResultRepository.ROW_KEY_SEPARATOR;
import java.io.IOException;
-import java.sql.Connection;
-import java.sql.DatabaseMetaData;
-import java.sql.ResultSet;
-import java.sql.SQLException;
+import java.sql.*;
Review Comment:
Don't use the * in imports, only import necessary classes.
##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:
##########
@@ -3847,21 +3847,34 @@ protected PhoenixConnection
upgradeSystemCatalogIfRequired(PhoenixConnection met
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_14_0,
PhoenixDatabaseMetaData.TRANSACTION_PROVIDER + " "
+ PTinyint.INSTANCE.getSqlTypeName());
- metaConnection.createStatement().executeUpdate("ALTER TABLE " +
- PhoenixDatabaseMetaData.SYSTEM_CATALOG + " SET " +
- HConstants.VERSIONS + "= " +
props.getInt(DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_MAX_VERSIONS) + ",\n" +
- ColumnFamilyDescriptorBuilder.KEEP_DELETED_CELLS + "=" +
props.getBoolean(DEFAULT_SYSTEM_KEEP_DELETED_CELLS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_KEEP_DELETED_CELLS)
- );
- metaConnection.createStatement().executeUpdate("ALTER TABLE " +
- PhoenixDatabaseMetaData.SYSTEM_FUNCTION + " SET " +
- TableDescriptorBuilder.SPLIT_POLICY + "='" +
SystemFunctionSplitPolicy.class.getName() + "',\n" +
- HConstants.VERSIONS + "= " +
props.getInt(DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_MAX_VERSIONS) + ",\n" +
- ColumnFamilyDescriptorBuilder.KEEP_DELETED_CELLS + "=" +
props.getBoolean(DEFAULT_SYSTEM_KEEP_DELETED_CELLS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_KEEP_DELETED_CELLS)
- );
- metaConnection.createStatement().executeUpdate("ALTER TABLE " +
- PhoenixDatabaseMetaData.SYSTEM_STATS_NAME + " SET " +
- TableDescriptorBuilder.SPLIT_POLICY + "='" +
SystemStatsSplitPolicy.class.getName() +"'"
- );
+ try (PreparedStatement stmt =
metaConnection.prepareStatement("ALTER TABLE "
+ + PhoenixDatabaseMetaData.SYSTEM_CATALOG + " SET "
+ + HConstants.VERSIONS + "= "
Review Comment:
This can be parameter as well.
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java:
##########
@@ -605,21 +602,27 @@ private long getMaxRebuildAsyncDate(String schemaName,
List<String> disableIndex
for (String index : disableIndexes) {
quotedIndexes.add("'" + index + "'");
}
- try (ResultSet rs = connection.createStatement()
- .executeQuery("SELECT MAX(" + ASYNC_REBUILD_TIMESTAMP +
"),MAX("+INDEX_DISABLE_TIMESTAMP+") FROM " + SYSTEM_CATALOG_NAME + " ("
- + ASYNC_REBUILD_TIMESTAMP + " BIGINT) WHERE " +
TABLE_SCHEM
- + (schemaName != null && schemaName.length() > 0 ?
"='" + schemaName + "'" : " IS NULL")
- + " and " + TABLE_NAME + " IN (" +
StringUtils.join(",", quotedIndexes) + ")")) {
- if (rs.next()) {
- maxRebuilAsyncDate = rs.getLong(1);
- maxDisabledTimeStamp = rs.getLong(2);
- }
- // Do check if table is disabled again after user invoked
async rebuilding during the run of the job
- if (maxRebuilAsyncDate > maxDisabledTimeStamp) {
- return maxRebuilAsyncDate;
- } else {
- throw new RuntimeException(
- "Inconsistent state we have one or more index
tables which are disabled after the async is called!!");
+ try (PreparedStatement stmt = connection.prepareStatement("SELECT
MAX("
+ + ASYNC_REBUILD_TIMESTAMP + "), MAX(" +
INDEX_DISABLE_TIMESTAMP
+ + ") FROM " + SYSTEM_CATALOG_NAME
+ + " (" + ASYNC_REBUILD_TIMESTAMP + " BIGINT) WHERE " +
TABLE_SCHEM
+ + (schemaName != null && schemaName.length() > 0 ? "='"
Review Comment:
Use ? here and set the value as a parameter to the query.
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/transform/Transform.java:
##########
@@ -525,7 +529,11 @@ public static void doCutover(PhoenixConnection connection,
SystemTransformRecord
Bytes.toString(view.getTableName()),
(columnValues.size() > 0? "," + String.join(",",
columnValues):""));
LOGGER.info("Cutover changing view via " + changeView);
- connection.createStatement().execute(changeView);
+ try {
+ connection.prepareStatement(changeView).execute();
+ } catch (SQLException e) {
+ throw new SQLException("Error during cutover via change
views");
Review Comment:
Same here not required to handle the error and throw the same exception.
> Rewrite dynamic SQL queries to use Preparedstatement
> ----------------------------------------------------
>
> Key: PHOENIX-6560
> URL: https://issues.apache.org/jira/browse/PHOENIX-6560
> Project: Phoenix
> Issue Type: Improvement
> Components: core
> Reporter: Istvan Toth
> Assignee: Abhishek Kothalikar
> Priority: Major
>
> Most of the Phoenix code base already uses PreparedStatements, and adds all
> potentially vulnerable data as parameters.
> However, there are some places where we concatenate potentially problematic
> strings into the query.
> While most of those are constants and such, we should preferably pass all
> data as parameters to be on the safe side.
> (We still have to use dynamic strings for the preparedstatement strings, for
> handling things as is null, empty in clauses and such)
> Spotbugs marks these with SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE, so
> they're easy to find.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
