Skip to content

feat: support Instant string conversion - #1089

Open
56wj wants to merge 3 commits into
apache:mainfrom
56wj:feat/instant-string-converter
Open

feat: support Instant string conversion#1089
56wj wants to merge 3 commits into
apache:mainfrom
56wj:feat/instant-string-converter

Conversation

@56wj

@56wj 56wj commented Sep 9, 2026

Copy link
Copy Markdown

Purpose of the pull request

Related: #1017

Add JDK 8-compatible support for lossless conversion between java.time.Instant and UTC ISO-8601 Excel string cells.

What's changed?

  • Add InstantStringConverter using OffsetDateTime for UTC-only reads and Instant.toString() for canonical writes.
  • Register the converter for default writes, explicit string writes, and string reads.
  • Reject non-UTC offsets to keep read/write string semantics symmetric.
  • Add unit coverage for converter keys, UTC parsing, non-UTC rejection, nanosecond round trips, invalid input, and loader registration.
  • Keep the implementation string-only because Excel date/number cells do not preserve timezone/offset semantics.

Verification

  • mvn clean package -B -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet
  • mvn -pl fesod-sheet -DskipTests spotless:check

Results after syncing current main on JDK 21: 924 tests, 0 failures, 0 errors, 0 skipped. The generated Surefire reports include 7 passing InstantStringConverterTest cases and 5 passing DefaultConverterLoaderTest cases.

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.

@56wj
56wj force-pushed the feat/instant-string-converter branch from 9077a24 to 87ce5b2 Compare September 9, 2026 12:25
@56wj

56wj commented Sep 9, 2026

Copy link
Copy Markdown
Author

CI on JDK 8/11 exposed a runtime compatibility difference: Instant.parse(...) rejected the +08:00 test input there, although newer JDKs accepted it. I updated the read path to OffsetDateTime.parse(...).toInstant(), which preserves the intended ISO-8601 offset normalization and remains JDK 8 compatible. The focused converter/loader tests and Spotless check pass locally on the updated commit 87ce5b2.

@56wj
56wj force-pushed the feat/instant-string-converter branch from 87ce5b2 to 3b7ae8f Compare September 10, 2026 14:11
Comment on lines +46 to +50
@Override
public Instant convertToJavaData(
ReadCellData<?> cellData, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
return OffsetDateTime.parse(cellData.getStringValue()).toInstant();
}

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.

The current implementation of InstantStringConverter has a semantic inconsistency between read and write behavior.

  • convertToJavaData(...): uses OffsetDateTime.parse(...).toInstant(), which accepts any offset (e.g., +08:00) and normalizes the value to UTC.
  • convertToExcelData(): always outputs Instant.toString(), which is strictly UTC.

This means that an Excel value like "2026-09-13T12:12:12+08:00" will be read as "2026-09-13T04:12:12Z", and writing it back will produce a different string. The round‑trip behavior is not symmetric.

Since Instant represents an absolute UTC timestamp, the converter should enforce that the input string is already in UTC. Otherwise the converter silently changes the timestamp.

I suggest adding a validation step in convertToJavaData to reject non‑UTC offsets. This keeps the read/write semantics consistent and avoids unexpected timezone normalization.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated. The read path now parses the offset explicitly and rejects any non-zero offset with DateTimeParseException; Z and +00:00 remain valid UTC inputs, and writes stay canonical Z. I added a regression test for rejecting +08:00. The focused tests, Spotless, and the full package run all pass (924 tests) on commit f0af0eb.

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.

Suggest using getOffset().equals(ZoneOffset.UTC) ensures that the input is truly UTC (Z or +00:00) and improves readability.

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