feat: add java.time.OffsetDateTime converters (#1017) - #1033
Conversation
Add OffsetDateTimeStringConverter, OffsetDateTimeNumberConverter and OffsetDateTimeDateConverter, following the existing LocalDateTime and ZonedDateTime converter patterns: - String conversion preserves the offset in ISO-8601 text by default, with a configurable pattern, falling back to local wall-clock time when the offset is missing. - Number and date conversions drop the offset while preserving the local wall-clock time, consistent with the ZonedDateTime converters.
- Read fallback now routes through DateUtils.parseLocalDateTime so the default space-separated format written by other date converters is accepted, and text that does not match a configured pattern is rejected. - Return null instead of NPE for invalid Excel serials, matching the LocalDateTime family. - Null-safe use1904windowing resolution and default-locale fallback.
347d5b5 to
b9122be
Compare
There was a problem hiding this comment.
Pull request overview
Adds built-in OffsetDateTime conversion support for native date, numeric serial, and string Excel cells.
Changes:
- Adds DATE, NUMBER, and STRING converters.
- Registers converters in default loader maps.
- Adds unit tests for formatting, parsing, windowing, and registration.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/OffsetDateTimeConverterTest.java | Updated as part of this pull request. |
| fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/offsetdatetime/OffsetDateTimeStringConverter.java | Updated as part of this pull request. |
| fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/offsetdatetime/OffsetDateTimeNumberConverter.java | Updated as part of this pull request. |
| fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/offsetdatetime/OffsetDateTimeDateConverter.java | Updated as part of this pull request. |
| fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/DefaultConverterLoader.java | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) { | ||
| format = contentProperty.getDateTimeFormatProperty().getFormat(); | ||
| } | ||
| WorkBookUtil.fillDataFormat(cellData, format, DateUtils.defaultDateFormat); |
There was a problem hiding this comment.
Empty @DateTimeFormat format — Fixed in OffsetDateTimeDateConverter: empty formats are now normalized to null before calling WorkBookUtil.fillDataFormat, so the yyyy-MM-dd HH:mm:ss default is applied. Added a regression test(dateConverterFallsBackToDefaultFormatForEmptyDateTimeFormat).
| if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) { | ||
| Boolean propertyUse1904windowing = | ||
| contentProperty.getDateTimeFormatProperty().getUse1904windowing(); | ||
| if (propertyUse1904windowing != null) { | ||
| return propertyUse1904windowing; |
There was a problem hiding this comment.
use1904windowing DEFAULT being unboxed to false — Agreed this is a real issue, but it's a pre-existing framework-level problem: DateTimeFormatProperty.build converts BooleanEnum.DEFAULT (null) to false (DateTimeFormatProperty.java:55-57), and all existing number converters (Date, LocalDate, LocalDateTime, ZonedDateTime) consume the property value without a global fallback. This PR's null-safe fallback covers the no-annotation path; the annotated path behaves identically to the existing converter families. Fixing it properly means changing DateTimeFormatProperty (preserving DEFAULT as null) and updating every date-number converter — a framework-wide change that deserves its own issue/PR. Happy to open one if that's useful.
| String format = format(contentProperty); | ||
| if (StringUtils.isEmpty(format)) { | ||
| return DateTimeFormatter.ISO_OFFSET_DATE_TIME; | ||
| } | ||
| return DateTimeFormatter.ofPattern(format, locale); |
There was a problem hiding this comment.
Formatter caching — Fixed in OffsetDateTimeStringConverter: DateTimeFormatter instances are now cached per pattern and locale in a thread-local map, avoiding rebuilds in the per-cell hot path (the ISO default is a shared constant).
Address review comments: - OffsetDateTimeDateConverter: an empty @DateTimeFormat value bypassed WorkBookUtil.fillDataFormat's default format (only null falls back), writing an empty/General number format instead of yyyy-MM-dd HH:mm:ss. Normalize empty formats to null; regression test added. - OffsetDateTimeStringConverter: cache DateTimeFormatter instances per pattern and locale in a thread-local map instead of rebuilding them on every cell conversion in the hot path.
|
The newly added files in this PR are implemented from scratch and are not derived from Alibaba's EasyExcel. Therefore, no EasyExcel-related license header is required for these files. Please refer to: https://github.com/apache/fesod/blob/main/fesod-sheet/src/main/java/org/apache/fesod/sheet/FesodSheet.java |
What and why
Adds a
java.time.OffsetDateTimeconverter family for the OffsetDateTime slice of #1017, following the existingLocalDateTime/ZonedDateTimepattern, so OffsetDateTime fields map to Excel natively instead of falling back toString:OffsetDateTimeDateConverter— write-only, emits an ExcelDATEcell viatoLocalDateTime(), default formatyyyy-MM-dd HH:mm:ssOffsetDateTimeNumberConverter— bidirectionalNUMBERserial, respectsuse1904windowing(property-level first, then a null-safe global default); on read attachesZoneId.systemDefault()to the parsedLocalDateTimeOffsetDateTimeStringConverter— bidirectionalSTRING, honors@DateTimeFormatand the configuredLocale, defaults toISO_OFFSET_DATE_TIMEwhen no format is setRegistered in
DefaultConverterLoader.initAllConverter()/initDefaultWriteConverter().Tests
OffsetDateTimeConverterTestcovers converter keys, DATE/NUMBER/STRING read & write,@DateTimeFormatformatting,use1904windowing(including the null-safe global default) and round-trip behavior. All tests pass,spotless:checkis green, and the full local build was verified.Closes #1017 (OffsetDateTime slice).