Fix custom converters not being inherited from parent write holders#893
Fix custom converters not being inherited from parent write holders#893utafrali wants to merge 1 commit intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a write-time converter inheritance bug where globally registered custom converters were not reliably available in child write holders (e.g., WriteSheetHolder), ensuring consistent converter lookup throughout the write context.
Changes:
- Track per-holder custom converters via a new
customConverterListonAbstractWriteHolder. - Initialize child holders’
converterMapwith inherited custom converters from the parent holder. - Add regression tests (plus test data model) to verify global converter availability in sheet holders and during CSV writes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/holder/AbstractWriteHolder.java | Adds holder-level custom converter tracking and applies inherited converters during child holder initialization. |
| fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/CustomConverterTest.java | Adds regression tests validating global converter inheritance in WriteSheetHolder and successful conversion during CSV writes. |
| fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/GlobalConverterWriteData.java | New test data class used to validate global converter behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Test | ||
| void t06GlobalConverterWriteWithoutAnnotation() throws Exception { | ||
| FesodSheet.write(converterCsvFile14) | ||
| .registerConverter(new TimestampStringConverter()) | ||
| .sheet() | ||
| .doWrite(globalData()); | ||
| } |
There was a problem hiding this comment.
Test name t06GlobalConverterWriteWithoutAnnotation is misleading because GlobalConverterWriteData uses @ExcelProperty. Consider renaming the test to reflect the actual intent (e.g., “without converter annotation” / “without field-level converter config”).
Purpose
Closes #648
What's changed?
The global custom converters weren't being inherited by child write holders. The parent holder wasn't storing which custom converters it had, so there was nothing to pass down when initializing child holders. Added a
customConverterListfield to track custom converters and updated the initialization logic to apply inherited converters when building the converter map. Also added a test to verify the behavior works correctly.Checklist