-
Notifications
You must be signed in to change notification settings - Fork 423
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
Fix to ensure metadata returned follows JDBC data type specs #2326
Conversation
… is Incorrect for Some Columns (1751) Fix ensures the correct data types for each metadata column match the data type from the SQL Server and the JDBC spec
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDatabaseMetaData.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Show resolved
Hide resolved
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDatabaseMetaData.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Show resolved
Hide resolved
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDatabaseMetaData.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Outdated
Show resolved
Hide resolved
TestResource.getResource("R_classLoaderNotFoundForColumnType")); | ||
Object[] msgArgs = {columnClassName, columnLabel}; | ||
fail(form.format(msgArgs)); | ||
} |
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.
I think we just need to assertTrue or assertEqual that the expectedClass.getName().equals(columnClassName) as per repro in the issue
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.
Changed the logic for checking the returned column data type matches the expected data type to simplify the logic. The new logic removes this Class Loader test. There is a new tests that actually compares the actual and expected class name for each column but does not use an assert. Instead a R_expectedClassDoesNotMatchActualClass message is used which includes the actual and expected data types as well as the column. This was done because if just an assert was used it would not show which column was generating the issue in the metadata result set.
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Outdated
Show resolved
Hide resolved
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.
pls run formatter on all files.
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.
there should be a test case to cover the error msg R_colCountNotMatchColTypeCount path
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2326 +/- ##
=======================================
Coverage ? 50.04%
Complexity ? 3793
=======================================
Files ? 143
Lines ? 33179
Branches ? 5631
=======================================
Hits ? 16605
Misses ? 14191
Partials ? 2383 ☔ View full report in Codecov by Sentry. |
Class<?> expectedClass = getColumnMetaDataClass.get(columnLabel); | ||
|
||
// Ensure the metadata column is in the metadata column class map | ||
assertNotNull(expectedClass); |
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 was mentioned in a previous comment above, but ensure that expectedClass
is not null, don't assert.
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.
assertNotNull(expectedClass); | |
if (null != expectedClass) { |
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 really shouldn't even be necessary, we just defined this above, this would never be null
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.
did we accidentally remove the check for isAssignableFrom that was in previous commit?
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.
The expected class can be null if the the column label/name in the expected class lookup table does not match the actual column label from the result set (i.e. the lookup by column name will fail). This shouldn't happen but it is possible if there was a future code change or perhaps the returned metadata columns change.
Yes the IsAssignableFrom() is no longer used as I changed the BUFFER_LENGTH metadata class from Object to Integer. It was because of this generic Object class that we needed to use the IsAssignableFrom() check. Is The reason why it was originally specified as a generic Object was that the JDBC spec does not use the BUFFER_LENGTH column and so I reflected that as a generic Object being the expected data type. But in the code I have always ensured that BUFFER_LENGTH was an Integer. Therefore to simplify the logic and remove the IsAssignableFrom() check, I switched the expected class for the BUFFER_LENGTH column from Object to Integer.
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.
I have kept the check but no longer use an assert to perform the check. I found that in the unlikely event there was an issue, a more detail error message was required that would include not just the actual and expected types but the column as well. Without the column name/label, it would initially be difficult t determine what the problem column was in the metadata
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
Outdated
Show resolved
Hide resolved
…QLServer docker image tag to `2022-CU14-ubuntu-22.04` ### What changes were proposed in this pull request? This PR aims to upgrade `mssql-jdbc` to 12.8.0.jre11 and MySQLServer docker image to `mcr.microsoft.com/mssql/server:2022-CU14-ubuntu-22.04`. ### Why are the changes needed? This is the latest stable version of `mssql-jdbc`, related release notes: https://github.com/microsoft/mssql-jdbc/releases/tag/v12.7.0 https://github.com/microsoft/mssql-jdbc/releases/tag/v12.7.1 https://github.com/microsoft/mssql-jdbc/releases/tag/v12.8.0 Some fixed issues: - Fix to ensure metadata returned follows JDBC data type specs microsoft/mssql-jdbc#2326 - Added token cache map to fix use of unintended auth token for subsequent connections microsoft/mssql-jdbc#2341 - Clear prepared statement handle before reconnect microsoft/mssql-jdbc#2364 - Reset socketTimeout to original value after a successful connection open microsoft/mssql-jdbc#2355 - Clear prepared statement cache when resetting statement pool connection microsoft/mssql-jdbc#2361 - Fixed ClassLoader leak of ActivityCorrelator ThreadLocal microsoft/mssql-jdbc#2366 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #47569 from wayneguow/ms_12_8. Authored-by: Wei Guo <guow93@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
…QLServer docker image tag to `2022-CU14-ubuntu-22.04` ### What changes were proposed in this pull request? This PR aims to upgrade `mssql-jdbc` to 12.8.0.jre11 and MySQLServer docker image to `mcr.microsoft.com/mssql/server:2022-CU14-ubuntu-22.04`. ### Why are the changes needed? This is the latest stable version of `mssql-jdbc`, related release notes: https://github.com/microsoft/mssql-jdbc/releases/tag/v12.7.0 https://github.com/microsoft/mssql-jdbc/releases/tag/v12.7.1 https://github.com/microsoft/mssql-jdbc/releases/tag/v12.8.0 Some fixed issues: - Fix to ensure metadata returned follows JDBC data type specs microsoft/mssql-jdbc#2326 - Added token cache map to fix use of unintended auth token for subsequent connections microsoft/mssql-jdbc#2341 - Clear prepared statement handle before reconnect microsoft/mssql-jdbc#2364 - Reset socketTimeout to original value after a successful connection open microsoft/mssql-jdbc#2355 - Clear prepared statement cache when resetting statement pool connection microsoft/mssql-jdbc#2361 - Fixed ClassLoader leak of ActivityCorrelator ThreadLocal microsoft/mssql-jdbc#2366 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47569 from wayneguow/ms_12_8. Authored-by: Wei Guo <guow93@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
ResultSetMetadata of the ResultSet from DatabaseMetadata.getColumns() is Incorrect for Some Columns (1751)
Fix ensures the correct data types for each metadata column by mapping the data type from the SQL Server to the type specified in the JDBC spec