Skip to content

Commit 893abd3

Browse files
Aias00liuhybengbengbalabalabengpsxjoy
authored
fix: copy default read converter map to isolate per-reader custom converters (#972)
* 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. * fix: use canonical ASF license header for ReadConverterIsolationTest * fix: separate license header from package with a blank line * fix: make default converter maps immutable * style: format default converter loader test --------- Co-authored-by: liuhy <liuhy@apache.org> Co-authored-by: Bengbengbalabalabeng <70380092+bengbengbalabalabeng@users.noreply.github.com> Co-authored-by: Shuxin Pan <psxjoy@apache.org>
1 parent c7993d5 commit 893abd3

5 files changed

Lines changed: 199 additions & 2 deletions

File tree

‎fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/DefaultConverterLoader.java‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525

2626
package org.apache.fesod.sheet.converters;
2727

28+
import java.util.Collections;
29+
import java.util.HashMap;
2830
import java.util.Map;
2931
import org.apache.fesod.common.util.MapUtils;
3032
import org.apache.fesod.sheet.converters.ConverterKeyBuild.ConverterKey;
@@ -139,6 +141,7 @@ private static void initAllConverter() {
139141
putAllConverter(new StringNumberConverter());
140142
putAllConverter(new StringStringConverter());
141143
putAllConverter(new StringErrorConverter());
144+
allConverter = Collections.unmodifiableMap(allConverter);
142145
}
143146

144147
private static void initDefaultWriteConverter() {
@@ -176,6 +179,7 @@ private static void initDefaultWriteConverter() {
176179
putWriteStringConverter(new LongStringConverter());
177180
putWriteStringConverter(new ShortStringConverter());
178181
putWriteStringConverter(new StringStringConverter());
182+
defaultWriteConverter = Collections.unmodifiableMap(defaultWriteConverter);
179183
}
180184

181185
/**
@@ -187,6 +191,15 @@ public static Map<ConverterKey, Converter<?>> loadDefaultWriteConverter() {
187191
return defaultWriteConverter;
188192
}
189193

194+
/**
195+
* Copy default write converter
196+
*
197+
* @return
198+
*/
199+
public static Map<ConverterKey, Converter<?>> copyDefaultWriteConverter() {
200+
return new HashMap<>(loadDefaultWriteConverter());
201+
}
202+
190203
private static void putWriteConverter(Converter<?> converter) {
191204
defaultWriteConverter.put(ConverterKeyBuild.buildKey(converter.supportJavaTypeKey()), converter);
192205
}
@@ -205,6 +218,15 @@ public static Map<ConverterKey, Converter<?>> loadDefaultReadConverter() {
205218
return loadAllConverter();
206219
}
207220

221+
/**
222+
* Copy default read converter
223+
*
224+
* @return
225+
*/
226+
public static Map<ConverterKey, Converter<?>> copyDefaultReadConverter() {
227+
return new HashMap<>(loadDefaultReadConverter());
228+
}
229+
208230
/**
209231
* Load all converter
210232
*
@@ -214,6 +236,15 @@ public static Map<ConverterKey, Converter<?>> loadAllConverter() {
214236
return allConverter;
215237
}
216238

239+
/**
240+
* Copy all converter
241+
*
242+
* @return
243+
*/
244+
public static Map<ConverterKey, Converter<?>> copyAllConverter() {
245+
return new HashMap<>(loadAllConverter());
246+
}
247+
217248
private static void putAllConverter(Converter<?> converter) {
218249
allConverter.put(
219250
ConverterKeyBuild.buildKey(converter.supportJavaTypeKey(), converter.supportExcelTypeKey()), converter);

‎fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/holder/AbstractReadHolder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ public AbstractReadHolder(ReadBasicParameter readBasicParameter, AbstractReadHol
120120
}
121121

122122
if (parentAbstractReadHolder == null) {
123-
setConverterMap(DefaultConverterLoader.loadDefaultReadConverter());
123+
setConverterMap(DefaultConverterLoader.copyDefaultReadConverter());
124124
} else {
125125
setConverterMap(new HashMap<>(parentAbstractReadHolder.getConverterMap()));
126126
}

‎fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/holder/AbstractWriteHolder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ public AbstractWriteHolder(WriteBasicParameter writeBasicParameter, AbstractWrit
268268

269269
// Set converterMap
270270
if (parentAbstractWriteHolder == null) {
271-
setConverterMap(new HashMap<>(DefaultConverterLoader.loadDefaultWriteConverter()));
271+
setConverterMap(DefaultConverterLoader.copyDefaultWriteConverter());
272272
} else {
273273
setConverterMap(new HashMap<>(parentAbstractWriteHolder.getConverterMap()));
274274
if (CollectionUtils.isNotEmpty(parentAbstractWriteHolder.getCustomConverterList())) {

‎fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/DefaultConverterLoaderTest.java‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.apache.fesod.sheet.converters;
21+
22+
import java.util.Map;
23+
import org.apache.fesod.sheet.converters.ConverterKeyBuild.ConverterKey;
24+
import org.junit.jupiter.api.Assertions;
25+
import org.junit.jupiter.api.Test;
26+
27+
public class DefaultConverterLoaderTest {
28+
29+
@Test
30+
void loadDefaultWriteConverterIsImmutableAndCopyIsMutable() {
31+
assertLoadIsImmutableAndCopyIsMutable(
32+
DefaultConverterLoader.loadDefaultWriteConverter(), DefaultConverterLoader.copyDefaultWriteConverter());
33+
}
34+
35+
@Test
36+
void loadDefaultReadConverterIsImmutableAndCopyIsMutable() {
37+
assertLoadIsImmutableAndCopyIsMutable(
38+
DefaultConverterLoader.loadDefaultReadConverter(), DefaultConverterLoader.copyDefaultReadConverter());
39+
}
40+
41+
@Test
42+
void loadAllConverterIsImmutableAndCopyIsMutable() {
43+
assertLoadIsImmutableAndCopyIsMutable(
44+
DefaultConverterLoader.loadAllConverter(), DefaultConverterLoader.copyAllConverter());
45+
}
46+
47+
private static void assertLoadIsImmutableAndCopyIsMutable(
48+
Map<ConverterKey, Converter<?>> loaded, Map<ConverterKey, Converter<?>> copy) {
49+
Map.Entry<ConverterKey, Converter<?>> entry =
50+
loaded.entrySet().iterator().next();
51+
52+
Assertions.assertThrows(
53+
UnsupportedOperationException.class, () -> loaded.put(entry.getKey(), entry.getValue()));
54+
55+
copy.remove(entry.getKey());
56+
Assertions.assertFalse(copy.containsKey(entry.getKey()));
57+
Assertions.assertTrue(loaded.containsKey(entry.getKey()));
58+
}
59+
}

‎fesod-sheet/src/test/java/org/apache/fesod/sheet/read/ReadConverterIsolationTest.java‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.apache.fesod.sheet.read;
21+
22+
import java.io.File;
23+
import java.util.ArrayList;
24+
import java.util.Collections;
25+
import java.util.List;
26+
import lombok.Data;
27+
import org.apache.fesod.sheet.FesodSheet;
28+
import org.apache.fesod.sheet.converters.Converter;
29+
import org.apache.fesod.sheet.enums.CellDataTypeEnum;
30+
import org.apache.fesod.sheet.metadata.GlobalConfiguration;
31+
import org.apache.fesod.sheet.metadata.data.ReadCellData;
32+
import org.apache.fesod.sheet.metadata.data.WriteCellData;
33+
import org.apache.fesod.sheet.metadata.property.ExcelContentProperty;
34+
import org.apache.fesod.sheet.read.listener.PageReadListener;
35+
import org.junit.jupiter.api.Assertions;
36+
import org.junit.jupiter.api.Test;
37+
38+
/**
39+
* A converter registered on one {@link org.apache.fesod.sheet.ExcelReader} must not leak into a
40+
* later, unrelated read.
41+
*/
42+
public class ReadConverterIsolationTest {
43+
44+
@Data
45+
public static class StringRow {
46+
private String value;
47+
}
48+
49+
/** Appends a marker so leakage is observable. */
50+
public static class MarkerConverter implements Converter<String> {
51+
@Override
52+
public Class<?> supportJavaTypeKey() {
53+
return String.class;
54+
}
55+
56+
@Override
57+
public CellDataTypeEnum supportExcelTypeKey() {
58+
return CellDataTypeEnum.STRING;
59+
}
60+
61+
@Override
62+
public String convertToJavaData(
63+
ReadCellData<?> cellData,
64+
ExcelContentProperty contentProperty,
65+
GlobalConfiguration globalConfiguration) {
66+
return cellData.getStringValue() + " [MARKER]";
67+
}
68+
69+
@Override
70+
public WriteCellData<?> convertToExcelData(
71+
String value, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
72+
return new WriteCellData<>(value);
73+
}
74+
}
75+
76+
@Test
77+
void registeredConverterDoesNotLeakIntoLaterRead() throws Exception {
78+
File file = File.createTempFile("conv-iso", ".xlsx");
79+
file.deleteOnExit();
80+
StringRow out = new StringRow();
81+
out.setValue("hello");
82+
FesodSheet.write(file, StringRow.class).sheet().doWrite(Collections.singletonList(out));
83+
84+
// First read: register the marker converter -> values carry the marker.
85+
List<StringRow> first = new ArrayList<>();
86+
FesodSheet.read(file, StringRow.class, new PageReadListener<StringRow>(first::addAll))
87+
.registerConverter(new MarkerConverter())
88+
.sheet()
89+
.doRead();
90+
Assertions.assertEquals(Collections.singletonList("hello [MARKER]"), values(first));
91+
92+
// Second read: fresh reader, NO converter registered -> must NOT see the marker.
93+
List<StringRow> second = new ArrayList<>();
94+
FesodSheet.read(file, StringRow.class, new PageReadListener<StringRow>(second::addAll))
95+
.sheet()
96+
.doRead();
97+
Assertions.assertEquals(Collections.singletonList("hello"), values(second));
98+
}
99+
100+
private static List<String> values(List<StringRow> rows) {
101+
List<String> out = new ArrayList<>();
102+
for (StringRow r : rows) {
103+
out.add(r.getValue());
104+
}
105+
return out;
106+
}
107+
}

0 commit comments

Comments
 (0)