You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fesod.apache.org by "GOODBOY008 (via GitHub)" <gi...@apache.org> on 2026/10/20 05:56:36 UTC
[PR] feat: consolidate default date formats behind thread-safe tiered config [fesod]
GOODBOY008 opened a new pull request, #1243:
URL: https://github.com/apache/fesod/pull/1243
## Purpose
The default date patterns were three undocumented public static mutable fields on `DateUtils` (`defaultDateFormat`, `defaultLocalDateFormat`, `defaultYearFormat`): no setter, no validation, no happens-before between a writer thread and reader threads, and no documentation — the only way to change them was a direct field write. This PR consolidates them behind a documented, thread-safe three-tier cascade:
| Priority | Source | Scope |
| --- | --- | --- |
| 1 | `@DateTimeFormat("...")` | per field (unchanged) |
| 2 | `.defaultDateFormat(...)` / `.defaultLocalDateFormat(...)` / `.defaultYearFormat(...)` builder parameters | per read/write call (new) |
| 3 | `DateUtils.setDefaultDateFormat(...)` etc. process-wide setters | process-wide (new, volatile) |
| — | built-in constants | `yyyy-MM-dd HH:mm:ss` / `yyyy-MM-dd` / `yyyy` |
Each read/write context snapshots tier 3 once at creation (`AbstractHolder`, workbook level; sheets inherit), so a single workbook always renders with one consistent pattern and a mid-run global change only affects contexts created afterwards.
## Background
- `defaultYearFormat` was added in #1087 (merged after 2.1.0, never shipped in a release), deliberately following the sibling-field pattern — this PR stops the pattern from replicating. Since it is unreleased, it is replaced outright by a private volatile field plus accessors, without a deprecation window.
- `defaultDateFormat`/`defaultLocalDateFormat` have shipped since 2.0.0, so they stay binary/source compatible: `volatile` + `@Deprecated`, with setter accessors following the `UrlImageConverter.fetchPolicy` precedent. Removal targeted at the next major version.
- #1020's review (ZonedDateTime STRING path keeping ISO) shows some types need type-specific defaults, so the `resolve*Format` helpers are opt-in infrastructure, not a mandate — documented in the new "Default date formats" section as guidance for the #1017 converter pipeline.
## Behavior audit
| Scenario | Before | After |
| --- | --- | --- |
| Direct `DateUtils.defaultDateFormat = x` field write | works, but the write may never be visible to other threads and can split an in-flight workbook | still works (field kept, now `volatile`); in-flight workbooks keep their snapshot |
| `DateUtils.format(date, null)` direct util call | global default | global default (unchanged) |
| Only the process-wide default is set (no annotations/params) | global value applied per cell | same value (snapshotted at context creation) — output identical |
| Empty `@DateTimeFormat` on Date/LocalDateTime/LocalDate write | empty string written into the cell style verbatim (only OffsetDateTime normalized it — pre-existing inconsistency, same direction as the #1020 review) | falls through to the resolved default |
| Read-side string date parsing | length-based auto-detection (unaffected by the global) | unchanged |
| `Year` string read / empty-`@DateTimeFormat` `String` field reading a numeric date cell | process-wide default | resolved cascade (annotation > per-call > snapshot) |
| Performance | static field read per conversion | instance field read per conversion — equivalent, no benchmark needed |
## Coordination
- #1217 (open, `YearNumberConverter`): no overlap — it owns the `(Year, NUMBER)` key and serial semantics, this PR only rewires `YearStringConverter`.
- #1020 (open, ZonedDateTime): on rebase its `fillDataFormat(cellData, format, DateUtils.defaultDateFormat)` fallback should switch to `DateUtils.resolveDateTimeFormat(contentProperty, globalConfiguration)`; the deprecated field keeps it compiling either way.
## Documentation
New "Default date formats" section in `website/docs/sheet/write/format.md` (+ zh-cn mirror): cascade table, per-call and process-wide usage, snapshot semantics, migration note for the deprecated fields, contributor guidance. Short read-side note in `read/format.md` and three new rows in `help/parameter.md` (+ zh-cn mirrors).
## Testing
- `DateUtilsDefaultFormatTest` (10 unit tests): full precedence matrix for the three pattern kinds, blank-pattern rejection, reset, workbook-level snapshot + sheet inheritance, param-over-global.
- `DefaultDateFormatWriteTest` (4 tests): xlsx write via builder params and via global setters (style `dataFormatString` assertions through `ExcelAssertions`), mid-write global flip does not affect the running workbook, CSV write uses the per-call default.
- Two existing tests updated from direct field reads to accessors.
- Full suite: 1737 tests, 0 failures; `spotless:check` and `javadoc:javadoc` clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: consolidate default date formats behind thread-safe tiered config [fesod]
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #1243:
URL: https://github.com/apache/fesod/pull/1243#discussion_r4355804366
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/AbstractHolder.java:
##########
@@ -125,6 +126,35 @@ public AbstractHolder(BasicParameter basicParameter, AbstractHolder prentAbstrac
} else {
globalConfiguration.setFiledCacheLocation(basicParameter.getFiledCacheLocation());
}
+
+ // Date patterns resolve eagerly: a workbook snapshots the process-wide default once so every cell in it uses
+ // one consistent value, while sheet/table holders inherit the workbook's snapshot.
+ if (basicParameter.getDefaultDateFormat() == null) {
Review Comment:
LGTM. Thanks for the update, @GOODBOY008. I pulled the latest head and reran the focused tests (16/16 passed, including the XLSX write/reopen path). The holder-level blank handling and the added empty/whitespace coverage resolve my concerns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: consolidate default date formats behind thread-safe tiered config [fesod]
Posted by "GOODBOY008 (via GitHub)" <gi...@apache.org>.
GOODBOY008 commented on code in PR #1243:
URL: https://github.com/apache/fesod/pull/1243#discussion_r4344300688
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/AbstractHolder.java:
##########
@@ -125,6 +126,35 @@ public AbstractHolder(BasicParameter basicParameter, AbstractHolder prentAbstrac
} else {
globalConfiguration.setFiledCacheLocation(basicParameter.getFiledCacheLocation());
}
+
+ // Date patterns resolve eagerly: a workbook snapshots the process-wide default once so every cell in it uses
+ // one consistent value, while sheet/table holders inherit the workbook's snapshot.
+ if (basicParameter.getDefaultDateFormat() == null) {
Review Comment:
Thanks @skytin1004 review this pr.
Confirmed — real bug, not just an edge case. `AbstractHolder:132` only checks `null`, so `""` is stored as the snapshot; `resolveDateTimeFormat:573` then treats it as unset and live-reads the static field, which is exactly the invariant this PR claims to protect (and what `write/format.md` promises).
Fixed at the holder boundary with `StringUtils.isBlank(...)` for all three patterns, plus blank rejection in the three builder methods so it fails at the call site instead of silently. Added `""` / `" "` cases — the existing `settersRejectBlankPatterns` only covered the static setters, which is why this slipped through.
Scope note: whether these fields were ever meant to be supported API is unproven either way — no docs, no tests, no setter, no assignment anywhere — and #1097 on the same area is still open with `+0` on every option. That is why this stays a deprecation rather than a removal, and why the cascade shape should be settled in a separate discussion rather than here. The blank fix lands regardless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: consolidate default date formats behind thread-safe tiered config [fesod]
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #1243:
URL: https://github.com/apache/fesod/pull/1243#discussion_r4355804366
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/AbstractHolder.java:
##########
@@ -125,6 +126,35 @@ public AbstractHolder(BasicParameter basicParameter, AbstractHolder prentAbstrac
} else {
globalConfiguration.setFiledCacheLocation(basicParameter.getFiledCacheLocation());
}
+
+ // Date patterns resolve eagerly: a workbook snapshots the process-wide default once so every cell in it uses
+ // one consistent value, while sheet/table holders inherit the workbook's snapshot.
+ if (basicParameter.getDefaultDateFormat() == null) {
Review Comment:
LGTM. Thanks for the update, @GOODBOY008. I pulled the latest head and reran the tests. The holder-level blank handling and the added empty/whitespace coverage resolve my concerns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: consolidate default date formats behind thread-safe tiered config [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on PR #1243:
URL: https://github.com/apache/fesod/pull/1243#issuecomment-6340452457
Currently, exposing methods like `.defaultDateFormat()`, `.defaultLocalDateFormat()`, and `.defaultYearFormat()` directly on the facade builder might lead to API bloat once we add support for other types like `LocalDateTime`, `LocalTime`, `Instant`, etc.
Could we consider consolidating these into a unified configuration builder instead? For example:
```Java
.dateTimeFormat((DateTimeFormatBuilder) fmt -> fmt
.pattern(Date.class, "yyyy/MM/dd HH:mm:ss")
.pattern(LocalDate.class, "yyyy/MM/dd")
)
```
Under the hood, this could be managed via `BasicParameter/GlobalConfiguration -> DateTimeFormatRegistry`. To use it, simply resolve the pattern via `registry.findPattern(Date.class)`.
This would be much more extensible for future date/time types. What do you think about this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: consolidate default date formats behind thread-safe tiered config [fesod]
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #1243:
URL: https://github.com/apache/fesod/pull/1243#discussion_r4322573799
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/AbstractHolder.java:
##########
@@ -125,6 +126,35 @@ public AbstractHolder(BasicParameter basicParameter, AbstractHolder prentAbstrac
} else {
globalConfiguration.setFiledCacheLocation(basicParameter.getFiledCacheLocation());
}
+
+ // Date patterns resolve eagerly: a workbook snapshots the process-wide default once so every cell in it uses
+ // one consistent value, while sheet/table holders inherit the workbook's snapshot.
+ if (basicParameter.getDefaultDateFormat() == null) {
Review Comment:
I was checking what happens when the per-call format is empty, for example `.defaultDateFormat("")`. It gets stored here because only `null` falls back to the snapshot, but the resolver later treats the empty value as missing and reads the current global default again. That means the workbook does not really keep its original snapshot if the global value changes afterwards.
I think blank values should either be rejected by all three builders or normalized to the parent/global snapshot when the holder is created. I'm leaning toward handling it here because that would also cover metadata created without the builder. Could we fix it in that direction and add a small empty/whitespace case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org