Bug 59355 - [PATCH] XSSFPivotTable::addColumnLabel sets the cell type of a cell outside of the source data area, potentially throwing NPE or causing side effects
Summary: [PATCH] XSSFPivotTable::addColumnLabel sets the cell type of a cell outside o...
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.15-dev
Hardware: Macintosh Mac OS X 10.1
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-19 15:04 UTC by Sherman Ying
Modified: 2016-05-19 20:18 UTC (History)
1 user (show)



Attachments
1 Line Patch Correction (457 bytes, application/x-gzip)
2016-04-19 15:04 UTC, Sherman Ying
Details
Patch for bug + testcase in TestXSSFPivotTable (1.15 KB, patch)
2016-04-19 21:35 UTC, Sherman Ying
Details | Diff
Patch for bug + testcase in TestXSSFPivotTable (4.00 KB, patch)
2016-04-19 21:42 UTC, Sherman Ying
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sherman Ying 2016-04-19 15:04:59 UTC
Created attachment 33778 [details]
1 Line Patch Correction

When the XSSFPivotTable is created with a source data area not starting from column index 0, adding a column label can throw an NPE when trying to change the column header cell style to String.

From investigation, it appears that the method private void addDataField(DataConsolidateFunction function, int columnIndex, String valueFieldName) incorrectly calculates the column for the corresponding header cell.
Comment 1 Dominik Stadler 2016-04-19 18:19:10 UTC
Thanks for the report, any chance of a small unit-test that allows us to reproduce this and ensure that it stays fixed in the future?
Comment 2 Sherman Ying 2016-04-19 21:30:27 UTC
(In reply to Dominik Stadler from comment #1)
> Thanks for the report, any chance of a small unit-test that allows us to
> reproduce this and ensure that it stays fixed in the future?

Should the unit test be added to the TestXSSFPivotTable test case?
Comment 3 Sherman Ying 2016-04-19 21:35:06 UTC
Created attachment 33779 [details]
Patch for bug + testcase in TestXSSFPivotTable
Comment 4 Sherman Ying 2016-04-19 21:39:06 UTC
The bug that causes the NullPointerException is in addDataField(DataConsolidateFunction function, int columnIndex, String valueFieldName)
It incorrectly calculates the column for the corresponding header cell. Instead of adding the column offset to the referenced area's first cell column index, it only uses the column offset.

If the mis-referenced column cell is uninitialized (and the sheet MissingCellPolicy is to return null), then it will throw a NullPointerException. Another side-effect is that if there was a non-String cell value in the mis-referenced cell, it will inadvertently change the cell type to String.
Comment 5 Sherman Ying 2016-04-19 21:41:00 UTC
Comment on attachment 33779 [details]
Patch for bug + testcase in TestXSSFPivotTable

Index: src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFPivotTable.java
===================================================================
--- src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFPivotTable.java	(revision 1739932)
+++ src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFPivotTable.java	(working copy)
@@ -355,7 +355,8 @@
         }
         CTDataField dataField = dataFields.addNewDataField();
         dataField.setSubtotal(STDataConsolidateFunction.Enum.forInt(function.getValue()));
-        Cell cell = getDataSheet().getRow(pivotArea.getFirstCell().getRow()).getCell(columnIndex);
+        Cell cell = getDataSheet().getRow(pivotArea.getFirstCell().getRow())
+                .getCell(pivotArea.getFirstCell().getCol() + columnIndex);
         cell.setCellType(Cell.CELL_TYPE_STRING);
         dataField.setName(valueFieldName);
         dataField.setFld(columnIndex);
Index: src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFPivotTable.java
===================================================================
--- src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFPivotTable.java	(revision 1739932)
+++ src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFPivotTable.java	(working copy)
@@ -32,6 +32,8 @@
 
 public class TestXSSFPivotTable extends TestCase {
     private XSSFPivotTable pivotTable;
+    private XSSFPivotTable offsetPivotTable;
+    private Cell offsetOuterCell;
     
     @Override
     public void setUp(){
@@ -71,6 +73,45 @@
 
         AreaReference source = new AreaReference("A1:C2");
         pivotTable = sheet.createPivotTable(source, new CellReference("H5"));
+        
+        XSSFSheet offsetSheet = (XSSFSheet) wb.createSheet();
+        
+        Row tableRow_1 = offsetSheet.createRow(1);
+        offsetOuterCell = tableRow_1.createCell(1);
+        offsetOuterCell.setCellValue(-1);
+        Cell tableCell_1_1 = tableRow_1.createCell(2);
+        tableCell_1_1.setCellValue("Row #");
+        Cell tableCell_1_2 = tableRow_1.createCell(3);
+        tableCell_1_2.setCellValue("Exponent");
+        Cell tableCell_1_3 = tableRow_1.createCell(4);
+        tableCell_1_3.setCellValue("10^Exponent");
+        
+        Row tableRow_2 = offsetSheet.createRow(2);
+        Cell tableCell_2_1 = tableRow_2.createCell(2);
+        tableCell_2_1.setCellValue(0);
+        Cell tableCell_2_2 = tableRow_2.createCell(3);
+        tableCell_2_2.setCellValue(0);
+        Cell tableCell_2_3 = tableRow_2.createCell(4);
+        tableCell_2_3.setCellValue(1);
+        
+        Row tableRow_3= offsetSheet.createRow(3);
+        Cell tableCell_3_1 = tableRow_3.createCell(2);
+        tableCell_3_1.setCellValue(1);
+        Cell tableCell_3_2 = tableRow_3.createCell(3);
+        tableCell_3_2.setCellValue(1);
+        Cell tableCell_3_3 = tableRow_3.createCell(4);
+        tableCell_3_3.setCellValue(10);
+        
+        Row tableRow_4 = offsetSheet.createRow(4);
+        Cell tableCell_4_1 = tableRow_4.createCell(2);
+        tableCell_4_1.setCellValue(2);
+        Cell tableCell_4_2 = tableRow_4.createCell(3);
+        tableCell_4_2.setCellValue(2);
+        Cell tableCell_4_3 = tableRow_4.createCell(4);
+        tableCell_4_3.setCellValue(100);
+        
+        AreaReference offsetSource = new AreaReference(new CellReference("C2"), new CellReference("E4"));
+        offsetPivotTable = offsetSheet.createPivotTable(offsetSource, new CellReference("C6"));
     }
 
     /**
@@ -268,4 +309,20 @@
         }
         fail();
     }
+    
+    /**
+     * Verify that the Pivot Table operates only within the referenced area, even when the
+     * first column of the referenced area is not index 0.
+     */
+    public void testAddDataColumnWithOffsetData() {
+        offsetPivotTable.addColumnLabel(DataConsolidateFunction.SUM, 1);
+        assertEquals(Cell.CELL_TYPE_NUMERIC, offsetOuterCell.getCellType());
+        
+        try {
+            offsetPivotTable.addColumnLabel(DataConsolidateFunction.SUM, 0);
+        } catch(NullPointerException e) {
+            fail();
+        }
+        return;
+    }
 }
Comment 6 Sherman Ying 2016-04-19 21:42:13 UTC
Created attachment 33780 [details]
Patch for bug + testcase in TestXSSFPivotTable
Comment 7 Dominik Stadler 2016-05-19 20:18:42 UTC
Applied via r1744635, thank you for the Patch!