-
Notifications
You must be signed in to change notification settings - Fork 426
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
Improvements in Database Metadata to prevent Statement leaks and enhance Statement caching #806
Improvements in Database Metadata to prevent Statement leaks and enhance Statement caching #806
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #806 +/- ##
============================================
- Coverage 48.58% 48.57% -0.01%
+ Complexity 2794 2791 -3
============================================
Files 116 116
Lines 27879 27890 +11
Branches 4651 4652 +1
============================================
+ Hits 13545 13548 +3
- Misses 12124 12141 +17
+ Partials 2210 2201 -9
Continue to review full report at Codecov.
|
Jars for testing : mssql-jdbc-PR806.zip |
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDatabaseMetaData.java
Show resolved
Hide resolved
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDatabaseMetaData.java
Show resolved
Hide resolved
SQLServerResultSet rsMoreMetaData = (!connection | ||
.getServerSupportsColumnEncryption() ? statementMoreMetadata | ||
.executeQueryInternal("select collation_name from sys.columns where " | ||
+ "object_id=OBJECT_ID('" + Util.escapeSingleQuotes(destinationTableName) + "') " |
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 less readable to me. Maybe you could only "if else" the query string?
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.
Then we can't place it in try with resources block. Also will need another try block, and finally block to manually close it.
I don't see any issues - its a simple ternary operator.
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.
+ "object_id=OBJECT_ID('" + Util.escapeSingleQuotes(destinationTableName) + "') " | |
/** | |
* Returns the column metadata for the destination table (and saves it for later) | |
*/ | |
private void getDestinationMetadata() throws SQLServerException { | |
if (null == destinationTableName) { | |
SQLServerException.makeFromDriverError(null, null, | |
SQLServerException.getErrString("R_invalidDestinationTable"), null, false); | |
} | |
SQLServerResultSet rs = null; | |
SQLServerStatement stmt = null; | |
String metaDataQuery = null; | |
String bulkCopyEncrytionType = null; | |
String escapedTableName = Util.escapeSingleQuotes(destinationTableName); | |
try { | |
if (null != destinationTableMetadata) { | |
rs = (SQLServerResultSet) destinationTableMetadata; | |
} else { | |
stmt = (SQLServerStatement) connection.createStatement(ResultSet.TYPE_FORWARD_ONLY, | |
ResultSet.CONCUR_READ_ONLY, connection.getHoldability(), stmtColumnEncriptionSetting); | |
// Get destination metadata | |
rs = stmt.executeQueryInternal( | |
"sp_executesql N'SET FMTONLY ON SELECT * FROM " + escapedTableName + " '"); | |
} | |
destColumnCount = rs.getMetaData().getColumnCount(); | |
destColumnMetadata = new HashMap<>(); | |
destCekTable = rs.getCekTable(); | |
if (connection.getServerSupportsColumnEncryption()) { | |
metaDataQuery = "select collation_name, encryption_type from sys.columns where " | |
+ "object_id=OBJECT_ID('" + escapedTableName + "') " + "order by column_id ASC"; | |
} else { | |
metaDataQuery = "select collation_name from sys.columns where " + "object_id=OBJECT_ID('" | |
+ escapedTableName + "') " + "order by column_id ASC"; | |
} | |
try (SQLServerStatement statementMoreMetadata = (SQLServerStatement) connection.createStatement(); | |
SQLServerResultSet rsMoreMetaData = statementMoreMetadata.executeQueryInternal(metaDataQuery)) { | |
for (int i = 1; i <= destColumnCount; ++i) { | |
if (rsMoreMetaData.next()) { | |
if (connection.getServerSupportsColumnEncryption()) { | |
bulkCopyEncrytionType = rsMoreMetaData.getString("encryption_type"); | |
} | |
destColumnMetadata.put(i, new BulkColumnMetaData(rs.getColumn(i), bulkCopyEncrytionType, null)); | |
} else { | |
destColumnMetadata.put(i, new BulkColumnMetaData(rs.getColumn(i))); | |
} | |
} | |
} | |
} catch (SQLException e) { | |
// Unable to retrieve metadata for destination | |
throw new SQLServerException(SQLServerException.getErrString("R_unableRetrieveColMeta"), e); | |
} finally { | |
if (null != rs) | |
rs.close(); | |
if (null != stmt) | |
stmt.close(); | |
} | |
} |
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.
Please consider the suggestion above.
This PR introduces below changes to SQLServerDatabaseMetadata: