Skip to content

fix: key the date format cache by format index and string - #1304

Open
hanzhe-one wants to merge 3 commits into
apache:mainfrom
hanzhe-one:fix/date-format-cache-key
Open

hanzhe-one wants to merge 3 commits into
apache:mainfrom
hanzhe-one:fix/date-format-cache-key

Conversation

@hanzhe-one

@hanzhe-one hanzhe-one commented Nov 2, 2026 •

Copy link
Copy Markdown

Purpose of the pull request

Closed: #1303

ExcelDateFormatDetector cached date-format detection in a thread-local Map<Short, Boolean> keyed by the format index alone. Custom format indexes (>= 164) are defined per workbook, so the same index can map to different format strings. Once an index was cached, later queries with a different string returned the earlier result:

isADateFormat((short) 164, "yyyy-mm-dd") -> true
isADateFormat((short) 164, "0.00")       -> true   // should be false

Reverse order made a date format report false. isADateFormatUncached returns the correct values, so the cache disagreed with the detector.

What's changed?

  • The thread-local cache is now Map<String, Boolean> keyed by formatIndex plus the format string; a null format string maps to a stable key.
  • Detection logic, public signatures and cache clearing are unchanged.
  • Existing DateUtilsTest.test_isADateFormat_Cache now asserts the new key shape instead of reflecting on a Short key.
  • Four new tests cover: same index with different strings in both orders, null format string, and the internal-date-index path.

The cache is cleared in WriteContextImpl.finish and ExcelAnalyserImpl.close, so a completed read or write normally starts fresh. This change removes the reliance on that clearing for correctness when the same thread queries one index with different format strings.

Validation

  • New tests fail against the unfixed implementation (3 failures) and pass with the change.
  • ./mvnw -pl fesod-sheet test: 1,793 tests, 0 failures. DateUtilsTest: 94 tests, 0 failures.
  • ./mvnw spotless:check: passes.

The only errors in that run are 10 in UrlImageConverterTest, all IOException: Unable to establish loopback connection when the test starts an HttpServer. The same 10 errors occur on unmodified main in this environment, so they are a sandbox networking restriction rather than a regression from this change; CI runs without that restriction.

Note: open PR #1260 also modifies ExcelDateFormatDetector when centralizing thread-local cleanup; whichever lands second needs a trivial rebase.

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.

@Mikkey-f Mikkey-f left a comment •

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.

Thanks @hanzhe-one — I reproduced the bug and verified the fix locally (Windows 11, JDK 17).

Root cause: DATE_THREAD_LOCAL is keyed by the format index alone. Custom indexes are workbook-scoped, so the same index can map to different format strings. Calling DateUtils.isADateFormat(164, "yyyy-mm-dd") then (164, "0.00") returns true twice; the reverse order returns false twice, while isADateFormatUncached gives the correct pair.

Verification

  • Red: with only ExcelDateFormatDetector reverted (your tests kept), 3 new tests fail as described — both orders plus the null format string; the internal-index test passes either way.
  • Green: DateUtilsTest 94/94.
  • Full suite: 60 + 1,793 tests, 0 failures; spotless:check clean. I didn't hit the UrlImageConverterTest loopback errors here, consistent with your sandbox note.

Design check: Mapping a null format string to "" is behavior-preserving — isADateFormatUncached treats both as empty after the internal-index check — and the NUL (0x00) separator keeps the composite key unambiguous. The cache lifecycle is unchanged: removeThreadLocalCache() still runs from ExcelAnalyserImpl.close and WriteContextImpl.finish, so normal read/write paths remain operation-scoped.

One nit (non-blocking): test_isADateFormat_Cache now asserts size and containsValue(true), but the key shape is the point of the fix — asserting the exact composite key (formatIndex + NUL + formatString) would pin it directly.

LGTM — the cache now agrees with the uncached detector, and both orders are covered.

@hanzhe-one
hanzhe-one requested a review from Mikkey-f November 5, 2026 13:59
@alaahong
alaahong requested a balanced review from Copilot and removed request for Mikkey-f November 7, 2026 01:37

Copilot AI left a comment

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.

🟢 Approval recommended

No active review issues remain for the date-format cache-key change.

No open issues 🎉

What changed in this PR

Fixes #1303 by making date-format cache results depend on both the format index and format string, preventing stale results when workbooks reuse an index for different formats.

Changes:

  • Uses a composite string cache key with stable handling of null format strings.
  • Updates cache assertions and adds regression tests for differing strings, call order, null strings, and built-in date indexes.
  • Preserves detection logic, public signatures, and cache cleanup behavior.
File Description
fesod-sheet/​src/​test/​java/​org/​apache/​fesod/​sheet/​util/​DateUtilsTest.java Updates cache assertions and adds regression coverage.
fesod-sheet/​src/​main/​java/​org/​apache/​fesod/​sheet/​util/​ExcelDateFormatDetector.java Keys cached results by format index and string.

Review effort: Balanced

This branch has not been deployed

No deployments
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.

[Bug] Date format cache is keyed by format index only and returns stale results

4 participants