Skip to content

feat: add converters for java.sql.Date, java.sql.Time, java.time.Instant and java.time.YearMonth - #1133

Open
CodeMan-cmd wants to merge 3 commits into
apache:mainfrom
CodeMan-cmd:feat-sql-date-converters
Open

CodeMan-cmd wants to merge 3 commits into
apache:mainfrom
CodeMan-cmd:feat-sql-date-converters

Conversation

@CodeMan-cmd

Copy link
Copy Markdown

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:

write/read java.sql.Date -> ExcelWriteDataConvertException: Can not find 'Converter' support class Date.
write/read java.sql.Time -> ... support class Time.
write/read Instant       -> ... support class Instant.
write/read YearMonth     -> ... support class YearMonth.

java.sql.Date is worth calling out separately: it extends java.util.Date, which does have converters, but the converter map is keyed by the exact class, so a java.sql.Date field 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/sqldate follows converters/date, using toLocalDate() as the intermediate and yyyy-MM-dd as the default format.
  • converters/sqltime follows converters/localtime, using toLocalTime() and HH:mm:ss.
  • converters/instant goes through java.util.Date / LocalDateTime, so use1904windowing keeps working. Excel has no time zone, so the value is read and written with the JVM default zone, the same way java.util.Date is handled.
  • converters/yearmonth writes a month 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.

Tests: SqlDateDataTest, SqlTimeDataTest, InstantDataTest, YearMonthDataTest, each one a round trip over xlsx, xls and csv, with a plain field, a string field and a @DateTimeFormat field.

I kept the four families in one PR because they all register in DefaultConverterLoader and 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 success

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

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 skytin1004 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants