fix: Handle java.sql date types compatibility in Excel read and write operations#719
fix: Handle java.sql date types compatibility in Excel read and write operations#719ongdisheng wants to merge 9 commits intoapache:mainfrom
java.sql date types compatibility in Excel read and write operations#719Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where java.sql.Date instances would throw an UnsupportedOperationException when passed to the WriteCellData constructor. The fix changes the date conversion from using toInstant() (which is not supported by java.sql.Date) to using getTime() (which works for both java.util.Date and java.sql.Date).
- Changed the
WriteCellDataDate constructor to useInstant.ofEpochMilli(dateValue.getTime())instead ofdateValue.toInstant() - Added comprehensive test coverage for
java.sql.Datecompatibility - Updated date format in test data classes from Chinese format to standard ISO format for consistency
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
fesod/src/main/java/org/apache/fesod/sheet/metadata/data/WriteCellData.java |
Fixed the Date constructor to handle both java.util.Date and java.sql.Date by using getTime() method instead of toInstant() |
fesod/src/test/java/org/apache/fesod/sheet/celldata/CellDataWriteData.java |
Added sqlDate field and updated date format to ISO format for testing |
fesod/src/test/java/org/apache/fesod/sheet/celldata/CellDataReadData.java |
Added corresponding sqlDate field for reading test data |
fesod/src/test/java/org/apache/fesod/sheet/celldata/CellDataDataTest.java |
Added test case that creates WriteCellData with java.sql.Date to verify the fix |
fesod/src/test/java/org/apache/fesod/sheet/celldata/CellDataDataListener.java |
Added assertion to verify java.sql.Date handling works correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setType(CellDataTypeEnum.DATE); | ||
| this.dateValue = LocalDateTime.ofInstant(dateValue.toInstant(), ZoneId.systemDefault()); | ||
| // Use getTime() which works for both java.util.Date and java.sql.Date | ||
| this.dateValue = LocalDateTime.ofInstant(Instant.ofEpochMilli(dateValue.getTime()), ZoneId.systemDefault()); |
There was a problem hiding this comment.
Using the Date#getTime() incurs a loss of precision, as it only provides millisecond accuracy. Furthermore, might we consider supporting java.sql.Timestamp? I believe the unit test examples could be improved.
There was a problem hiding this comment.
Thank you for the feedback! I'll look into this and explore supporting java.sql.Timestamp as well. Will improve the test examples too.
java.sql.Date compatibility in WriteCellDatajava.sql date types compatibility in Excel read and write operations
…heng/fesod into fix/handle-sql-date-compatibility
|
Hi @ongdisheng, @delei I was looking through #719 and tried reproducing the original "WriteCellData(Date)" issue locally. It feels like the PR has grown a bit beyond the original "java.sql.Date" / "java.sql.Time" "toInstant()" problem. Do you think it would be easier to review this in smaller pieces first starting with the constructor behavior and a few focused regression tests? |
Since so many conflicts, I will close this PR in 24hours. Welcome any contributions :) |
Closed: #714
Purpose of the pull request
This PR fixes a bug where using
java.sql.DatewithWriteCellDataconstructor throws anUnsupportedOperationException. The issue occurs because the original implementation callstoInstant()method which is not supported byjava.sql.Date.What's changed?
The
WriteCellDataDate constructor now usesInstant.ofEpochMilli(dateValue.getTime())instead ofdateValue.toInstant()as thegetTime()method works for bothjava.util.Date and java.sql.Date.Checklist