From a0fcf4718653760c4af62051ee7604c4870e0cf6 Mon Sep 17 00:00:00 2001 From: liuhy Date: Mon, 27 Jul 2026 06:24:44 -0700 Subject: [PATCH 1/5] fix: copy default read converter map to isolate per-reader custom converters (#971) The workbook-level read holder aliased DefaultConverterLoader's shared static allConverter map instead of copying it, so custom converters registered via registerConverter() were put() into the global map and leaked into every later, unrelated read on the same JVM. Mirror the write side (AbstractWriteHolder:271), which already copies, and add a regression test. --- .../metadata/holder/AbstractReadHolder.java | 5 +- .../read/ReadConverterIsolationTest.java | 104 ++++++++++++++++++ 2 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java index d85c1d200..b374ec5e1 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java @@ -120,7 +120,10 @@ public AbstractReadHolder(ReadBasicParameter readBasicParameter, AbstractReadHol } if (parentAbstractReadHolder == null) { - setConverterMap(DefaultConverterLoader.loadDefaultReadConverter()); + // Copy the defaults instead of aliasing the shared static map, otherwise custom + // converters registered below mutate DefaultConverterLoader's global map and leak + // into every later read. Mirrors the write side (AbstractWriteHolder). + setConverterMap(new HashMap<>(DefaultConverterLoader.loadDefaultReadConverter())); } else { setConverterMap(new HashMap<>(parentAbstractReadHolder.getConverterMap())); } diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java new file mode 100644 index 000000000..21a482315 --- /dev/null +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package org.apache.fesod.sheet.read; + +import java.io.File; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import lombok.Data; +import org.apache.fesod.sheet.FesodSheet; +import org.apache.fesod.sheet.converters.Converter; +import org.apache.fesod.sheet.enums.CellDataTypeEnum; +import org.apache.fesod.sheet.metadata.GlobalConfiguration; +import org.apache.fesod.sheet.metadata.data.ReadCellData; +import org.apache.fesod.sheet.metadata.data.WriteCellData; +import org.apache.fesod.sheet.metadata.property.ExcelContentProperty; +import org.apache.fesod.sheet.read.listener.PageReadListener; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * A converter registered on one {@link org.apache.fesod.sheet.ExcelReader} must not leak into a + * later, unrelated read. + */ +public class ReadConverterIsolationTest { + + @Data + public static class StringRow { + private String value; + } + + /** Appends a marker so leakage is observable. */ + public static class MarkerConverter implements Converter { + @Override + public Class supportJavaTypeKey() { + return String.class; + } + + @Override + public CellDataTypeEnum supportExcelTypeKey() { + return CellDataTypeEnum.STRING; + } + + @Override + public String convertToJavaData( + ReadCellData cellData, + ExcelContentProperty contentProperty, + GlobalConfiguration globalConfiguration) { + return cellData.getStringValue() + " [MARKER]"; + } + + @Override + public WriteCellData convertToExcelData( + String value, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) { + return new WriteCellData<>(value); + } + } + + @Test + void registeredConverterDoesNotLeakIntoLaterRead() throws Exception { + File file = File.createTempFile("conv-iso", ".xlsx"); + file.deleteOnExit(); + StringRow out = new StringRow(); + out.setValue("hello"); + FesodSheet.write(file, StringRow.class).sheet().doWrite(Collections.singletonList(out)); + + // First read: register the marker converter -> values carry the marker. + List first = new ArrayList<>(); + FesodSheet.read(file, StringRow.class, new PageReadListener(first::addAll)) + .registerConverter(new MarkerConverter()) + .sheet() + .doRead(); + Assertions.assertEquals(Collections.singletonList("hello [MARKER]"), values(first)); + + // Second read: fresh reader, NO converter registered -> must NOT see the marker. + List second = new ArrayList<>(); + FesodSheet.read(file, StringRow.class, new PageReadListener(second::addAll)) + .sheet() + .doRead(); + Assertions.assertEquals(Collections.singletonList("hello"), values(second)); + } + + private static List values(List rows) { + List out = new ArrayList<>(); + for (StringRow r : rows) { + out.add(r.getValue()); + } + return out; + } +} From b36f79f92e9bbe8caf4a81b2211c40494325d05f Mon Sep 17 00:00:00 2001 From: liuhy Date: Mon, 27 Jul 2026 06:35:42 -0700 Subject: [PATCH 2/5] fix: use canonical ASF license header for ReadConverterIsolationTest --- .../read/ReadConverterIsolationTest.java | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java index 21a482315..1a718e4d8 100644 --- a/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java @@ -1,17 +1,19 @@ /* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with this - * work for additional information regarding copyright ownership. The ASF - * licenses this file to You under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance with the License. - * You may obtain a copy of the License at + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations * under the License. */ package org.apache.fesod.sheet.read; From 9f6bafda35e7f44d843289d41a671212b61768a2 Mon Sep 17 00:00:00 2001 From: liuhy Date: Mon, 27 Jul 2026 06:51:19 -0700 Subject: [PATCH 3/5] fix: separate license header from package with a blank line --- .../org/apache/fesod/sheet/read/ReadConverterIsolationTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java index 1a718e4d8..6a7f4101a 100644 --- a/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ + package org.apache.fesod.sheet.read; import java.io.File; From dcd8a3b536c51f751233849b21e1adcd66290bcf Mon Sep 17 00:00:00 2001 From: liuhy Date: Thu, 30 Jul 2026 18:33:55 -0700 Subject: [PATCH 4/5] fix: make default converter maps immutable --- .../converters/DefaultConverterLoader.java | 31 ++++++++++ .../metadata/holder/AbstractReadHolder.java | 5 +- .../metadata/holder/AbstractWriteHolder.java | 2 +- .../DefaultConverterLoaderTest.java | 60 +++++++++++++++++++ 4 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/DefaultConverterLoaderTest.java diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/DefaultConverterLoader.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/DefaultConverterLoader.java index 6200008b5..88b90d955 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/DefaultConverterLoader.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/DefaultConverterLoader.java @@ -25,6 +25,8 @@ package org.apache.fesod.sheet.converters; +import java.util.Collections; +import java.util.HashMap; import java.util.Map; import org.apache.fesod.common.util.MapUtils; import org.apache.fesod.sheet.converters.ConverterKeyBuild.ConverterKey; @@ -139,6 +141,7 @@ private static void initAllConverter() { putAllConverter(new StringNumberConverter()); putAllConverter(new StringStringConverter()); putAllConverter(new StringErrorConverter()); + allConverter = Collections.unmodifiableMap(allConverter); } private static void initDefaultWriteConverter() { @@ -176,6 +179,7 @@ private static void initDefaultWriteConverter() { putWriteStringConverter(new LongStringConverter()); putWriteStringConverter(new ShortStringConverter()); putWriteStringConverter(new StringStringConverter()); + defaultWriteConverter = Collections.unmodifiableMap(defaultWriteConverter); } /** @@ -187,6 +191,15 @@ public static Map> loadDefaultWriteConverter() { return defaultWriteConverter; } + /** + * Copy default write converter + * + * @return + */ + public static Map> copyDefaultWriteConverter() { + return new HashMap<>(loadDefaultWriteConverter()); + } + private static void putWriteConverter(Converter converter) { defaultWriteConverter.put(ConverterKeyBuild.buildKey(converter.supportJavaTypeKey()), converter); } @@ -205,6 +218,15 @@ public static Map> loadDefaultReadConverter() { return loadAllConverter(); } + /** + * Copy default read converter + * + * @return + */ + public static Map> copyDefaultReadConverter() { + return new HashMap<>(loadDefaultReadConverter()); + } + /** * Load all converter * @@ -214,6 +236,15 @@ public static Map> loadAllConverter() { return allConverter; } + /** + * Copy all converter + * + * @return + */ + public static Map> copyAllConverter() { + return new HashMap<>(loadAllConverter()); + } + private static void putAllConverter(Converter converter) { allConverter.put( ConverterKeyBuild.buildKey(converter.supportJavaTypeKey(), converter.supportExcelTypeKey()), converter); diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java index b374ec5e1..f7b58be88 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java @@ -120,10 +120,7 @@ public AbstractReadHolder(ReadBasicParameter readBasicParameter, AbstractReadHol } if (parentAbstractReadHolder == null) { - // Copy the defaults instead of aliasing the shared static map, otherwise custom - // converters registered below mutate DefaultConverterLoader's global map and leak - // into every later read. Mirrors the write side (AbstractWriteHolder). - setConverterMap(new HashMap<>(DefaultConverterLoader.loadDefaultReadConverter())); + setConverterMap(DefaultConverterLoader.copyDefaultReadConverter()); } else { setConverterMap(new HashMap<>(parentAbstractReadHolder.getConverterMap())); } diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/holder/AbstractWriteHolder.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/holder/AbstractWriteHolder.java index 293f18adc..e658b5d8e 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/holder/AbstractWriteHolder.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/holder/AbstractWriteHolder.java @@ -268,7 +268,7 @@ public AbstractWriteHolder(WriteBasicParameter writeBasicParameter, AbstractWrit // Set converterMap if (parentAbstractWriteHolder == null) { - setConverterMap(new HashMap<>(DefaultConverterLoader.loadDefaultWriteConverter())); + setConverterMap(DefaultConverterLoader.copyDefaultWriteConverter()); } else { setConverterMap(new HashMap<>(parentAbstractWriteHolder.getConverterMap())); if (CollectionUtils.isNotEmpty(parentAbstractWriteHolder.getCustomConverterList())) { diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/DefaultConverterLoaderTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/DefaultConverterLoaderTest.java new file mode 100644 index 000000000..de7c05c90 --- /dev/null +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/DefaultConverterLoaderTest.java @@ -0,0 +1,60 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.sheet.converters; + +import java.util.Map; +import org.apache.fesod.sheet.converters.ConverterKeyBuild.ConverterKey; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class DefaultConverterLoaderTest { + + @Test + void loadDefaultWriteConverterIsImmutableAndCopyIsMutable() { + assertLoadIsImmutableAndCopyIsMutable( + DefaultConverterLoader.loadDefaultWriteConverter(), + DefaultConverterLoader.copyDefaultWriteConverter()); + } + + @Test + void loadDefaultReadConverterIsImmutableAndCopyIsMutable() { + assertLoadIsImmutableAndCopyIsMutable( + DefaultConverterLoader.loadDefaultReadConverter(), + DefaultConverterLoader.copyDefaultReadConverter()); + } + + @Test + void loadAllConverterIsImmutableAndCopyIsMutable() { + assertLoadIsImmutableAndCopyIsMutable( + DefaultConverterLoader.loadAllConverter(), DefaultConverterLoader.copyAllConverter()); + } + + private static void assertLoadIsImmutableAndCopyIsMutable( + Map> loaded, Map> copy) { + Map.Entry> entry = loaded.entrySet().iterator().next(); + + Assertions.assertThrows( + UnsupportedOperationException.class, () -> loaded.put(entry.getKey(), entry.getValue())); + + copy.remove(entry.getKey()); + Assertions.assertFalse(copy.containsKey(entry.getKey())); + Assertions.assertTrue(loaded.containsKey(entry.getKey())); + } +} From fabc6b33e51cc6d6cd42fa9337461c373147080f Mon Sep 17 00:00:00 2001 From: liuhy Date: Thu, 30 Jul 2026 18:41:18 -0700 Subject: [PATCH 5/5] style: format default converter loader test --- .../sheet/converters/DefaultConverterLoaderTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/DefaultConverterLoaderTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/DefaultConverterLoaderTest.java index de7c05c90..e05837b86 100644 --- a/fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/DefaultConverterLoaderTest.java +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/DefaultConverterLoaderTest.java @@ -29,15 +29,13 @@ public class DefaultConverterLoaderTest { @Test void loadDefaultWriteConverterIsImmutableAndCopyIsMutable() { assertLoadIsImmutableAndCopyIsMutable( - DefaultConverterLoader.loadDefaultWriteConverter(), - DefaultConverterLoader.copyDefaultWriteConverter()); + DefaultConverterLoader.loadDefaultWriteConverter(), DefaultConverterLoader.copyDefaultWriteConverter()); } @Test void loadDefaultReadConverterIsImmutableAndCopyIsMutable() { assertLoadIsImmutableAndCopyIsMutable( - DefaultConverterLoader.loadDefaultReadConverter(), - DefaultConverterLoader.copyDefaultReadConverter()); + DefaultConverterLoader.loadDefaultReadConverter(), DefaultConverterLoader.copyDefaultReadConverter()); } @Test @@ -48,7 +46,8 @@ void loadAllConverterIsImmutableAndCopyIsMutable() { private static void assertLoadIsImmutableAndCopyIsMutable( Map> loaded, Map> copy) { - Map.Entry> entry = loaded.entrySet().iterator().next(); + Map.Entry> entry = + loaded.entrySet().iterator().next(); Assertions.assertThrows( UnsupportedOperationException.class, () -> loaded.put(entry.getKey(), entry.getValue()));