From f9d7e3b9183f542c363dce4b6ebc3b2db6b43ca3 Mon Sep 17 00:00:00 2001 From: lance Date: Mon, 7 Sep 2026 14:29:09 +0800 Subject: [PATCH 1/3] fix close unused xlsx sheet and workbook XML streams Signed-off-by: lance --- .../sheet/analysis/v07/XlsxSaxAnalyser.java | 80 ++++--- .../XlsxSaxAnalyserSheetStreamCloseTest.java | 213 ++++++++++++++++++ 2 files changed, 265 insertions(+), 28 deletions(-) create mode 100644 fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java index b65048ebf..308055eb8 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java @@ -149,6 +149,7 @@ public XlsxSaxAnalyser(XlsxReadContext xlsxReadContext, InputStream decryptedStr String sheetName = ite.getSheetName(); CTSheet ctSheet = ctSheetMap.get(sheetName); if (ctSheet == null) { + closeSheetInputStream(inputStream, "sheetName=" + sheetName); continue; } ReadSheet readSheet = new ReadSheet(index, sheetName); @@ -204,14 +205,15 @@ private void analysisUse1904WindowDate(XSSFReader xssfReader, XlsxReadWorkbookHo if (xlsxReadWorkbookHolder.getReadWorkbook().getUse1904windowing() != null) { return; } - InputStream workbookXml = xssfReader.getWorkbookData(); - WorkbookDocument ctWorkbook = WorkbookDocument.Factory.parse(workbookXml); - CTWorkbook wb = ctWorkbook.getWorkbook(); - CTWorkbookPr prefix = wb.getWorkbookPr(); - if (prefix != null && prefix.getDate1904()) { - xlsxReadWorkbookHolder.getGlobalConfiguration().setUse1904windowing(Boolean.TRUE); - } else { - xlsxReadWorkbookHolder.getGlobalConfiguration().setUse1904windowing(Boolean.FALSE); + try (InputStream workbookXml = xssfReader.getWorkbookData()) { + WorkbookDocument ctWorkbook = WorkbookDocument.Factory.parse(workbookXml); + CTWorkbook wb = ctWorkbook.getWorkbook(); + CTWorkbookPr prefix = wb.getWorkbookPr(); + if (prefix != null && prefix.getDate1904()) { + xlsxReadWorkbookHolder.getGlobalConfiguration().setUse1904windowing(Boolean.TRUE); + } else { + xlsxReadWorkbookHolder.getGlobalConfiguration().setUse1904windowing(Boolean.FALSE); + } } } @@ -224,13 +226,14 @@ private void analysisSharedStringsTable( private void analysisCtSheetMap(XSSFReader xssfReader, XlsxReadWorkbookHolder xlsxReadWorkbookHolder) throws Exception { - CTWorkbook wb = - WorkbookDocument.Factory.parse(xssfReader.getWorkbookData()).getWorkbook(); - for (CTSheet ctSheet : wb.getSheets().getSheetList()) { - boolean isHidden = - (ctSheet.getState() == STSheetState.HIDDEN) || (ctSheet.getState() == STSheetState.VERY_HIDDEN); - if (Boolean.FALSE.equals(xlsxReadWorkbookHolder.getIgnoreHiddenSheet()) || !isHidden) { - ctSheetMap.put(ctSheet.getName(), ctSheet); + try (InputStream workbookXml = xssfReader.getWorkbookData()) { + CTWorkbook wb = WorkbookDocument.Factory.parse(workbookXml).getWorkbook(); + for (CTSheet ctSheet : wb.getSheets().getSheetList()) { + boolean isHidden = + (ctSheet.getState() == STSheetState.HIDDEN) || (ctSheet.getState() == STSheetState.VERY_HIDDEN); + if (Boolean.FALSE.equals(xlsxReadWorkbookHolder.getIgnoreHiddenSheet()) || !isHidden) { + ctSheetMap.put(ctSheet.getName(), ctSheet); + } } } } @@ -312,22 +315,43 @@ private void parseXmlSource(InputStream inputStream, ContentHandler handler) { @Override public void execute() { - for (ReadSheet readSheet : sheetList) { - readSheet = SheetUtils.match(readSheet, xlsxReadContext); - if (readSheet != null) { - try { - xlsxReadContext.currentSheet(readSheet); - parseXmlSource(sheetMap.get(readSheet.getSheetNo()), new XlsxRowHandler(xlsxReadContext)); - // Read comments - readComments(readSheet); - } catch (ExcelAnalysisStopSheetException e) { - if (log.isDebugEnabled()) { - log.debug("Custom stop!", e); + try { + for (ReadSheet readSheet : sheetList) { + ReadSheet matchedSheet = SheetUtils.match(readSheet, xlsxReadContext); + if (matchedSheet != null) { + try { + xlsxReadContext.currentSheet(matchedSheet); + parseXmlSource(sheetMap.get(matchedSheet.getSheetNo()), new XlsxRowHandler(xlsxReadContext)); + // Read comments + readComments(matchedSheet); + } catch (ExcelAnalysisStopSheetException e) { + if (log.isDebugEnabled()) { + log.debug("Custom stop!", e); + } } + // The last sheet is read + xlsxReadContext.analysisEventProcessor().endSheet(xlsxReadContext); } - // The last sheet is read - xlsxReadContext.analysisEventProcessor().endSheet(xlsxReadContext); } + } finally { + closeRemainingSheetStreams(); + } + } + + private void closeRemainingSheetStreams() { + for (Map.Entry entry : sheetMap.entrySet()) { + closeSheetInputStream(entry.getValue(), "sheetNo=" + entry.getKey()); + } + } + + private void closeSheetInputStream(InputStream inputStream, String description) { + if (inputStream == null) { + return; + } + try { + inputStream.close(); + } catch (IOException e) { + log.warn("Failed to close sheet input stream, {}", description, e); } } diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java new file mode 100644 index 000000000..7a58d6e73 --- /dev/null +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java @@ -0,0 +1,213 @@ +/* + * 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.analysis.v07; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.lang.reflect.Field; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.fesod.sheet.ExcelReader; +import org.apache.fesod.sheet.ExcelWriter; +import org.apache.fesod.sheet.FesodSheet; +import org.apache.fesod.sheet.context.AnalysisContext; +import org.apache.fesod.sheet.event.AnalysisEventListener; +import org.apache.fesod.sheet.read.metadata.ReadSheet; +import org.apache.fesod.sheet.testkit.Tags; +import org.apache.fesod.sheet.testkit.base.AbstractExcelTest; +import org.apache.fesod.sheet.testkit.builders.TestDataBuilder; +import org.apache.fesod.sheet.testkit.enums.ExcelFormat; +import org.apache.fesod.sheet.testkit.listeners.CollectingReadListener; +import org.apache.fesod.sheet.testkit.models.SimpleData; +import org.apache.fesod.sheet.write.metadata.WriteSheet; +import org.apache.poi.xssf.usermodel.XSSFWorkbook; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Verifies that {@link XlsxSaxAnalyser} closes sheet {@link InputStream}s that are skipped or left unread. + */ +@Tag(Tags.READ) +@Tag(Tags.ROUND_TRIP) +class XlsxSaxAnalyserSheetStreamCloseTest extends AbstractExcelTest { + + @Test + void execute_closesUnreadSheetStreams() throws Exception { + File file = writeThreeSheets(); + CollectingReadListener listener = new CollectingReadListener(); + + try (ExcelReader excelReader = + FesodSheet.read(file, SimpleData.class, listener).build()) { + XlsxSaxAnalyser analyser = (XlsxSaxAnalyser) excelReader.excelExecutor(); + Map sheetMap = sheetMap(analyser); + Assertions.assertEquals(3, sheetMap.size()); + + Map tracked = wrapSheetStreams(sheetMap); + excelReader.read(FesodSheet.readSheet(0).build()); + + Assertions.assertEquals(1, listener.getRowCount()); + Assertions.assertEquals("sheet1", listener.getFirstRow().getName()); + assertAllClosed(tracked); + } + } + + @Test + void execute_closesRemainingSheetStreams_whenListenerThrows() throws Exception { + File file = writeThreeSheets(); + AnalysisEventListener failingListener = new AnalysisEventListener() { + @Override + public void invoke(SimpleData data, AnalysisContext context) { + throw new IllegalStateException("boom"); + } + + @Override + public void doAfterAllAnalysed(AnalysisContext context) {} + }; + + try (ExcelReader excelReader = + FesodSheet.read(file, SimpleData.class, failingListener).build()) { + XlsxSaxAnalyser analyser = (XlsxSaxAnalyser) excelReader.excelExecutor(); + Map tracked = wrapSheetStreams(sheetMap(analyser)); + + Assertions.assertThrows(IllegalStateException.class, excelReader::readAll); + assertAllClosed(tracked); + } + } + + @Test + void constructor_doesNotRetainHiddenSheetStreams_whenIgnoreHiddenSheet() throws Exception { + File file = writeWorkbookWithHiddenSheet(); + + try (ExcelReader excelReader = + FesodSheet.read(file).ignoreHiddenSheet(Boolean.TRUE).build()) { + XlsxSaxAnalyser analyser = (XlsxSaxAnalyser) excelReader.excelExecutor(); + List sheets = analyser.sheetList(); + Map sheetMap = sheetMap(analyser); + + Assertions.assertEquals(2, sheets.size()); + Assertions.assertEquals(2, sheetMap.size()); + Assertions.assertFalse(containsSheetName(sheets, "Hidden")); + Assertions.assertTrue(containsSheetName(sheets, "Visible1")); + Assertions.assertTrue(containsSheetName(sheets, "Visible2")); + + Map tracked = wrapSheetStreams(sheetMap); + excelReader.readAll(); + assertAllClosed(tracked); + } + } + + private File writeThreeSheets() { + File file = createTempFileUnchecked(ExcelFormat.XLSX); + try (ExcelWriter excelWriter = FesodSheet.write(file, SimpleData.class).build()) { + WriteSheet sheet1 = FesodSheet.writerSheet(0, "Sheet1").build(); + WriteSheet sheet2 = FesodSheet.writerSheet(1, "Sheet2").build(); + WriteSheet sheet3 = FesodSheet.writerSheet(2, "Sheet3").build(); + excelWriter.write(namedData("sheet1"), sheet1); + excelWriter.write(namedData("sheet2"), sheet2); + excelWriter.write(namedData("sheet3"), sheet3); + } + return file; + } + + private File writeWorkbookWithHiddenSheet() throws IOException { + File file = createTempFileUnchecked(ExcelFormat.XLSX); + try (XSSFWorkbook workbook = new XSSFWorkbook()) { + workbook.createSheet("Visible1").createRow(0).createCell(0).setCellValue("a"); + workbook.createSheet("Hidden").createRow(0).createCell(0).setCellValue("b"); + workbook.createSheet("Visible2").createRow(0).createCell(0).setCellValue("c"); + workbook.setSheetHidden(1, true); + try (FileOutputStream outputStream = new FileOutputStream(file)) { + workbook.write(outputStream); + } + } + return file; + } + + private File createTempFileUnchecked(ExcelFormat format) { + try { + return createTempFile(format); + } catch (IOException e) { + throw new IllegalStateException(e); + } + } + + private static List namedData(String name) { + List data = TestDataBuilder.simpleData(1); + data.get(0).setName(name); + return data; + } + + private static boolean containsSheetName(List sheets, String sheetName) { + for (ReadSheet sheet : sheets) { + if (sheetName.equals(sheet.getSheetName())) { + return true; + } + } + return false; + } + + @SuppressWarnings("unchecked") + private static Map sheetMap(XlsxSaxAnalyser analyser) throws Exception { + Field field = XlsxSaxAnalyser.class.getDeclaredField("sheetMap"); + field.setAccessible(true); + return (Map) field.get(analyser); + } + + private static Map wrapSheetStreams(Map sheetMap) { + Map tracked = new HashMap(); + for (Map.Entry entry : sheetMap.entrySet()) { + CloseTrackingInputStream wrapped = new CloseTrackingInputStream(entry.getValue()); + tracked.put(entry.getKey(), wrapped); + sheetMap.put(entry.getKey(), wrapped); + } + return tracked; + } + + private static void assertAllClosed(Map tracked) { + Assertions.assertFalse(tracked.isEmpty()); + for (Map.Entry entry : tracked.entrySet()) { + Assertions.assertTrue( + entry.getValue().isClosed(), "Sheet stream should be closed, sheetNo=" + entry.getKey()); + } + } + + private static final class CloseTrackingInputStream extends FilterInputStream { + private boolean closed; + + private CloseTrackingInputStream(InputStream in) { + super(in); + } + + @Override + public void close() throws IOException { + closed = true; + super.close(); + } + + private boolean isClosed() { + return closed; + } + } +} From bbac578401276c1e837de9977e0e974ff5e0d90d Mon Sep 17 00:00:00 2001 From: lance Date: Mon, 7 Sep 2026 14:42:14 +0800 Subject: [PATCH 2/3] fix close unused xlsx sheet and workbook XML streams Signed-off-by: lance --- .../sheet/analysis/ExcelAnalyserImpl.java | 7 ++++ .../sheet/analysis/ExcelReadExecutor.java | 8 +++++ .../sheet/analysis/v07/XlsxSaxAnalyser.java | 35 ++++++++++--------- .../XlsxSaxAnalyserSheetStreamCloseTest.java | 35 +++++++++++++++++-- 4 files changed, 65 insertions(+), 20 deletions(-) diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelAnalyserImpl.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelAnalyserImpl.java index 14ed82bf2..459b2d004 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelAnalyserImpl.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelAnalyserImpl.java @@ -233,6 +233,13 @@ public void finish() { } catch (Throwable t) { throwable = t; } + try { + if (excelReadExecutor != null) { + excelReadExecutor.close(); + } + } catch (Throwable t) { + throwable = t; + } try { if ((readWorkbookHolder instanceof XlsxReadWorkbookHolder) && ((XlsxReadWorkbookHolder) readWorkbookHolder).getOpcPackage() != null) { diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelReadExecutor.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelReadExecutor.java index d7700fca0..41883289d 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelReadExecutor.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelReadExecutor.java @@ -46,4 +46,12 @@ public interface ExcelReadExecutor { * Read the sheet. */ void execute(); + + /** + * Release resources held by this executor. Invoked when the reader is finished. + * + *

XLSX sheet streams are opened up front and may be consumed across multiple + * {@link #execute()} calls, so leftover streams must not be closed until this method runs. + */ + default void close() {} } diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java index 308055eb8..90dde6dda 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java @@ -315,29 +315,30 @@ private void parseXmlSource(InputStream inputStream, ContentHandler handler) { @Override public void execute() { - try { - for (ReadSheet readSheet : sheetList) { - ReadSheet matchedSheet = SheetUtils.match(readSheet, xlsxReadContext); - if (matchedSheet != null) { - try { - xlsxReadContext.currentSheet(matchedSheet); - parseXmlSource(sheetMap.get(matchedSheet.getSheetNo()), new XlsxRowHandler(xlsxReadContext)); - // Read comments - readComments(matchedSheet); - } catch (ExcelAnalysisStopSheetException e) { - if (log.isDebugEnabled()) { - log.debug("Custom stop!", e); - } + for (ReadSheet readSheet : sheetList) { + ReadSheet matchedSheet = SheetUtils.match(readSheet, xlsxReadContext); + if (matchedSheet != null) { + try { + xlsxReadContext.currentSheet(matchedSheet); + parseXmlSource(sheetMap.get(matchedSheet.getSheetNo()), new XlsxRowHandler(xlsxReadContext)); + // Read comments + readComments(matchedSheet); + } catch (ExcelAnalysisStopSheetException e) { + if (log.isDebugEnabled()) { + log.debug("Custom stop!", e); } - // The last sheet is read - xlsxReadContext.analysisEventProcessor().endSheet(xlsxReadContext); } + // The last sheet is read + xlsxReadContext.analysisEventProcessor().endSheet(xlsxReadContext); } - } finally { - closeRemainingSheetStreams(); } } + @Override + public void close() { + closeRemainingSheetStreams(); + } + private void closeRemainingSheetStreams() { for (Map.Entry entry : sheetMap.entrySet()) { closeSheetInputStream(entry.getValue(), "sheetNo=" + entry.getKey()); diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java index 7a58d6e73..45a0e0584 100644 --- a/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java @@ -54,9 +54,10 @@ class XlsxSaxAnalyserSheetStreamCloseTest extends AbstractExcelTest { @Test - void execute_closesUnreadSheetStreams() throws Exception { + void execute_keepsUnreadSheetStreamsOpen_untilReaderCloses() throws Exception { File file = writeThreeSheets(); CollectingReadListener listener = new CollectingReadListener(); + Map tracked = null; try (ExcelReader excelReader = FesodSheet.read(file, SimpleData.class, listener).build()) { @@ -64,13 +65,23 @@ void execute_closesUnreadSheetStreams() throws Exception { Map sheetMap = sheetMap(analyser); Assertions.assertEquals(3, sheetMap.size()); - Map tracked = wrapSheetStreams(sheetMap); + tracked = wrapSheetStreams(sheetMap); excelReader.read(FesodSheet.readSheet(0).build()); Assertions.assertEquals(1, listener.getRowCount()); Assertions.assertEquals("sheet1", listener.getFirstRow().getName()); - assertAllClosed(tracked); + Assertions.assertTrue(tracked.get(0).isClosed()); + Assertions.assertFalse(tracked.get(1).isClosed()); + Assertions.assertFalse(tracked.get(2).isClosed()); + + excelReader.read(FesodSheet.readSheet(1).build()); + Assertions.assertEquals(2, listener.getRowCount()); + Assertions.assertTrue(tracked.get(1).isClosed()); + Assertions.assertFalse(tracked.get(2).isClosed()); } + + Assertions.assertNotNull(tracked); + assertAllClosed(tracked); } @Test @@ -96,6 +107,24 @@ public void doAfterAllAnalysed(AnalysisContext context) {} } } + @Test + void sequentialSheetReads_doNotCloseLaterSheetsEarly() throws Exception { + File file = writeThreeSheets(); + CollectingReadListener listener = new CollectingReadListener(); + + try (ExcelReader excelReader = + FesodSheet.read(file, SimpleData.class, listener).build()) { + excelReader.read(FesodSheet.readSheet(0).build()); + excelReader.read(FesodSheet.readSheet(1).build()); + excelReader.read(FesodSheet.readSheet(2).build()); + } + + Assertions.assertEquals(3, listener.getRowCount()); + Assertions.assertEquals("sheet1", listener.getRows().get(0).getName()); + Assertions.assertEquals("sheet2", listener.getRows().get(1).getName()); + Assertions.assertEquals("sheet3", listener.getRows().get(2).getName()); + } + @Test void constructor_doesNotRetainHiddenSheetStreams_whenIgnoreHiddenSheet() throws Exception { File file = writeWorkbookWithHiddenSheet(); From 38e04648a0162b8283884d2a33eb15bc714bde28 Mon Sep 17 00:00:00 2001 From: lance Date: Tue, 8 Sep 2026 20:40:28 +0800 Subject: [PATCH 3/3] fix close unused xlsx sheet and workbook XML streams Signed-off-by: lance --- .../sheet/analysis/v07/XlsxSaxAnalyser.java | 5 +- .../selector/SimpleReadCacheSelector.java | 5 +- .../XlsxSaxAnalyserSheetStreamCloseTest.java | 79 +++++++++++++++-- .../selector/SimpleReadCacheSelectorTest.java | 86 +++++++++++++++++++ 4 files changed, 163 insertions(+), 12 deletions(-) create mode 100644 fesod-sheet/src/test/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelectorTest.java diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java index 90dde6dda..4bdb30547 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java @@ -345,7 +345,10 @@ private void closeRemainingSheetStreams() { } } - private void closeSheetInputStream(InputStream inputStream, String description) { + /** + * Package-private so tests can assert skipped-sheet streams are closed. + */ + void closeSheetInputStream(InputStream inputStream, String description) { if (inputStream == null) { return; } diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelector.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelector.java index 6084aa32b..a170587ac 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelector.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelector.java @@ -26,6 +26,7 @@ package org.apache.fesod.sheet.cache.selector; import java.io.IOException; +import java.io.InputStream; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.Setter; @@ -98,8 +99,8 @@ public SimpleReadCacheSelector(Long maxUseMapCacheSize, Integer maxCacheActivate public ReadCache readCache(PackagePart sharedStringsTablePackagePart) { long size = sharedStringsTablePackagePart.getSize(); if (size < 0) { - try { - size = sharedStringsTablePackagePart.getInputStream().available(); + try (InputStream inputStream = sharedStringsTablePackagePart.getInputStream()) { + size = inputStream.available(); } catch (IOException e) { log.warn("Unable to get file size, default used MapCache"); return new MapCache(); diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java index 45a0e0584..7e8e2093e 100644 --- a/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java @@ -25,6 +25,7 @@ import java.io.IOException; import java.io.InputStream; import java.lang.reflect.Field; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -32,14 +33,19 @@ import org.apache.fesod.sheet.ExcelWriter; import org.apache.fesod.sheet.FesodSheet; import org.apache.fesod.sheet.context.AnalysisContext; +import org.apache.fesod.sheet.context.xlsx.DefaultXlsxReadContext; +import org.apache.fesod.sheet.context.xlsx.XlsxReadContext; import org.apache.fesod.sheet.event.AnalysisEventListener; import org.apache.fesod.sheet.read.metadata.ReadSheet; +import org.apache.fesod.sheet.read.metadata.ReadWorkbook; +import org.apache.fesod.sheet.support.ExcelTypeEnum; import org.apache.fesod.sheet.testkit.Tags; import org.apache.fesod.sheet.testkit.base.AbstractExcelTest; import org.apache.fesod.sheet.testkit.builders.TestDataBuilder; import org.apache.fesod.sheet.testkit.enums.ExcelFormat; import org.apache.fesod.sheet.testkit.listeners.CollectingReadListener; import org.apache.fesod.sheet.testkit.models.SimpleData; +import org.apache.fesod.sheet.util.FileUtils; import org.apache.fesod.sheet.write.metadata.WriteSheet; import org.apache.poi.xssf.usermodel.XSSFWorkbook; import org.junit.jupiter.api.Assertions; @@ -56,7 +62,7 @@ class XlsxSaxAnalyserSheetStreamCloseTest extends AbstractExcelTest { @Test void execute_keepsUnreadSheetStreamsOpen_untilReaderCloses() throws Exception { File file = writeThreeSheets(); - CollectingReadListener listener = new CollectingReadListener(); + CollectingReadListener listener = new CollectingReadListener<>(); Map tracked = null; try (ExcelReader excelReader = @@ -94,7 +100,9 @@ public void invoke(SimpleData data, AnalysisContext context) { } @Override - public void doAfterAllAnalysed(AnalysisContext context) {} + public void doAfterAllAnalysed(AnalysisContext context) { + // ignore code + } }; try (ExcelReader excelReader = @@ -108,9 +116,9 @@ public void doAfterAllAnalysed(AnalysisContext context) {} } @Test - void sequentialSheetReads_doNotCloseLaterSheetsEarly() throws Exception { + void sequentialSheetReads_doNotCloseLaterSheetsEarly() { File file = writeThreeSheets(); - CollectingReadListener listener = new CollectingReadListener(); + CollectingReadListener listener = new CollectingReadListener<>(); try (ExcelReader excelReader = FesodSheet.read(file, SimpleData.class, listener).build()) { @@ -147,8 +155,40 @@ void constructor_doesNotRetainHiddenSheetStreams_whenIgnoreHiddenSheet() throws } } + @Test + void constructor_closesSkippedHiddenSheetStream_whenIgnoreHiddenSheet() throws Exception { + File file = writeWorkbookWithHiddenSheet(); + ReadWorkbook readWorkbook = new ReadWorkbook(); + readWorkbook.setFile(file); + readWorkbook.setIgnoreHiddenSheet(Boolean.TRUE); + XlsxReadContext context = new DefaultXlsxReadContext(readWorkbook, ExcelTypeEnum.XLSX); + TrackingXlsxSaxAnalyser analyser = new TrackingXlsxSaxAnalyser(context); + try { + Assertions.assertFalse(containsSheetName(analyser.sheetList(), "Hidden")); + boolean hiddenClosed = false; + for (String description : analyser.closedDescriptions()) { + if (description != null && description.contains("Hidden")) { + hiddenClosed = true; + break; + } + } + Assertions.assertTrue(hiddenClosed, "Skipped hidden sheet stream should be closed during construction"); + } finally { + analyser.close(); + if (context.xlsxReadWorkbookHolder().getOpcPackage() != null) { + context.xlsxReadWorkbookHolder().getOpcPackage().revert(); + } + if (context.xlsxReadWorkbookHolder().getReadCache() != null) { + context.xlsxReadWorkbookHolder().getReadCache().destroy(); + } + if (context.xlsxReadWorkbookHolder().getTempFile() != null) { + FileUtils.delete(context.xlsxReadWorkbookHolder().getTempFile()); + } + } + } + private File writeThreeSheets() { - File file = createTempFileUnchecked(ExcelFormat.XLSX); + File file = createTempFileUnchecked(); try (ExcelWriter excelWriter = FesodSheet.write(file, SimpleData.class).build()) { WriteSheet sheet1 = FesodSheet.writerSheet(0, "Sheet1").build(); WriteSheet sheet2 = FesodSheet.writerSheet(1, "Sheet2").build(); @@ -161,7 +201,7 @@ private File writeThreeSheets() { } private File writeWorkbookWithHiddenSheet() throws IOException { - File file = createTempFileUnchecked(ExcelFormat.XLSX); + File file = createTempFileUnchecked(); try (XSSFWorkbook workbook = new XSSFWorkbook()) { workbook.createSheet("Visible1").createRow(0).createCell(0).setCellValue("a"); workbook.createSheet("Hidden").createRow(0).createCell(0).setCellValue("b"); @@ -174,9 +214,9 @@ private File writeWorkbookWithHiddenSheet() throws IOException { return file; } - private File createTempFileUnchecked(ExcelFormat format) { + private File createTempFileUnchecked() { try { - return createTempFile(format); + return createTempFile(ExcelFormat.XLSX); } catch (IOException e) { throw new IllegalStateException(e); } @@ -205,7 +245,7 @@ private static Map sheetMap(XlsxSaxAnalyser analyser) thro } private static Map wrapSheetStreams(Map sheetMap) { - Map tracked = new HashMap(); + Map tracked = new HashMap<>(); for (Map.Entry entry : sheetMap.entrySet()) { CloseTrackingInputStream wrapped = new CloseTrackingInputStream(entry.getValue()); tracked.put(entry.getKey(), wrapped); @@ -239,4 +279,25 @@ private boolean isClosed() { return closed; } } + + private static final class TrackingXlsxSaxAnalyser extends XlsxSaxAnalyser { + private List closedDescriptions; + + private TrackingXlsxSaxAnalyser(XlsxReadContext xlsxReadContext) throws Exception { + super(xlsxReadContext, null); + } + + private List closedDescriptions() { + if (closedDescriptions == null) { + closedDescriptions = new ArrayList<>(); + } + return closedDescriptions; + } + + @Override + void closeSheetInputStream(InputStream inputStream, String description) { + closedDescriptions().add(description); + super.closeSheetInputStream(inputStream, description); + } + } } diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelectorTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelectorTest.java new file mode 100644 index 000000000..5667a5959 --- /dev/null +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelectorTest.java @@ -0,0 +1,86 @@ +/* + * 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.cache.selector; + +import java.io.ByteArrayInputStream; +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; +import org.apache.fesod.sheet.cache.MapCache; +import org.apache.fesod.sheet.cache.ReadCache; +import org.apache.fesod.sheet.testkit.Tags; +import org.apache.poi.openxml4j.opc.PackagePart; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +/** + * Tests {@link SimpleReadCacheSelector}. + */ +@Tag(Tags.UNIT) +@ExtendWith(MockitoExtension.class) +class SimpleReadCacheSelectorTest { + + @Test + void readCache_closesInputStream_whenPackagePartSizeUnknown() throws Exception { + PackagePart packagePart = Mockito.mock(PackagePart.class); + CloseTrackingInputStream inputStream = + new CloseTrackingInputStream(new ByteArrayInputStream(new byte[] {1, 2, 3, 4})); + Mockito.when(packagePart.getSize()).thenReturn(-1L); + Mockito.when(packagePart.getInputStream()).thenReturn(inputStream); + + ReadCache cache = new SimpleReadCacheSelector().readCache(packagePart); + + Assertions.assertTrue(inputStream.isClosed()); + Assertions.assertInstanceOf(MapCache.class, cache); + } + + @Test + void readCache_doesNotOpenInputStream_whenPackagePartSizeIsKnown() throws Exception { + PackagePart packagePart = Mockito.mock(PackagePart.class); + Mockito.when(packagePart.getSize()).thenReturn(1024L); + + ReadCache cache = new SimpleReadCacheSelector().readCache(packagePart); + + Mockito.verify(packagePart, Mockito.never()).getInputStream(); + Assertions.assertInstanceOf(MapCache.class, cache); + } + + private static final class CloseTrackingInputStream extends FilterInputStream { + private boolean closed; + + private CloseTrackingInputStream(InputStream in) { + super(in); + } + + @Override + public void close() throws IOException { + closed = true; + super.close(); + } + + private boolean isClosed() { + return closed; + } + } +}