Repository navigation
feat: consolidate default date formats behind thread-safe tiered config - #1243
GOODBOY008 wants to merge 4 commits into
Conversation
The default date patterns were three undocumented public static mutable fields on DateUtils with no setter, no validation and no happens-before between writer and reader threads, read at ~15 scattered call sites. Resolve them through a documented three-tier cascade instead: @DateTimeFormat > per-call builder parameter > process-wide setter > built-in constant - DateUtils: defaultDateFormat/defaultLocalDateFormat become volatile and deprecated in favor of get/set accessors (blank patterns are rejected, resetDefaultDateFormats() restores builtins); defaultYearFormat never shipped (added in apache#1087 after 2.1.0) and is replaced by a private volatile field plus accessors outright - resolveDateTimeFormat/resolveLocalDateFormat/resolveYearFormat give converters one shared annotation-over-configuration entry point (also normalizing empty @DateTimeFormat values, which Date/ LocalDateTime/LocalDate converters previously wrote into cell styles verbatim) - GlobalConfiguration carries the per-call patterns; AbstractHolder snapshots the process-wide default once per workbook so every cell of a workbook uses one consistent pattern and mid-run global changes only affect contexts created afterwards - builder parameters defaultDateFormat/defaultLocalDateFormat/ defaultYearFormat on both read and write sides - CsvWorkbook/CsvSheet thread the snapshot into CSV rendering - document the cascade (en + zh-cn) including a migration note and guidance for new converters (apache#1017 pipeline) Signed-off-by: gongzhongqiang <gongzhongqiang@apache.org>
- wrap the disclaimer paragraph of the ASF header in the two new test files exactly like tools/spotless/license-header.txt (hawkeye wanted to replace the headers) - repair the zh-cn parameter.md table rows mangled by the previous insertion: restore the use1904windowing row's trailing pipe (MD055) and drop the duplicated trailing pipe after defaultYearFormat (MD056) Signed-off-by: gongzhongqiang <gongzhongqiang@apache.org>
There was a problem hiding this comment.
Hi @GOODBOY008, I think the snapshot invariant should be enforced at the holder boundary, or blank values should be rejected consistently across all three builders. Could you please take a look?
|
|
||
| // 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) { |
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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.
A blank per-call pattern was stored into the context snapshot as-is, but
the resolvers read an empty pattern as "not set" and fell back to the
process-wide default. A workbook created with .defaultDateFormat("") was
therefore not really snapshotted: changing the global value afterwards
still changed its output, contradicting the snapshot guarantee that this
change documents and tests.
A whitespace-only pattern took a third path: StringUtils.isEmpty does not
trim, so " " was accepted as a pattern and only failed later when the
formatter was built, far from the call site.
Normalize at the holder boundary so blank always means "inherit the
parent or process-wide default", and reject empty/whitespace in the three
builder methods so the mistake surfaces at the call site. null keeps its
existing "not set" meaning, consistent with the sibling builder
parameters.
|
Currently, exposing methods like Could we consider consolidating these into a unified configuration builder instead? For example: .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 This would be much more extensible for future date/time types. What do you think about this? |
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:@DateTimeFormat("...").defaultDateFormat(...)/.defaultLocalDateFormat(...)/.defaultYearFormat(...)builder parametersDateUtils.setDefaultDateFormat(...)etc. process-wide settersyyyy-MM-dd HH:mm:ss/yyyy-MM-dd/yyyyEach 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
defaultYearFormatwas added in feat: add Character and Year converters (#1017) #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/defaultLocalDateFormathave shipped since 2.0.0, so they stay binary/source compatible:volatile+@Deprecated, with setter accessors following theUrlImageConverter.fetchPolicyprecedent. Removal targeted at the next major version.resolve*Formathelpers are opt-in infrastructure, not a mandate — documented in the new "Default date formats" section as guidance for the [Task] Implement more commonly used converter classes #1017 converter pipeline.Behavior audit
DateUtils.defaultDateFormat = xfield writevolatile); in-flight workbooks keep their snapshotDateUtils.format(date, null)direct util call@DateTimeFormaton Date/LocalDateTime/LocalDate writeYearstring read / empty-@DateTimeFormatStringfield reading a numeric date cellCoordination
YearNumberConverter): no overlap — it owns the(Year, NUMBER)key and serial semantics, this PR only rewiresYearStringConverter.fillDataFormat(cellData, format, DateUtils.defaultDateFormat)fallback should switch toDateUtils.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 inread/format.mdand three new rows inhelp/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 (styledataFormatStringassertions throughExcelAssertions), mid-write global flip does not affect the running workbook, CSV write uses the per-call default.spotless:checkandjavadoc:javadocclean.