fix: ensure converter isolation across ExcelWriter and ExcelReader #740
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes a converter isolation bug where custom converters registered in one ExcelWriter or ExcelReader instance would leak to other instances due to shared mutable static maps.
Key Changes:
- Wrapped default converter maps with
Collections.unmodifiableMap()inDefaultConverterLoaderto prevent direct modification - Modified holder initialization to create defensive mutable copies of converter maps for each instance
- Added test case to verify converter isolation between
ExcelWriterinstances
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
fesod/src/main/java/org/apache/fesod/sheet/converters/DefaultConverterLoader.java |
Returns unmodifiable maps from loadDefaultWriteConverter() and loadAllConverter() methods to protect static converter maps |
fesod/src/main/java/org/apache/fesod/sheet/write/metadata/holder/AbstractWriteHolder.java |
Creates mutable HashMap copy from unmodifiable default converter map during holder initialization |
fesod/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java |
Creates mutable HashMap copy from unmodifiable default converter map during holder initialization |
fesod/src/test/java/org/apache/fesod/sheet/converter/ConverterIsolationTest.java |
Adds new test to validate custom converters do not leak between ExcelWriter instances |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
56
to
72
| @Test | ||
| public void testConverterIsolation() { | ||
| ExcelWriter writer1 = FesodSheet.write(new File(TestFileUtil.getPath() + "writer1.xlsx"), TestData.class) | ||
| .registerConverter(new ConverterA()) | ||
| .build(); | ||
|
|
||
| ExcelWriter writer2 = FesodSheet.write(new File(TestFileUtil.getPath() + "writer2.xlsx"), TestData.class) | ||
| .build(); | ||
|
|
||
| boolean writer2HasConverterA = writer2.writeContext().currentWriteHolder().converterMap().values().stream() | ||
| .anyMatch(c -> c instanceof ConverterA); | ||
|
|
||
| writer1.finish(); | ||
| writer2.finish(); | ||
|
|
||
| assertFalse(writer2HasConverterA, "Custom converter should not leak between ExcelWriter instances"); | ||
| } |
There was a problem hiding this comment.
The test only validates converter isolation for ExcelWriter instances, but the PR description and code changes indicate that the fix also applies to ExcelReader instances. Consider adding a corresponding test case for ExcelReader to ensure complete coverage of the bug fix and prevent regression in the read path.
YangSiJun528
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose of the pull request
Closes #729
Fixes converter isolation bug where custom converters registered in one
ExcelWriterorExcelReaderinstance would leak to other instances.What's changed?
DefaultConverterLoader.loadDefaultWriteConverter()andDefaultConverterLoader.loadAllConverter()to return unmodifiable mapAbstractWriteHolderandAbstractReadHolderto create mutable copies from the unmodifiable mapsConverterIsolationTestto verify converters are properly isolated between instancesChecklist