#60996 - XSSF: Multiple embedded objects on same sheet are ignored

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1791644 13f79535-47bb-0310-9956-ffa450edef68

6 files changed, 99 insertions, 39 deletions

diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFSimpleShape.java b/src/java/org/apache/poi/hssf/usermodel/HSSFSimpleShape.java
index 4e405cae46..1082362a44 100644
--- a/src/java/org/apache/poi/hssf/usermodel/HSSFSimpleShape.java
+++ b/src/java/org/apache/poi/hssf/usermodel/HSSFSimpleShape.java
@@ -263,4 +263,9 @@ public class HSSFSimpleShape extends HSSFShape implements SimpleShape
         }
         return _textObjectRecord;
     }
+
+    @Override
+    public int getShapeId(){
+        return super.getShapeId();
+    }
 }
diff --git a/src/java/org/apache/poi/ss/usermodel/SimpleShape.java b/src/java/org/apache/poi/ss/usermodel/SimpleShape.java
index 2d65fd8531..fe97c850f3 100644
--- a/src/java/org/apache/poi/ss/usermodel/SimpleShape.java
+++ b/src/java/org/apache/poi/ss/usermodel/SimpleShape.java
@@ -19,10 +19,14 @@ package org.apache.poi.ss.usermodel;
 
 /**
  * A common interface for simple shapes.
- * (Currently the HSSF and XSSF classes don't share common method signatures ...)
  * 
  * @since POI 3.16-beta2
  */
 public interface SimpleShape extends Shape {
-
+    /**
+     * @return the shape id, which is unique within the sheet
+     * 
+     * @since POI 3.17-beta1
+     */
+    int getShapeId();
 }
diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFDrawing.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFDrawing.java
index 35f71381c3..42518fa9f5 100644
--- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFDrawing.java
+++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFDrawing.java
@@ -385,7 +385,24 @@ public final class XSSFDrawing extends POIXMLDocumentPart implements Drawing<XSS
     public XSSFObjectData createObjectData(ClientAnchor anchor, int storageId, int pictureIndex) {
         XSSFSheet sh = getSheet();
         PackagePart sheetPart = sh.getPackagePart();
-        long shapeId = newShapeId();
+
+        /*
+         * The shape id of the ole object seems to be a legacy shape id.
+         * 
+         * see 5.3.2.1 legacyDrawing (Legacy Drawing Object):
+         * Legacy Shape ID that is unique throughout the entire document.
+         * Legacy shape IDs should be assigned based on which portion of the document the
+         * drawing resides on. The assignment of these ids is broken down into clusters of
+         * 1024 values. The first cluster is 1-1024, the second 1025-2048 and so on.
+         *
+         * Ole shapes seem to start with 1025 on the first sheet ...
+         * and not sure, if the ids need to be reindexed when sheets are removed
+         * or more than 1024 shapes are on a given sheet (see #51332 for a similar issue)
+         */
+        XSSFSheet sheet = getSheet();
+        XSSFWorkbook wb = sheet.getWorkbook();
+        int sheetIndex = wb.getSheetIndex(sheet);
+        long shapeId = (sheetIndex+1)*1024 + newShapeId();
 
         // add reference to OLE part
         PackagePartName olePN;
@@ -446,6 +463,7 @@ public final class XSSFDrawing extends POIXMLDocumentPart implements Drawing<XSS
         
         CTNonVisualDrawingProps cNvPr = ctShape.getNvSpPr().getCNvPr();
         cNvPr.setId(shapeId);
+        cNvPr.setName("Object "+shapeId);
 
         XmlCursor extCur = cNvPr.getExtLst().getExtArray(0).newCursor();
         extCur.toFirstChild();
@@ -520,7 +538,10 @@ public final class XSSFDrawing extends POIXMLDocumentPart implements Drawing<XSS
     }
 
     private long newShapeId(){
-        return drawing.sizeOfTwoCellAnchorArray() + 1;
+        return 1+
+            drawing.sizeOfAbsoluteAnchorArray()+
+            drawing.sizeOfOneCellAnchorArray()+
+            drawing.sizeOfTwoCellAnchorArray();
     }
 
     /**
diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFObjectData.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFObjectData.java
index 2221979411..b5df89f641 100644
--- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFObjectData.java
+++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFObjectData.java
@@ -210,7 +210,7 @@ public class XSSFObjectData extends XSSFSimpleShape implements ObjectData {
         try {
             if (cur.toChild(XSSFRelation.NS_SPREADSHEETML, "objectPr")) {
                 String blipId = cur.getAttributeText(new QName(PackageRelationshipTypes.CORE_PROPERTIES_ECMA376_NS, "id"));
-                return (XSSFPictureData)getDrawing().getRelationById(blipId);
+                return (XSSFPictureData)getSheet().getRelationById(blipId);
             }
             return null;
         } finally {
diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSimpleShape.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSimpleShape.java
index 1f82167654..a3e78dac32 100644
--- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSimpleShape.java
+++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSimpleShape.java
@@ -869,4 +869,9 @@ public class XSSFSimpleShape extends XSSFShape implements Iterable<XSSFTextParag
     public String getShapeName() {
         return ctShape.getNvSpPr().getCNvPr().getName();
     }
+
+    @Override
+    public int getShapeId() {
+        return (int)ctShape.getNvSpPr().getCNvPr().getId();
+    }
 }
diff --git a/src/ooxml/testcases/org/apache/poi/ss/usermodel/TestEmbedOLEPackage.java b/src/ooxml/testcases/org/apache/poi/ss/usermodel/TestEmbedOLEPackage.java
index b17d5d32d4..71496c7efb 100644
--- a/src/ooxml/testcases/org/apache/poi/ss/usermodel/TestEmbedOLEPackage.java
+++ b/src/ooxml/testcases/org/apache/poi/ss/usermodel/TestEmbedOLEPackage.java
@@ -18,15 +18,20 @@
 package org.apache.poi.ss.usermodel;
 
 import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeTrue;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
 
 import org.apache.poi.POIDataSamples;
 import org.apache.poi.hssf.HSSFTestDataSamples;
 import org.apache.poi.hssf.usermodel.HSSFWorkbook;
+import org.apache.poi.poifs.filesystem.DirectoryNode;
 import org.apache.poi.sl.usermodel.AutoShape;
 import org.apache.poi.sl.usermodel.ShapeType;
 import org.apache.poi.sl.usermodel.Slide;
@@ -36,29 +41,26 @@ import org.apache.poi.ss.extractor.EmbeddedExtractor;
 import org.apache.poi.xslf.usermodel.XMLSlideShow;
 import org.apache.poi.xssf.XSSFTestDataSamples;
 import org.apache.poi.xssf.usermodel.XSSFWorkbook;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
 public class TestEmbedOLEPackage {
+    private static byte[] samplePPT, samplePPTX, samplePNG;
+    
+    @BeforeClass
+    public static void init() throws IOException {
+        samplePPT = getSamplePPT(false);
+        samplePPTX = getSamplePPT(true);
+        samplePNG = POIDataSamples.getSpreadSheetInstance().readFile("logoKarmokar4.png");
+    }
+    
     @Test
     public void embedXSSF() throws IOException {
         Workbook wb1 = new XSSFWorkbook();
-        Sheet sh = wb1.createSheet();
-        int picIdx = wb1.addPicture(getSamplePng(), Workbook.PICTURE_TYPE_PNG);
-        byte samplePPTX[] = getSamplePPT(true);
-        int oleIdx = wb1.addOlePackage(samplePPTX, "dummy.pptx", "dummy.pptx", "dummy.pptx");
-
-        Drawing<?> pat = sh.createDrawingPatriarch();
-        ClientAnchor anchor = pat.createAnchor(0, 0, 0, 0, 1, 1, 3, 6);
-        pat.createObjectData(anchor, oleIdx, picIdx);
+        addEmbeddedObjects(wb1);
 
         Workbook wb2 = XSSFTestDataSamples.writeOutAndReadBack(wb1);
-
-        pat = wb2.getSheetAt(0).getDrawingPatriarch();
-        assertTrue(pat.iterator().next() instanceof ObjectData);
-
-        EmbeddedExtractor ee = new EmbeddedExtractor();
-        EmbeddedData ed = ee.extractAll(wb2.getSheetAt(0)).get(0);
-        assertArrayEquals(samplePPTX, ed.getEmbeddedData());
+        validateEmbeddedObjects(wb2);
 
         wb2.close();
         wb1.close();
@@ -73,32 +75,55 @@ public class TestEmbedOLEPackage {
         }
 
         Workbook wb1 = new HSSFWorkbook();
-        Sheet sh = wb1.createSheet();
-        int picIdx = wb1.addPicture(getSamplePng(), Workbook.PICTURE_TYPE_PNG);
-        byte samplePPT[] = getSamplePPT(false);
-        int oleIdx = wb1.addOlePackage(samplePPT, "dummy.ppt", "dummy.ppt", "dummy.ppt");
-
-        Drawing<?> pat = sh.createDrawingPatriarch();
-        ClientAnchor anchor = pat.createAnchor(0, 0, 0, 0, 1, 1, 3, 6);
-        pat.createObjectData(anchor, oleIdx, picIdx);
-
+        addEmbeddedObjects(wb1);
         Workbook wb2 = HSSFTestDataSamples.writeOutAndReadBack((HSSFWorkbook)wb1);
-
-        pat = wb2.getSheetAt(0).getDrawingPatriarch();
-        assertTrue(pat.iterator().next() instanceof ObjectData);
-
-        EmbeddedExtractor ee = new EmbeddedExtractor();
-        EmbeddedData ed = ee.extractAll(wb2.getSheetAt(0)).get(0);
-        assertArrayEquals(samplePPT, ed.getEmbeddedData());
+        validateEmbeddedObjects(wb2);
 
         wb2.close();
         wb1.close();
     }
 
-    static byte[] getSamplePng() {
-        return POIDataSamples.getSpreadSheetInstance().readFile("logoKarmokar4.png");
+    static void validateEmbeddedObjects(Workbook wb) throws IOException {
+        boolean ooxml = wb.getClass().getName().toLowerCase().contains("xssf");
+        byte data[] = (ooxml) ? samplePPTX : samplePPT;
+        Iterator<Integer> shapeIds = Arrays.asList(1025,1026,2049).iterator();
+        EmbeddedExtractor ee = new EmbeddedExtractor();
+        for (Sheet sheet : wb) {
+            Drawing<? extends Shape> pat = sheet.getDrawingPatriarch();
+            for (Shape shape : pat) {
+                assertTrue(shape instanceof ObjectData);
+                ObjectData od = (ObjectData)shape;
+                EmbeddedData ed = ee.extractOne((DirectoryNode)od.getDirectory());
+                assertArrayEquals(data, ed.getEmbeddedData());
+                assertArrayEquals(samplePNG, od.getPictureData().getData());
+                assertEquals((int)shapeIds.next(), od.getShapeId());
+            }
+        }
     }
-
+    
+    static void addEmbeddedObjects(Workbook wb) throws IOException {
+        boolean ooxml = wb.getClass().getName().toLowerCase().contains("xssf");
+        int picIdx = wb.addPicture(samplePNG, Workbook.PICTURE_TYPE_PNG);
+        byte data[] = (ooxml) ? samplePPTX : samplePPT;
+        String ext = (ooxml) ? ".pptx" : ".ppt";
+        
+        int oleIdx1a = wb.addOlePackage(data, "dummy1a"+ext, "dummy1a"+ext, "dummy1a"+ext);
+        int oleIdx1b = wb.addOlePackage(data, "dummy1b"+ext, "dummy1b"+ext, "dummy1b"+ext);
+        int oleIdx2 = wb.addOlePackage(data, "dummy2"+ext, "dummy2"+ext, "dummy2"+ext);
+        
+        Sheet sh1 = wb.createSheet();
+        Drawing<?> pat1 = sh1.createDrawingPatriarch();
+        ClientAnchor anchor1a = pat1.createAnchor(0, 0, 0, 0, 1, 1, 3, 6);
+        pat1.createObjectData(anchor1a, oleIdx1a, picIdx);
+        ClientAnchor anchor1b = pat1.createAnchor(0, 0, 0, 0, 1, 1+7, 3, 6+7);
+        pat1.createObjectData(anchor1b, oleIdx1b, picIdx);
+
+        Sheet sh2 = wb.createSheet();
+        Drawing<?> pat2 = sh2.createDrawingPatriarch();
+        ClientAnchor anchor2 = pat2.createAnchor(0, 0, 0, 0, 1, 1, 3, 6);
+        pat2.createObjectData(anchor2, oleIdx2, picIdx);
+    }
+    
     static byte[] getSamplePPT(boolean ooxml) throws IOException {
         SlideShow<?,?> ppt = (ooxml) ? new XMLSlideShow() : new org.apache.poi.hslf.usermodel.HSLFSlideShow();
         Slide<?,?> slide = ppt.createSlide();