-
Notifications
You must be signed in to change notification settings - Fork 998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHOENIX-6560 Dynamic SQL queries to use Preparedstatement #1429
Conversation
recheck |
1 similar comment
recheck |
9d52a7d
to
21857aa
Compare
recheck |
phoenixConnection.createStatement().execute(deleteQuery); | ||
try { | ||
phoenixConnection.prepareStatement(deleteQuery).execute(); | ||
} catch (SQLException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why to handle the SQLException and make it IOException better to through the SQLException as it is?
import java.sql.DatabaseMetaData; | ||
import java.sql.ResultSet; | ||
import java.sql.SQLException; | ||
import java.sql.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use the * in imports, only import necessary classes.
+ ASYNC_REBUILD_TIMESTAMP + "), MAX(" + INDEX_DISABLE_TIMESTAMP | ||
+ ") FROM " + SYSTEM_CATALOG_NAME | ||
+ " (" + ASYNC_REBUILD_TIMESTAMP + " BIGINT) WHERE " + TABLE_SCHEM | ||
+ (schemaName != null && schemaName.length() > 0 ? "='" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ? here and set the value as a parameter to the query.
+ (schemaName != null && schemaName.length() > 0 ? "='" | ||
+ schemaName + "'" : " IS NULL") | ||
+ " and " + TABLE_NAME + " IN (" | ||
+ StringUtils.join(",", quotedIndexes) + ")")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use parameter here also.
); | ||
try (PreparedStatement stmt = metaConnection.prepareStatement("ALTER TABLE " | ||
+ PhoenixDatabaseMetaData.SYSTEM_CATALOG + " SET " | ||
+ HConstants.VERSIONS + "= " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be parameter as well.
+ HConstants.VERSIONS + "= " | ||
+ props.getInt(DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB, | ||
QueryServicesOptions.DEFAULT_SYSTEM_MAX_VERSIONS) + ",\n" | ||
+ ColumnFamilyDescriptorBuilder.KEEP_DELETED_CELLS + "=" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be parameter as well.
try { | ||
connection.prepareStatement(changeTable).execute(); | ||
} catch (SQLException e) { | ||
throw new SQLException("Error during cutover via change table"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is overriding exact error reported from the query better need not handle SQLException and through the same Exception.
try { | ||
connection.prepareStatement(changeView).execute(); | ||
} catch (SQLException e) { | ||
throw new SQLException("Error during cutover via change views"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here not required to handle the error and throw the same exception.
+ PhoenixDatabaseMetaData.SYSTEM_SEQUENCE | ||
+ " WHERE " + PhoenixDatabaseMetaData.SEQUENCE_SCHEMA | ||
+ (schemaName.length() > 0 ? "='" + schemaName + "'" : " IS NULL") | ||
+ " AND " + PhoenixDatabaseMetaData.SEQUENCE_NAME + " = '" + sequenceName + "'" ); | ||
+ " AND " + PhoenixDatabaseMetaData.SEQUENCE_NAME + " = '" + sequenceName + "'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be parameter as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Rajeshbabu said.
Using preparedstatements does not help in itself, the point here is to avoid sql injection attacks by avoiding string concatenation and using parameters instead.
rebuild |
retest |
No description provided.