try to make lists returned unmodifiable to avoid external manipulation of inner data (#863)

* try to make lists returned unmodifiable to avoid external manipulation of inner data

* ExternalLinksTable list needs to be mutable

* pivot tables need to be modifiable

* refactor

* more

* test issue

poi-ooxml/build.gradle

@@ -58,6 +58,12 @@ dependencies {
     api project(':poi-ooxml-full')
     api project(path: ':poi-ooxml-full', configuration: 'archives')
 
+    // Can be very useful in local testing to comment out the 2 poi-ooxml-full lines above
+    // and uncomment the line below to use a pre-built version of poi-ooxml-full.
+    // Try to use the last release version of poi-ooxml-full. You might be unlucky if
+    // recent unreleased changes in poi-ooxml-full are needed.
+    // api "org.apache.poi:poi-ooxml-full:5.4.1"
+
     api "org.apache.xmlbeans:xmlbeans:${xmlbeansVersion}"
     api "org.apache.commons:commons-compress:${commonsCompressVersion}"
     api "commons-io:commons-io:${commonsIoVersion}"

poi-ooxml/src/main/java/org/apache/poi/xddf/usermodel/chart/XDDFChart.java

@@ -688,7 +688,7 @@ public abstract class XDDFChart extends POIXMLDocumentPart implements TextContai
         if (axes.isEmpty() && hasAxes()) {
             parseAxes();
         }
-        return axes;
+        return Collections.unmodifiableList(axes);
     }
 
     private boolean hasAxes() {

poi-ooxml/src/main/java/org/apache/poi/xddf/usermodel/text/XDDFTextParagraph.java

@@ -113,7 +113,7 @@ public class XDDFTextParagraph implements Iterable<XDDFTextRun> {
     }
 
     public List<XDDFTextRun> getTextRuns() {
-        return _runs;
+        return Collections.unmodifiableList(_runs);
     }
 
     @Override

poi-ooxml/src/main/java/org/apache/poi/xdgf/usermodel/XDGFShape.java

@@ -27,6 +27,7 @@ import java.awt.geom.Path2D;
 import java.awt.geom.Rectangle2D;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map.Entry;
 
@@ -307,7 +308,7 @@ public class XDGFShape extends XDGFSheet {
      */
     // -> May be null
     public List<XDGFShape> getShapes() {
-        return _shapes;
+        return _shapes == null ? null : Collections.unmodifiableList(_shapes);
     }
 
     // unique to this shape on the page?

poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/BaseXSSFEvaluationWorkbook.java

@@ -109,7 +109,7 @@ public abstract class BaseXSSFEvaluationWorkbook implements FormulaRenderingWork
         } catch (NumberFormatException e) {}
 
         // Look up an External Link Table for this name
-        List<ExternalLinksTable> tables = _uBook.getExternalLinksTable();
+        List<ExternalLinksTable> tables = _uBook.getExternalLinksTables();
         int index = findExternalLinkIndex(bookName, tables);
         if (index != -1) return index;
 
@@ -126,7 +126,7 @@ public abstract class BaseXSSFEvaluationWorkbook implements FormulaRenderingWork
             // So, add the missing reference and return
             // Note - this is really rather nasty...
             ExternalLinksTable fakeLinkTable = new FakeExternalLinksTable(relBookName);
-            tables.add(fakeLinkTable);
+            _uBook.addExternalLinksTable(fakeLinkTable);
             return tables.size(); // 1 based results, 0 = current workbook
         }
 
@@ -193,7 +193,7 @@ public abstract class BaseXSSFEvaluationWorkbook implements FormulaRenderingWork
         if (externalWorkbookNumber > 0) {
             // External reference - reference is 1 based, link table is 0 based
             int linkNumber = externalWorkbookNumber - 1;
-            ExternalLinksTable linkTable = _uBook.getExternalLinksTable().get(linkNumber);
+            ExternalLinksTable linkTable = _uBook.getExternalLinksTable(linkNumber);
 
             for (org.apache.poi.ss.usermodel.Name name : linkTable.getDefinedNames()) {
                 if (name.getNameName().equals(nameName)) {
@@ -300,7 +300,7 @@ public abstract class BaseXSSFEvaluationWorkbook implements FormulaRenderingWork
         if (externalWorkbookNumber > 0) {
             // External reference - reference is 1 based, link table is 0 based
             int linkNumber = externalWorkbookNumber - 1;
-            ExternalLinksTable linkTable = _uBook.getExternalLinksTable().get(linkNumber);
+            ExternalLinksTable linkTable = _uBook.getExternalLinksTable(linkNumber);
             workbookName = linkTable.getLinkedFileName();
         } else {
             // Internal reference

poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/BaseXSSFFormulaEvaluator.java

@@ -128,7 +128,7 @@ public abstract class BaseXSSFFormulaEvaluator extends BaseFormulaEvaluator {
         XSSFWorkbook xssfWorkbook = xssfCell.getSheet().getWorkbook();
         XSSFWorkbook externalWorkbook = (XSSFWorkbook) xssfWorkbook.getCreationHelper()
                 .getReferencedWorkbooks().get(externalSheet.getWorkbookName());
-        ExternalLinksTable externalLinksTable = xssfWorkbook.getExternalLinksTable().get(area3DPxg.getExternalWorkbookNumber() - 1);
+        ExternalLinksTable externalLinksTable = xssfWorkbook.getExternalLinksTable(area3DPxg.getExternalWorkbookNumber() - 1);
 
         if (externalWorkbook != null && externalLinksTable != null) {
             int firstSheet = externalWorkbook.getSheetIndex(area3DPxg.getSheetName());

poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFSheet.java

@@ -172,7 +172,7 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet, OoxmlSheetEx
                 tables.put( rp.getRelationship().getId(), (XSSFTable)p );
             }
             if(p instanceof XSSFPivotTable) {
-                getWorkbook().getPivotTables().add((XSSFPivotTable) p);
+                getWorkbook().addPivotTable((XSSFPivotTable) p);
             }
         }
 
@@ -4705,13 +4705,12 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet, OoxmlSheetEx
     @Beta
     private XSSFPivotTable createPivotTable() {
         XSSFWorkbook wb = getWorkbook();
-        List<XSSFPivotTable> pivotTables = wb.getPivotTables();
         int tableId = getWorkbook().getPivotTables().size()+1;
         //Create relationship between pivotTable and the worksheet
         XSSFPivotTable pivotTable = (XSSFPivotTable) createRelationship(XSSFRelation.PIVOT_TABLE,
                 getWorkbook().getXssfFactory(), tableId);
         pivotTable.setParentSheet(this);
-        pivotTables.add(pivotTable);
+        wb.addPivotTable(pivotTable);
         XSSFWorkbook workbook = getWorkbook();
 
         //Create relationship between the pivot cache definition and the workbook
@@ -4735,8 +4734,6 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet, OoxmlSheetEx
         //Set relationships id for pivotCacheDefinition to pivotCacheRecords
         pivotTable.getPivotCacheDefinition().getCTPivotCacheDefinition().setId(pivotCacheDefinition.getRelationId(pivotCacheRecords));
 
-        wb.setPivotTables(pivotTables);
-
         return pivotTable;
     }
 

poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFTextParagraph.java

@@ -19,6 +19,7 @@ package org.apache.poi.xssf.usermodel;
 
 import java.awt.Color;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 
@@ -84,7 +85,7 @@ public class XSSFTextParagraph implements Iterable<XSSFTextRun>{
     }
 
     public List<XSSFTextRun> getTextRuns(){
-        return _runs;
+        return Collections.unmodifiableList(_runs);
     }
 
     public Iterator<XSSFTextRun> iterator(){

poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java

@@ -205,7 +205,7 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su
     /**
      * List of all pivot tables in workbook
      */
-    private List<XSSFPivotTable> pivotTables;
+    private final List<XSSFPivotTable> pivotTables = new ArrayList<>();
     private List<CTPivotCache> pivotCaches;
 
     private final XSSFFactory xssfFactory;
@@ -386,7 +386,6 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su
         }
 
         // Create arrays for parts attached to the workbook itself
-        pivotTables = new ArrayList<>();
         pivotCaches = new ArrayList<>();
     }
 
@@ -524,7 +523,6 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su
         namedRanges = new ArrayList<>();
         namedRangesByName = new ArrayListValuedHashMap<>();
         sheets = new ArrayList<>();
-        pivotTables = new ArrayList<>();
 
         externalLinks = new ArrayList<>();
     }
@@ -1042,14 +1040,14 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su
      */
     @Override
     public List<XSSFPictureData> getAllPictures() {
-        if(pictures == null){
+        if (pictures == null) {
             List<PackagePart> mediaParts = getPackage().getPartsByName(GET_ALL_PICTURES_PATTERN);
             pictures = new ArrayList<>(mediaParts.size());
             for(PackagePart part : mediaParts){
                 pictures.add(new XSSFPictureData(part));
             }
         }
-        return pictures; //YK: should return Collections.unmodifiableList(pictures);
+        return Collections.unmodifiableList(pictures);
     }
 
     /**
@@ -2079,10 +2077,45 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su
      * @return the {@code ExternalLinksTable} list, which may be empty
      */
     @Internal
-    public List<ExternalLinksTable> getExternalLinksTable() {
+    public List<ExternalLinksTable> getExternalLinksTables() {
+        // needs to be the live copy because unfortunately we don't have APIs to
+        // add and remove external links (this method is annotated @Internal)
         return externalLinks;
     }
 
+    @Deprecated // use getExternalLinksTables() instead
+    @Removal(version = "7.0.0")
+    @Internal
+    public List<ExternalLinksTable> getExternalLinksTable() {
+        // needs to be the live copy because unfortunately we don't have APIs to
+        // add and remove external links (this method is annotated @Internal)
+        return externalLinks;
+    }
+
+    /**
+     * Adds an External Links Table to the workbook.
+     *
+     * @param externalLinksTable the External Links Table to add
+     * @since POI 5.4.2
+     */
+    @Internal
+    public void addExternalLinksTable(ExternalLinksTable externalLinksTable) {
+        if (externalLinks == null) {
+            externalLinks = new ArrayList<>();
+        }
+        externalLinks.add(externalLinksTable);
+    }
+
+    /**
+     * @param index the index at which to add the External Links Table
+     * @return  externalLinksTable the External Links Table to add
+     * @since POI 5.4.2
+     */
+    @Internal
+    public ExternalLinksTable getExternalLinksTable(int index) {
+        return externalLinks == null ? null : externalLinks.get(index);
+    }
+
     /**
      *
      * @return a collection of custom XML mappings defined in this workbook
@@ -2125,7 +2158,7 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su
             POIXMLDocumentPart.RelationPart rp = this.createRelationship(XSSFRelation.EXTERNAL_LINKS, xssfFactory, externalLinkIdx, false);
             ExternalLinksTable linksTable = rp.getDocumentPart();
             linksTable.setLinkedFileName(name);
-            this.getExternalLinksTable().add(linksTable);
+            this.addExternalLinksTable(linksTable);
 
             CTExternalReference ctExternalReference = this.getCTWorkbook().addNewExternalReferences().addNewExternalReference();
             ctExternalReference.setId(rp.getRelationship().getId());
@@ -2385,7 +2418,7 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su
         }
         CTPivotCache cache = caches.addNewPivotCache();
 
-        int tableId = getPivotTables().size()+1;
+        final int tableId = pivotTables.size() + 1;
         cache.setCacheId(tableId);
         cache.setId(rId);
         if(pivotCaches == null) {
@@ -2397,12 +2430,14 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su
 
     @Beta
     public List<XSSFPivotTable> getPivotTables() {
+        // needs to be the live copy because unfortunately we don't have APIs to
+        // add and remove external links (this method is annotated @Beta)
         return pivotTables;
     }
 
-    @Beta
-    protected void setPivotTables(List<XSSFPivotTable> pivotTables) {
-        this.pivotTables = pivotTables;
+    @Internal
+    public void addPivotTable(XSSFPivotTable pivotTable) {
+        pivotTables.add(pivotTable);
     }
 
     public XSSFWorkbookType getWorkbookType() {

poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFAbstractFootnoteEndnote.java

@@ -18,6 +18,7 @@ package org.apache.poi.xwpf.usermodel;
 
 import java.math.BigInteger;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Spliterator;
@@ -101,7 +102,7 @@ public abstract class XWPFAbstractFootnoteEndnote  implements Iterable<XWPFParag
      */
     @Override
     public List<XWPFParagraph> getParagraphs() {
-        return paragraphs;
+        return Collections.unmodifiableList(paragraphs);
     }
 
     /**
@@ -130,7 +131,7 @@ public abstract class XWPFAbstractFootnoteEndnote  implements Iterable<XWPFParag
      */
     @Override
     public List<XWPFTable> getTables() {
-        return tables;
+        return Collections.unmodifiableList(tables);
     }
 
     /**
@@ -138,7 +139,7 @@ public abstract class XWPFAbstractFootnoteEndnote  implements Iterable<XWPFParag
      * @return List of pictures
      */
     public List<XWPFPictureData> getPictures() {
-        return pictures;
+        return Collections.unmodifiableList(pictures);
     }
 
     /**
@@ -147,7 +148,7 @@ public abstract class XWPFAbstractFootnoteEndnote  implements Iterable<XWPFParag
      */
     @Override
     public List<IBodyElement> getBodyElements() {
-        return bodyElements;
+        return Collections.unmodifiableList(bodyElements);
     }
 
     /**

poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFComments.java

@@ -233,7 +233,7 @@ public class XWPFComments extends POIXMLDocumentPart {
      * Get the list of {@link XWPFComment} in the Comments part.
      */
     public List<XWPFComment> getComments() {
-        return comments;
+        return Collections.unmodifiableList(comments);
     }
 
     /**

poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFHeaderFooter.java

@@ -210,7 +210,7 @@ public abstract class XWPFHeaderFooter extends POIXMLDocumentPart implements IBo
      * @return a list of {@link XWPFParagraph}
      */
     public List<XWPFParagraph> getListParagraph() {
-        return paragraphs;
+        return Collections.unmodifiableList(paragraphs);
     }
 
     public List<XWPFPictureData> getAllPictures() {

poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFTableRow.java

@@ -18,6 +18,7 @@ package org.apache.poi.xwpf.usermodel;
 
 import java.math.BigInteger;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 
 import org.apache.poi.ooxml.util.POIXMLUnits;
@@ -211,7 +212,7 @@ public class XWPFTableRow {
 
             this.tableCells = cells;
         }
-        return tableCells;
+        return Collections.unmodifiableList(tableCells);
     }
 
     /**

poi-ooxml/src/test/java/org/apache/poi/xssf/model/TestExternalLinksTable.java

@@ -32,18 +32,18 @@ public final class TestExternalLinksTable {
     @Test
     void none() throws IOException {
         try (XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("SampleSS.xlsx")) {
-            assertNotNull(wb.getExternalLinksTable());
-            assertEquals(0, wb.getExternalLinksTable().size());
+            assertNotNull(wb.getExternalLinksTables());
+            assertEquals(0, wb.getExternalLinksTables().size());
         }
     }
 
     @Test
     void basicRead() throws IOException {
         try (XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("ref-56737.xlsx")) {
-            assertNotNull(wb.getExternalLinksTable());
-            assertEquals(1, wb.getExternalLinksTable().size());
+            assertNotNull(wb.getExternalLinksTables());
+            assertEquals(1, wb.getExternalLinksTables().size());
 
-            ExternalLinksTable links = wb.getExternalLinksTable().get(0);
+            ExternalLinksTable links = wb.getExternalLinksTable(0);
             assertEquals(3, links.getSheetNames().size());
             assertEquals(2, links.getDefinedNames().size());
 
@@ -70,13 +70,13 @@ public final class TestExternalLinksTable {
     @Test
     void basicReadWriteRead() throws IOException {
         try (XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("ref-56737.xlsx")) {
-            Name name = wb.getExternalLinksTable().get(0).getDefinedNames().get(1);
+            Name name = wb.getExternalLinksTable(0).getDefinedNames().get(1);
             name.setNameName("Testing");
             name.setRefersToFormula("$A$1");
 
             XSSFWorkbook wbBack = XSSFTestDataSamples.writeOutAndReadBack(wb);
-            assertEquals(1, wbBack.getExternalLinksTable().size());
-            ExternalLinksTable links = wbBack.getExternalLinksTable().get(0);
+            assertEquals(1, wbBack.getExternalLinksTables().size());
+            ExternalLinksTable links = wbBack.getExternalLinksTable(0);
 
             name = links.getDefinedNames().get(0);
             assertEquals("NR_Global_B2", name.getNameName());
@@ -95,11 +95,11 @@ public final class TestExternalLinksTable {
     @Test
     void readWithReferencesToTwoExternalBooks() throws IOException {
         try (XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("ref2-56737.xlsx")) {
-            assertNotNull(wb.getExternalLinksTable());
-            assertEquals(2, wb.getExternalLinksTable().size());
+            assertNotNull(wb.getExternalLinksTables());
+            assertEquals(2, wb.getExternalLinksTables().size());
 
             // Check the first one, links to 56737.xlsx
-            ExternalLinksTable links = wb.getExternalLinksTable().get(0);
+            ExternalLinksTable links = wb.getExternalLinksTable(0);
             assertEquals("56737.xlsx", links.getLinkedFileName());
             assertEquals(3, links.getSheetNames().size());
             assertEquals(2, links.getDefinedNames().size());
@@ -122,7 +122,7 @@ public final class TestExternalLinksTable {
 
 
             // Check the second one, links to 56737.xls, slightly differently
-            links = wb.getExternalLinksTable().get(1);
+            links = wb.getExternalLinksTable(1);
             assertEquals("56737.xls", links.getLinkedFileName());
             assertEquals(2, links.getSheetNames().size());
             assertEquals(2, links.getDefinedNames().size());

poi-ooxml/src/test/java/org/apache/poi/xssf/usermodel/TestXSSFPictureData.java

@@ -35,7 +35,7 @@ public final class TestXSSFPictureData {
         XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("WithDrawing.xlsx");
         List<XSSFPictureData> pictures = wb.getAllPictures();
         //wb.getAllPictures() should return the same instance across multiple calls
-        assertSame(pictures, wb.getAllPictures());
+        assertEquals(pictures, wb.getAllPictures());
 
         assertEquals(5, pictures.size());
         String[] ext = {"jpeg", "emf", "png", "emf", "wmf"};

poi-ooxml/src/test/java/org/apache/poi/xssf/usermodel/TestXSSFWorkbook.java

@@ -1391,7 +1391,7 @@ public final class TestXSSFWorkbook extends BaseTestXWorkbook {
             try(
                     XSSFWorkbook workbook2 = new XSSFWorkbook(bosB.toInputStream())
             ) {
-                CTExternalLink link = workbook2.getExternalLinksTable().get(0).getCTExternalLink();
+                CTExternalLink link = workbook2.getExternalLinksTable(0).getCTExternalLink();
                 CTExternalSheetData sheetData = link.getExternalBook().getSheetDataSet().getSheetDataArray(0);
                 assertEquals(Double.valueOf(sheetData.getRowArray(0).getCellArray(0).getV()), v1);
                 assertEquals(Double.valueOf(sheetData.getRowArray(0).getCellArray(1).getV()), v2);