fix bug for TypeUtils.castToTimestamp, for issue #3906 and #3907 #3908
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes bugs in TypeUtils.castToTimestamp and TypeUtils.castToSqlDate methods (issues #3906 and #3907) by adding proper handling for converting java.util.Date objects to SQL date/time types and aligning the API signatures with version 1.2.83.
Key changes:
- Added conversion logic in core TypeUtils to handle
java.util.Dateto SQL types (Timestamp,java.sql.Date,java.sql.Time) - Changed return types of
castToTimestampandcastToSqlDatefrom specific types toObjectin fastjson1-compatible module - Added test case for
castToTimestampwithjava.util.Dateinput
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
fastjson1-compatible/src/test/java/com/alibaba/fastjson2/Issue3906.java |
New test verifying that castToTimestamp doesn't throw an exception when converting java.util.Date |
fastjson1-compatible/src/main/java/com/alibaba/fastjson/util/TypeUtils.java |
Changed return types of castToTimestamp and castToSqlDate from specific types to Object to match 1.2.83 API |
core/src/main/java/com/alibaba/fastjson2/util/TypeUtils.java |
Added conversion logic to handle java.util.Date to SQL date/time types in the cast method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| public static java.sql.Date castToSqlDate(final Object value) { | ||
| public static Object castToSqlDate(final Object value) { |
There was a problem hiding this comment.
Changing the return type from java.sql.Date to Object is a breaking API change that will cause compilation errors for existing code. While this may match version 1.2.83's signature, it breaks type safety and forces callers to cast the result. Consider keeping the return type as java.sql.Date to maintain backward compatibility, or document this as a breaking change in the release notes.
| public static Object castToSqlDate(final Object value) { | |
| public static java.sql.Date castToSqlDate(final Object value) { |
|
|
||
| public class Issue3906 { | ||
| @Test | ||
| public void test() throws NoSuchMethodException { |
There was a problem hiding this comment.
The method signature declares throws NoSuchMethodException but this exception is never thrown by the test logic. The assertDoesNotThrow lambda wraps the call to TypeUtils.castToTimestamp, which doesn't throw this checked exception. Remove the unnecessary throws NoSuchMethodException declaration.
| public void test() throws NoSuchMethodException { | |
| public void test() { |
| @Test | ||
| public void test() throws NoSuchMethodException { | ||
| assertDoesNotThrow(() -> TypeUtils.castToTimestamp(new java.util.Date())); | ||
| } |
There was a problem hiding this comment.
The test has two issues:
- It only verifies that no exception is thrown, but doesn't verify that the conversion actually produces a correct
Timestampobject. Consider adding assertions to verify the result is not null, is an instance ofjava.sql.Timestamp, and has the same time value as the input. - The PR description mentions fixing both issue [BUG]fastjson兼容包报错，2.0.60版本，使用TypeUtils转换java.util.Date为Timestamp报错，1.2.83版本则是正常的 #3906 and [BUG]二方包里使用com.alibaba.fastjson.util.TypeUtils#castToTimestamp时，升级fastjson版本不兼容 #3907, but only includes a test for
castToTimestamp. Add a similar test case forcastToSqlDateto ensure the fix works for both methods.
| } | ||
|
|
||
| public static Timestamp castToTimestamp(final Object value) { | ||
| public static Object castToTimestamp(final Object value) { |
There was a problem hiding this comment.
Changing the return type from Timestamp to Object is a breaking API change that will cause compilation errors for existing code. While this may match version 1.2.83's signature, it breaks type safety and forces callers to cast the result. Consider keeping the return type as Timestamp to maintain backward compatibility, or document this as a breaking change in the release notes.
| public static Object castToTimestamp(final Object value) { | |
| public static Timestamp castToTimestamp(final Object value) { |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3908 +/- ##
============================================
- Coverage 75.04% 75.04% -0.01%
- Complexity 23166 23240 +74
============================================
Files 653 659 +6
Lines 87194 87407 +213
Branches 20233 20307 +74
============================================
+ Hits 65433 65592 +159
- Misses 13354 13372 +18
- Partials 8407 8443 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| if (obj instanceof Date) { | ||
| long time = ((Date) obj).getTime(); | ||
| if (targetClass == java.sql.Timestamp.class) { |
There was a problem hiding this comment.
会有些环境没有java.sql.Timestamp这个类，要用className代替
| long time = ((Date) obj).getTime(); | ||
| String className = targetClass.getName(); | ||
|
|
||
| if ("java.sql.Timestamp".equals(className) || |
There was a problem hiding this comment.
这样可能性能比较差，可以考虑在JdbcSupport中使用static Constructor缓存来加速性能
3 participants
What this PR does / why we need it?
检查了下 1.2.83 的 TypeUtils，这两个都返回 Object，所以都做了修改：
