POI-55294 and 52186

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

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

@@ -1220,6 +1220,18 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet {
         worksheet.setColsArray(0,ctCols);
         setSheetFormatPrOutlineLevelCol();
     }
+    
+    /**
+     * Do not leave the width attribute undefined (see #52186).
+     */
+    private void setColWidthAttribute(CTCols ctCols) {
+        for (CTCol col : ctCols.getColList()) {
+        	if (!col.isSetWidth()) {
+        		col.setWidth(getDefaultColumnWidth());
+        		col.setCustomWidth(false);
+        	}
+        }
+    }
 
     /**
      * Tie a range of cell together so that they can be collapsed or expanded
@@ -1249,11 +1261,10 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet {
     }
 
 
-    @SuppressWarnings("deprecation") //YK: getXYZArray() array accessors are deprecated in xmlbeans with JDK 1.5 support
     private short getMaxOutlineLevelCols() {
         CTCols ctCols = worksheet.getColsArray(0);
         short outlineLevel = 0;
-        for (CTCol col : ctCols.getColArray()) {
+        for (CTCol col : ctCols.getColList()) {
             outlineLevel = col.getOutlineLevel() > outlineLevel ? col.getOutlineLevel() : outlineLevel;
         }
         return outlineLevel;
@@ -2682,6 +2693,8 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet {
             CTCols col = worksheet.getColsArray(0);
             if(col.sizeOfColArray() == 0) {
                 worksheet.setColsArray(null);
+            } else {
+            	setColWidthAttribute(col);
             }
         }
 

src/ooxml/java/org/apache/poi/xssf/usermodel/helpers/ColumnHelper.java

@@ -18,6 +18,8 @@
 package org.apache.poi.xssf.usermodel.helpers;
 
 import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.Map;
 
 import org.apache.poi.ss.usermodel.CellStyle;
 import org.apache.poi.xssf.util.CTColComparator;
@@ -112,6 +114,8 @@ public class ColumnHelper {
 
     public CTCols addCleanColIntoCols(CTCols cols, CTCol col) {
         boolean colOverlaps = false;
+        // a Map to remember overlapping columns
+        Map<Long, Boolean> overlappingCols = new LinkedHashMap<Long, Boolean>();
         int sizeOfColArray = cols.sizeOfColArray();
         for (int i = 0; i < sizeOfColArray; i++) {
             CTCol ithCol = cols.getColArray(i);
@@ -124,54 +128,65 @@ public class ColumnHelper {
             // different behavior required for each of the 4 different
             // overlapping types
             if (overlappingType == NumericRanges.OVERLAPS_1_MINOR) {
+            	// move the max border of the ithCol 
+            	// and insert a new column within the overlappingRange with merged column attributes
                 ithCol.setMax(overlappingRange[0] - 1);
-                CTCol rangeCol = insertCol(cols, overlappingRange[0],
+                insertCol(cols, overlappingRange[0],
                         overlappingRange[1], new CTCol[] { ithCol, col });
                 i++;
-                CTCol newCol = insertCol(cols, (overlappingRange[1] + 1), col
-                        .getMax(), new CTCol[] { col });
-                i++;
             } else if (overlappingType == NumericRanges.OVERLAPS_2_MINOR) {
+            	// move the min border of the ithCol 
+            	// and insert a new column within the overlappingRange with merged column attributes
                 ithCol.setMin(overlappingRange[1] + 1);
-                CTCol rangeCol = insertCol(cols, overlappingRange[0],
+                insertCol(cols, overlappingRange[0],
                         overlappingRange[1], new CTCol[] { ithCol, col });
                 i++;
-                CTCol newCol = insertCol(cols, col.getMin(),
-                        (overlappingRange[0] - 1), new CTCol[] { col });
-                i++;
             } else if (overlappingType == NumericRanges.OVERLAPS_2_WRAPS) {
+            	// merge column attributes, no new column is needed
                 setColumnAttributes(col, ithCol);
-                if (col.getMin() != ithCol.getMin()) {
-                    CTCol newColBefore = insertCol(cols, col.getMin(), (ithCol
-                            .getMin() - 1), new CTCol[] { col });
-                    i++;
-                }
-                if (col.getMax() != ithCol.getMax()) {
-                    CTCol newColAfter = insertCol(cols, (ithCol.getMax() + 1),
-                            col.getMax(), new CTCol[] { col });
-                    i++;
-                }
             } else if (overlappingType == NumericRanges.OVERLAPS_1_WRAPS) {
+            	// split the ithCol in three columns: before the overlappingRange, overlappingRange, and after the overlappingRange
+            	// before overlappingRange
                 if (col.getMin() != ithCol.getMin()) {
-                    CTCol newColBefore = insertCol(cols, ithCol.getMin(), (col
+                    insertCol(cols, ithCol.getMin(), (col
                             .getMin() - 1), new CTCol[] { ithCol });
                     i++;
                 }
+                // after the overlappingRange
                 if (col.getMax() != ithCol.getMax()) {
-                    CTCol newColAfter = insertCol(cols, (col.getMax() + 1),
+                    insertCol(cols, (col.getMax() + 1),
                             ithCol.getMax(), new CTCol[] { ithCol });
                     i++;
                 }
+                // within the overlappingRange
                 ithCol.setMin(overlappingRange[0]);
                 ithCol.setMax(overlappingRange[1]);
                 setColumnAttributes(col, ithCol);
             }
             if (overlappingType != NumericRanges.NO_OVERLAPS) {
                 colOverlaps = true;
+                // remember overlapped columns
+                for (long j = overlappingRange[0]; j <= overlappingRange[1]; j++) {
+                	overlappingCols.put(Long.valueOf(j), Boolean.TRUE);
+                }
             }
         }
         if (!colOverlaps) {
-            CTCol newCol = cloneCol(cols, col);
+            cloneCol(cols, col);
+        } else {
+            // insert new columns for ranges without overlaps
+        	long colMin = -1;
+        	for (long j = col.getMin(); j <= col.getMax(); j++) {
+        		if (!Boolean.TRUE.equals(overlappingCols.get(Long.valueOf(j)))) {
+        			if (colMin < 0) {
+        				colMin = j;
+        			}
+            		if ((j + 1) > col.getMax() || Boolean.TRUE.equals(overlappingCols.get(Long.valueOf(j + 1)))) {
+            			insertCol(cols, colMin, j, new CTCol[] { col });
+                    	colMin = -1;
+        			}
+        		}
+        	}
         }
         sortColumns(cols);
         return cols;

src/ooxml/testcases/org/apache/poi/xssf/XSSFTestDataSamples.java

@@ -19,6 +19,9 @@ package org.apache.poi.xssf;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 
@@ -29,11 +32,17 @@ import org.apache.poi.ss.usermodel.Workbook;
 import org.apache.poi.xssf.usermodel.XSSFWorkbook;
 
 /**
- * Centralises logic for finding/opening sample files in the src/testcases/org/apache/poi/hssf/hssf/data folder. 
+ * Centralises logic for finding/opening sample files in the test-data/spreadsheet folder. 
  * 
  * @author Josh Micich
  */
 public class XSSFTestDataSamples {
+   /**
+    * Used by {@link writeOutAndReadBack(R wb, String testName)}.  If a
+    * value is set for this in the System Properties, the xlsx file
+    * will be written out to that directory.
+    */
+   public static final String TEST_OUTPUT_DIR = "poi.test.xssf.output.dir";
 
    public static OPCPackage openSamplePackage(String sampleName) {
       try {
@@ -73,4 +82,42 @@ public class XSSFTestDataSamples {
 		R r = (R) result;
 		return r;
     }
+    
+    /**
+     * Writes the Workbook either into a file or into a byte array, depending on presence of 
+     * the system property {@value #TEST_OUTPUT_DIR}, and reads it in a new instance of the Workbook back.
+     * @param wb workbook to write
+     * @param testName file name to be used if writing into a file. The old file with the same name will be overridden.
+     * @return new instance read from the stream written by the wb parameter.
+     */
+    public static <R extends Workbook> R writeOutAndReadBack(R wb, String testName) {
+        XSSFWorkbook result = null;
+        if (System.getProperty(TEST_OUTPUT_DIR) != null) {
+            try {
+                File file = new File(System.getProperty(TEST_OUTPUT_DIR), testName + ".xlsx");
+                if (file.exists()) {
+                    file.delete();
+                }
+                FileOutputStream out = new FileOutputStream(file);
+                try {
+                    wb.write(out);
+                } finally {
+                    out.close();
+                }
+                FileInputStream in = new FileInputStream(file);
+                try {
+                    result = new XSSFWorkbook(in);
+                } finally {
+                    in.close();
+                }
+            } catch (IOException e) {
+                throw new RuntimeException(e);
+            }
+        } else {
+            return writeOutAndReadBack(wb);
+        }
+        @SuppressWarnings("unchecked")
+        R r = (R) result;
+        return r;
+    }
 }

src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFColGrouping.java

@@ -0,0 +1,276 @@
+/* ====================================================================
+   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.poi.xssf.usermodel;
+
+import junit.framework.TestCase;
+
+import org.apache.poi.util.POILogFactory;
+import org.apache.poi.util.POILogger;
+import org.apache.poi.xssf.XSSFTestDataSamples;
+import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTCol;
+import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTCols;
+
+/**
+ * Test asserts the POI produces <cols> element that could be read and properly interpreted by the MS Excel.
+ * For specification of the "cols" element see the chapter 3.3.1.16 of the "Office Open XML Part 4 - Markup Language Reference.pdf".
+ * The specification can be downloaded at http://www.ecma-international.org/publications/files/ECMA-ST/Office%20Open%20XML%201st%20edition%20Part%204%20(PDF).zip.
+ * 
+ * <p><em>
+ * The test saves xlsx file on a disk if the system property is set:
+ * -Dpoi.test.xssf.output.dir=${workspace_loc}/poi/build/xssf-output
+ * </em>
+ * 
+ */
+public class TestXSSFColGrouping extends TestCase {
+	
+	private static final POILogger logger = POILogFactory.getLogger(TestXSSFColGrouping.class);
+
+	
+    /**
+     * Tests that POI doesn't produce "col" elements without "width" attribute. 
+     * POI-52186
+     */
+    public void testNoColsWithoutWidthWhenGrouping() {
+		XSSFWorkbook wb = new XSSFWorkbook();
+		XSSFSheet sheet = wb.createSheet("test");
+
+        sheet.setColumnWidth(4, 5000);
+        sheet.setColumnWidth(5, 5000);
+       
+        sheet.groupColumn((short) 4, (short) 7);
+        sheet.groupColumn((short) 9, (short) 12);
+        
+        wb = XSSFTestDataSamples.writeOutAndReadBack(wb, "testNoColsWithoutWidthWhenGrouping");
+        sheet = wb.getSheet("test");
+        
+        CTCols cols = sheet.getCTWorksheet().getColsArray(0);
+        logger.log(POILogger.DEBUG, "test52186/cols:" + cols);
+        for (CTCol col : cols.getColList()) {
+			assertTrue("Col width attribute is unset: " + col.toString(), col.isSetWidth());
+		}
+    }
+
+    /**
+     * Tests that POI doesn't produce "col" elements without "width" attribute. 
+     * POI-52186
+     */
+    public void testNoColsWithoutWidthWhenGroupingAndCollapsing() {
+		XSSFWorkbook wb = new XSSFWorkbook();
+		XSSFSheet sheet = wb.createSheet("test");
+
+        sheet.setColumnWidth(4, 5000);
+        sheet.setColumnWidth(5, 5000);
+       
+        sheet.groupColumn((short) 4, (short) 5);
+        
+        sheet.setColumnGroupCollapsed(4, true);
+        
+        CTCols cols = sheet.getCTWorksheet().getColsArray(0);
+        logger.log(POILogger.DEBUG, "test52186_2/cols:" + cols);
+
+        wb = XSSFTestDataSamples.writeOutAndReadBack(wb, "testNoColsWithoutWidthWhenGroupingAndCollapsing");
+        sheet = wb.getSheet("test");
+        
+        for (int i = 4; i <= 5; i++) {
+        	assertEquals("Unexpected width of column "+ i, 5000, sheet.getColumnWidth(i));
+        }
+        cols = sheet.getCTWorksheet().getColsArray(0);
+        for (CTCol col : cols.getColList()) {
+			assertTrue("Col width attribute is unset: " + col.toString(), col.isSetWidth());
+		}
+    }
+    
+    /**
+     * Test the cols element is correct in case of NumericRanges.OVERLAPS_2_WRAPS
+     */
+    public void testMergingOverlappingCols_OVERLAPS_2_WRAPS() {
+		XSSFWorkbook wb = new XSSFWorkbook();
+		XSSFSheet sheet = wb.createSheet("test");
+
+        CTCols cols = sheet.getCTWorksheet().getColsArray(0);
+        CTCol col = cols.addNewCol();
+        col.setMin(1 + 1);
+        col.setMax(4 + 1);
+        col.setWidth(20);
+        col.setCustomWidth(true);
+
+        sheet.groupColumn((short) 2, (short) 3);
+        
+        sheet.getCTWorksheet().getColsArray(0);
+        logger.log(POILogger.DEBUG, "testMergingOverlappingCols_OVERLAPS_2_WRAPS/cols:" + cols);
+        
+        assertEquals(0, cols.getColArray(0).getOutlineLevel());
+        assertEquals(2, cols.getColArray(0).getMin()); // 1 based
+        assertEquals(2, cols.getColArray(0).getMax()); // 1 based
+        assertEquals(true, cols.getColArray(0).getCustomWidth());
+
+        assertEquals(1, cols.getColArray(1).getOutlineLevel());
+        assertEquals(3, cols.getColArray(1).getMin()); // 1 based
+        assertEquals(4, cols.getColArray(1).getMax()); // 1 based        
+        assertEquals(true, cols.getColArray(1).getCustomWidth());
+
+        assertEquals(0, cols.getColArray(2).getOutlineLevel());
+        assertEquals(5, cols.getColArray(2).getMin()); // 1 based
+        assertEquals(5, cols.getColArray(2).getMax()); // 1 based
+        assertEquals(true, cols.getColArray(2).getCustomWidth());
+        
+        assertEquals(3, cols.sizeOfColArray());
+
+        wb = XSSFTestDataSamples.writeOutAndReadBack(wb, "testMergingOverlappingCols_OVERLAPS_2_WRAPS");
+        sheet = wb.getSheet("test");
+        
+        for (int i = 1; i <= 4; i++) {
+        	assertEquals("Unexpected width of column "+ i, 20 * 256, sheet.getColumnWidth(i));
+        }
+    }
+
+    /**
+     * Test the cols element is correct in case of NumericRanges.OVERLAPS_1_WRAPS
+     */
+    public void testMergingOverlappingCols_OVERLAPS_1_WRAPS() {
+		XSSFWorkbook wb = new XSSFWorkbook();
+		XSSFSheet sheet = wb.createSheet("test");
+
+        CTCols cols = sheet.getCTWorksheet().getColsArray(0);
+        CTCol col = cols.addNewCol();
+        col.setMin(2 + 1);
+        col.setMax(4 + 1);
+        col.setWidth(20);
+        col.setCustomWidth(true);
+
+        sheet.groupColumn((short) 1, (short) 5);
+        
+        cols = sheet.getCTWorksheet().getColsArray(0);
+        logger.log(POILogger.DEBUG, "testMergingOverlappingCols_OVERLAPS_1_WRAPS/cols:" + cols);
+        
+        assertEquals(1, cols.getColArray(0).getOutlineLevel());
+        assertEquals(2, cols.getColArray(0).getMin()); // 1 based
+        assertEquals(2, cols.getColArray(0).getMax()); // 1 based
+        assertEquals(false, cols.getColArray(0).getCustomWidth());
+
+        assertEquals(1, cols.getColArray(1).getOutlineLevel());
+        assertEquals(3, cols.getColArray(1).getMin()); // 1 based
+        assertEquals(5, cols.getColArray(1).getMax()); // 1 based        
+        assertEquals(true, cols.getColArray(1).getCustomWidth());
+
+        assertEquals(1, cols.getColArray(2).getOutlineLevel());
+        assertEquals(6, cols.getColArray(2).getMin()); // 1 based
+        assertEquals(6, cols.getColArray(2).getMax()); // 1 based
+        assertEquals(false, cols.getColArray(2).getCustomWidth());
+        
+        assertEquals(3, cols.sizeOfColArray());
+        
+        wb = XSSFTestDataSamples.writeOutAndReadBack(wb, "testMergingOverlappingCols_OVERLAPS_1_WRAPS");
+        sheet = wb.getSheet("test");
+        
+        for (int i = 2; i <= 4; i++) {
+        	assertEquals("Unexpected width of column "+ i, 20 * 256, sheet.getColumnWidth(i));
+        }
+    }
+
+    /**
+     * Test the cols element is correct in case of NumericRanges.OVERLAPS_1_MINOR
+     */
+    public void testMergingOverlappingCols_OVERLAPS_1_MINOR() {
+		XSSFWorkbook wb = new XSSFWorkbook();
+		XSSFSheet sheet = wb.createSheet("test");
+
+        CTCols cols = sheet.getCTWorksheet().getColsArray(0);
+        CTCol col = cols.addNewCol();
+        col.setMin(2 + 1);
+        col.setMax(4 + 1);
+        col.setWidth(20);
+        col.setCustomWidth(true);
+
+        sheet.groupColumn((short) 3, (short) 5);
+        
+        cols = sheet.getCTWorksheet().getColsArray(0);
+        logger.log(POILogger.DEBUG, "testMergingOverlappingCols_OVERLAPS_1_MINOR/cols:" + cols);
+
+        assertEquals(0, cols.getColArray(0).getOutlineLevel());
+        assertEquals(3, cols.getColArray(0).getMin()); // 1 based
+        assertEquals(3, cols.getColArray(0).getMax()); // 1 based
+        assertEquals(true, cols.getColArray(0).getCustomWidth());
+
+        assertEquals(1, cols.getColArray(1).getOutlineLevel());
+        assertEquals(4, cols.getColArray(1).getMin()); // 1 based
+        assertEquals(5, cols.getColArray(1).getMax()); // 1 based        
+        assertEquals(true, cols.getColArray(1).getCustomWidth());
+
+        assertEquals(1, cols.getColArray(2).getOutlineLevel());
+        assertEquals(6, cols.getColArray(2).getMin()); // 1 based
+        assertEquals(6, cols.getColArray(2).getMax()); // 1 based
+        assertEquals(false, cols.getColArray(2).getCustomWidth());
+        
+        assertEquals(3, cols.sizeOfColArray());
+        
+        wb = XSSFTestDataSamples.writeOutAndReadBack(wb, "testMergingOverlappingCols_OVERLAPS_1_MINOR");
+        sheet = wb.getSheet("test");
+        
+        for (int i = 2; i <= 4; i++) {
+        	assertEquals("Unexpected width of column "+ i, 20 * 256, sheet.getColumnWidth(i));
+        }
+    	assertEquals("Unexpected width of column "+ 5, sheet.getDefaultColumnWidth() * 256, sheet.getColumnWidth(5));
+    }
+
+    /**
+     * Test the cols element is correct in case of NumericRanges.OVERLAPS_2_MINOR
+     */
+   public void testMergingOverlappingCols_OVERLAPS_2_MINOR() {
+		XSSFWorkbook wb = new XSSFWorkbook();
+		XSSFSheet sheet = wb.createSheet("test");
+
+        CTCols cols = sheet.getCTWorksheet().getColsArray(0);
+        CTCol col = cols.addNewCol();
+        col.setMin(2 + 1);
+        col.setMax(4 + 1);
+        col.setWidth(20);
+        col.setCustomWidth(true);
+
+        sheet.groupColumn((short) 1, (short) 3);
+        
+        cols = sheet.getCTWorksheet().getColsArray(0);
+        logger.log(POILogger.DEBUG, "testMergingOverlappingCols_OVERLAPS_2_MINOR/cols:" + cols);
+
+        assertEquals(1, cols.getColArray(0).getOutlineLevel());
+        assertEquals(2, cols.getColArray(0).getMin()); // 1 based
+        assertEquals(2, cols.getColArray(0).getMax()); // 1 based
+        assertEquals(false, cols.getColArray(0).getCustomWidth());
+
+        assertEquals(1, cols.getColArray(1).getOutlineLevel());
+        assertEquals(3, cols.getColArray(1).getMin()); // 1 based
+        assertEquals(4, cols.getColArray(1).getMax()); // 1 based        
+        assertEquals(true, cols.getColArray(1).getCustomWidth());
+
+        assertEquals(0, cols.getColArray(2).getOutlineLevel());
+        assertEquals(5, cols.getColArray(2).getMin()); // 1 based
+        assertEquals(5, cols.getColArray(2).getMax()); // 1 based
+        assertEquals(true, cols.getColArray(2).getCustomWidth());
+        
+        assertEquals(3, cols.sizeOfColArray());
+        
+        wb = XSSFTestDataSamples.writeOutAndReadBack(wb, "testMergingOverlappingCols_OVERLAPS_2_MINOR");
+        sheet = wb.getSheet("test");
+        
+        for (int i = 2; i <= 4; i++) {
+        	assertEquals("Unexpected width of column "+ i, 20 * 256, sheet.getColumnWidth(i));
+        }
+    	assertEquals("Unexpected width of column "+ 1, sheet.getDefaultColumnWidth() * 256, sheet.getColumnWidth(1));
+    }
+
+}