feat: add java.time.OffsetDateTime converters (#1017) by Mikkey-f · Pull Request #1033 · apache/fesod

Mikkey-f · 2026-08-23T06:16:14Z

What and why

Adds a java.time.OffsetDateTime converter family for the OffsetDateTime slice of #1017, following the existing LocalDateTime / ZonedDateTime pattern, so OffsetDateTime fields map to Excel natively instead of falling back to String:

OffsetDateTimeDateConverter — write-only, emits an Excel DATE cell via toLocalDateTime(), default format yyyy-MM-dd HH:mm:ss
OffsetDateTimeNumberConverter — bidirectional NUMBER serial, respects use1904windowing (property-level first, then a null-safe global default); on read attaches ZoneId.systemDefault() to the parsed LocalDateTime
OffsetDateTimeStringConverter — bidirectional STRING, honors @DateTimeFormat and the configured Locale, defaults to ISO_OFFSET_DATE_TIME when no format is set; reads parse strictly and reject offset-less text (fail fast instead of a silent ZoneId.systemDefault() interpretation)

Registered in DefaultConverterLoader.initAllConverter() / initDefaultWriteConverter().

Tests

OffsetDateTimeConverterTest covers converter keys, DATE/NUMBER/STRING read & write, @DateTimeFormat formatting, use1904windowing (including the null-safe global default) and round-trip behavior. All tests pass, spotless:check is green, and the full local build was verified.

Related: #1017 (OffsetDateTime slice).

Copilot

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.

Mikkey-f · 2026-08-23T11:06:45Z

+        if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) {
+            format = contentProperty.getDateTimeFormatProperty().getFormat();
+        }
+        WorkBookUtil.fillDataFormat(cellData, format, DateUtils.defaultDateFormat);


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).

Mikkey-f · 2026-08-23T11:07:46Z

+        if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) {
+            Boolean propertyUse1904windowing =
+                    contentProperty.getDateTimeFormatProperty().getUse1904windowing();
+            if (propertyUse1904windowing != null) {
+                return propertyUse1904windowing;


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.

Mikkey-f · 2026-08-23T11:07:04Z

+        String format = format(contentProperty);
+        if (StringUtils.isEmpty(format)) {
+            return DateTimeFormatter.ISO_OFFSET_DATE_TIME;
+        }
+        return DateTimeFormatter.ofPattern(format, locale);


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).

bengbengbalabalabeng · 2026-08-23T13:32:44Z

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

bengbengbalabalabeng · 2026-09-07T12:26:22Z

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

Please update the license header.

Mikkey-f · 2026-09-07T14:07:56Z

Thanks for the review. You're right — these files were implemented from scratch, so I've removed the EasyExcel-derived header block (including the Alibaba copyright notice) from the three new converters and the test file. They now carry only the standard ASF header, matching the convention in FesodSheet.java and the LocalTime converters merged in #1032.

bengbengbalabalabeng

LGTM.

nkuprins · 2026-09-11T21:47:55Z

The tests don't seem to pin the locale? If you swap it for a different locale, nothing fails. Maybe use something locale-sensitive like "dd MMMM yyyy HH:mm:ss XXX"?

nkuprins · 2026-09-11T22:02:07Z

2 new methods were added to DateUtils by your last refactoring. However, no new tests for them were added to DateUtilsTest? Maybe we can add something like this to DateUtilsTest to keep it consistent:

@Test
void test_parseOffsetDateTime() {
    OffsetDateTime expected = OffsetDateTime.of(2020, 1, 2, 3, 4, 5, 0, ZoneOffset.ofHours(8));
    Assertions.assertEquals(expected, DateUtils.parseOffsetDateTime("2020-01-02T03:04:05+08:00", null, Locale.US));
    Assertions.assertEquals(expected, DateUtils.parseOffsetDateTime("2020-01-02T03:04:05+08:00", "", Locale.US));
    Assertions.assertEquals(
            expected,
            DateUtils.parseOffsetDateTime("02 Januar 2020 03:04:05 +08:00", "dd MMMM yyyy HH:mm:ss XXX", Locale.GERMAN));
    Assertions.assertThrows(
            DateTimeParseException.class,
            () -> DateUtils.parseOffsetDateTime("2020-01-02T03:04:05", null, Locale.US));
}

@Test
void test_format_OffsetDateTime() {
    OffsetDateTime value = OffsetDateTime.of(2020, 1, 2, 3, 4, 5, 0, ZoneOffset.ofHours(8));
    Assertions.assertNull(DateUtils.format((OffsetDateTime) null, null, Locale.US));
    Assertions.assertEquals("2020-01-02T03:04:05+08:00", DateUtils.format(value, null, Locale.US));
    Assertions.assertEquals("2020-01-02T03:04:05+08:00", DateUtils.format(value, "", Locale.US));
    Assertions.assertEquals(
            "02 Januar 2020 03:04:05 +08:00", DateUtils.format(value, "dd MMMM yyyy HH:mm:ss XXX", Locale.GERMAN));
}

nkuprins · 2026-09-11T22:16:11Z

Sibling test classes to OffsetDateTimeConverterTest seem to have:

@AfterEach
void tearDown() {
    DateUtils.removeThreadLocalCache();
}

Should we also add it for consistency?

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.

…erters (apache#1017)

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.

…converters (apache#1017)

…nverter (apache#1017)

…s in OffsetDateTimeConverterTest (apache#1017)

…ers (apache#1017) Addresses second-round review comments on OffsetDateTimeStringConverter. DateUtils now provides parseOffsetDateTime(String, String, Locale) and a format(OffsetDateTime, String, Locale) overload. Both reuse the bounded DATE_TIME_FORMATTER_THREAD_LOCAL cache, which is cleared by removeThreadLocalCache() at the end of each read/write context, instead of a converter-local ThreadLocal that has no cleanup hook. An empty format falls back to ISO_OFFSET_DATE_TIME. Offset-less STRING reads now fail fast with DateTimeParseException instead of silently re-interpreting the text in ZoneId.systemDefault(), which mapped the same text to different instants depending on the server timezone. Tests: the two ZoneId.systemDefault fallback cases now assert failure, the configured-pattern rejection test uses an offset-bearing pattern, and a round-trip case for a configured offset pattern was added.

…ache#1017)

Mikkey-f · 2026-09-12T06:33:53Z

Thanks for the review — all three are addressed in 7653c54 (test-only, no production code changed):

Added stringConverterHonoursConfiguredLocale, which pins Locale.GERMAN with a locale-sensitive pattern (dd MMMM yyyy HH:mm:ss XXX) and asserts both the written text and the round-trip read, so the locale is now actually exercised and independent of the machine default.
Added test_parseOffsetDateTime and test_format_OffsetDateTime to DateUtilsTest, following the cases you outlined.
Added the @AfterEach tearDown() calling DateUtils.removeThreadLocalCache(), matching the sibling converter tests.

spotless and the full test suite (932 tests) are green.

Mikkey-f · 2026-09-12T06:35:13Z

@bengbengbalabalabeng a quick heads-up on the latest push: 7653c54 only adds tests, covering the three points nkuprins raised — locale-sensitive coverage for the string converter, direct tests for the two new DateUtils helpers, and the @AfterEach thread-local cache cleanup to match the sibling tests. No production code changed; the converters you approved are untouched.

The new commit auto-dismissed your approval, so it needs a fresh approve before it can be merged. Sorry for the extra round trip.

bengbengbalabalabeng

LGTM.

Mikkey-f force-pushed the feat/offsetdatetime-converters branch from 347d5b5 to b9122be Compare August 23, 2026 06:26

delei added the PR: first-time contributor first-time contributor label Aug 23, 2026

delei requested a lite review from Copilot August 23, 2026 10:18

Copilot started reviewing on behalf of delei August 23, 2026 10:19 View session

Copilot AI reviewed Aug 23, 2026

View reviewed changes

Mikkey-f force-pushed the feat/offsetdatetime-converters branch from 3d00856 to 7dd544e Compare August 25, 2026 10:13

Mikkey-f mentioned this pull request Aug 28, 2026

fix: fall back to global 1904 windowing when @DateTimeFormat default is used (#1042) #1054

Merged

bengbengbalabalabeng reviewed Sep 7, 2026

View reviewed changes

Comment thread fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/OffsetDateTimeConverterTest.java Outdated

Mikkey-f force-pushed the feat/offsetdatetime-converters branch from 4fb6929 to 255c63a Compare September 7, 2026 14:19

bengbengbalabalabeng reviewed Sep 8, 2026

View reviewed changes

Comment thread ...ain/java/org/apache/fesod/sheet/converters/offsetdatetime/OffsetDateTimeStringConverter.java Outdated

bengbengbalabalabeng reviewed Sep 8, 2026

View reviewed changes

Comment thread ...ain/java/org/apache/fesod/sheet/converters/offsetdatetime/OffsetDateTimeStringConverter.java Outdated

Mikkey-f force-pushed the feat/offsetdatetime-converters branch from e5d642a to 3336675 Compare September 8, 2026 16:27

bengbengbalabalabeng previously approved these changes Sep 10, 2026

View reviewed changes

Mikkey-f added 9 commits September 12, 2026 14:23

fix: null-safe global use1904windowing default in OffsetDateTime conv…

8d15f67

…erters (apache#1017)

fix: remove EasyExcel-derived license header from new OffsetDateTime …

570faf7

…converters (apache#1017)

refactor: reuse shared DateUtils.isDate1904 in OffsetDateTimeNumberCo…

dc32b0b

…nverter (apache#1017)

refactor: replace static Assertions imports with fully-qualified call…

8ad7959

…s in OffsetDateTimeConverterTest (apache#1017)

test: cover locale handling and DateUtils helper tests per review (ap…

7653c54

…ache#1017)

Mikkey-f dismissed bengbengbalabalabeng’s stale review via 7653c54 September 12, 2026 06:28

Mikkey-f force-pushed the feat/offsetdatetime-converters branch from 3336675 to 7653c54 Compare September 12, 2026 06:28

Mikkey-f mentioned this pull request Sep 12, 2026

[Discussion] Where should isDate1904 and getDateTimeFormat reside? (DateUtils vs. AbstractDateTimeConverter) #1097

Open

bengbengbalabalabeng approved these changes Sep 13, 2026

View reviewed changes

Uh oh!

Conversation

Mikkey-f commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What and why

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Mikkey-f Aug 23, 2026

Choose a reason for hiding this comment

Uh oh!

Mikkey-f Aug 23, 2026

Choose a reason for hiding this comment

Uh oh!

Mikkey-f Aug 23, 2026

Choose a reason for hiding this comment

Uh oh!

bengbengbalabalabeng commented Aug 23, 2026

Uh oh!

Uh oh!

bengbengbalabalabeng commented Sep 7, 2026

Uh oh!

Mikkey-f commented Sep 7, 2026

Uh oh!

Uh oh!

Uh oh!

bengbengbalabalabeng left a comment

Choose a reason for hiding this comment

Uh oh!

nkuprins commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkuprins commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkuprins commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mikkey-f commented Sep 12, 2026

Uh oh!

Mikkey-f commented Sep 12, 2026

Uh oh!

bengbengbalabalabeng left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Mikkey-f commented Aug 23, 2026 •

edited

Loading

nkuprins commented Sep 11, 2026 •

edited

Loading

nkuprins commented Sep 11, 2026 •

edited

Loading

nkuprins commented Sep 11, 2026 •

edited

Loading