feat: add converters for java.sql.Date, java.sql.Time, java.time.Instant and java.time.YearMonth - #1133
feat: add converters for java.sql.Date, java.sql.Time, java.time.Instant and java.time.YearMonth#1133CodeMan-cmd wants to merge 3 commits into
Conversation
A java.sql.Date field has no converter today, so it fails on read and on write with 'Can not find Converter support class Date' even though java.util.Date is covered: the converter map is keyed by the exact class and java.sql.Date only extends it. Add the usual family (DATE, NUMBER, STRING) following converters/date and converters/localdate, with yyyy-MM-dd as the default format, and register it in DefaultConverterLoader.
java.sql.Time and java.time.Instant have no converter, so a field of either type fails on read and on write with 'Can not find Converter support class'. Add the usual family (DATE, NUMBER, STRING) for both, following converters/localtime for the sql time and converters/date for the instant, and register them in DefaultConverterLoader. Excel has no time zone, so Instant goes through the default zone of the JVM, like java.util.Date.
Excel has no month type, so a month is written as a date cell of the first day of that month with a yyyy-MM number format, which keeps sorting and date arithmetic in the sheet working. The string family reads and writes yyyy-MM (or the value of @DateTimeFormat) directly.
skytin1004
left a comment
There was a problem hiding this comment.
Thanks for working on these converters. I ran the full test suite locally with Java 11, and everything passed. I did notice two edge cases that do not seem to be covered yet.
YearMonth does not fall back to the default yyyy-MM format when DateTimeFormat is present but empty. In that case, reading 2020-01 fails, and writing can produce an empty value.
The new string converters also appear to ignore the locale configured in Fesod. For example, with French configured, a value such as 01 janvier 2020 cannot be parsed because the system locale is used instead.
Could you take a look at these cases and add tests for them?
| } | ||
| return YearMonth.parse( | ||
| stringValue, | ||
| DateTimeFormatter.ofPattern( |
There was a problem hiding this comment.
Could we treat an empty format the same as a missing one here? DateTimeFormat defaults to an empty value, so reading 2020-01 currently uses an empty pattern and fails. The write path below has the same problem.
| if (contentProperty == null || contentProperty.getDateTimeFormatProperty() == null) { | ||
| return Date.valueOf(DateUtils.parseLocalDate(dateString, null, null)); | ||
| } | ||
| return Date.valueOf(DateUtils.parseLocalDate( |
There was a problem hiding this comment.
Could we use globalConfiguration.getLocale() here and in the other new string converters? With French configured, 01 janvier 2020 currently fails because the system locale is used instead.
Purpose of the pull request
Related: #1017
Four types from the task in #1017 that are not on the list yet. I checked the current behaviour on main with a bean per type and all of them fail on read and on write:
java.sql.Dateis worth calling out separately: it extendsjava.util.Date, which does have converters, but the converter map is keyed by the exact class, so ajava.sql.Datefield gets nothing.What's changed?
The usual family (DATE, NUMBER, STRING) for each of the four types, following the shape of the families that are already here, with unit tests per family. All of them are registered in
DefaultConverterLoader.converters/sqldatefollowsconverters/date, usingtoLocalDate()as the intermediate andyyyy-MM-ddas the default format.converters/sqltimefollowsconverters/localtime, usingtoLocalTime()andHH:mm:ss.converters/instantgoes throughjava.util.Date/LocalDateTime, souse1904windowingkeeps working. Excel has no time zone, so the value is read and written with the JVM default zone, the same wayjava.util.Dateis handled.converters/yearmonthwrites a month as a date cell of the first day of that month with ayyyy-MMnumber format, which keeps sorting and date arithmetic in the sheet working; the string family reads and writesyyyy-MM.Tests:
SqlDateDataTest,SqlTimeDataTest,InstantDataTest,YearMonthDataTest, each one a round trip over xlsx, xls and csv, with a plain field, a string field and a@DateTimeFormatfield.I kept the four families in one PR because they all register in
DefaultConverterLoaderand separate PRs would conflict there. Happy to split it into smaller PRs if you prefer.Local verification with Temurin 1.8.0_472:
./mvnw clean package -B -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet-> 928 tests, 0 failures./mvnw spotless:check-> Build successChecklist