Bug 58432

Summary: Sheet and Row iterators expose remove method which does not correctly remove the row or cell
Product: POI Reporter: Javen O'Neal <onealj>
Component: SS CommonAssignee: POI Developers List <dev>
Status: NEW ---    
Severity: normal    
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Attachments: failing unit tests
unit test results

Description Javen O'Neal 2015-09-20 21:46:08 UTC 
Sheet[1] and Row[2] interfaces expose an iterator interface that has a remove method. This allows sheets to be removed from the underlying data structure (TreeMap.values() -> Collection view of TreeMap's values), but doesn't do the rest of the cleanup that is done in removeRow(Row)[3] or removeCell(Cell)[4]. If I understand the behavior of a Collection *view* correctly, this would remove the row/cell from the TreeMap without cleaning up the rest of the data structures.

The rowIterator and cellIterator should explicitly disallow the remove method unless the method correctly handles the rest of the cleanup without causing a ConcurrentModificationException.

Perhaps this would be a good usage for Collections.unmodifiableMap or Collections.unmodifiableList[5]. Otherwise, a custom iterator could be written like was done for XSSFWorkbook.iterator[6]

[1] XSSFSheet.iterator https://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java?revision=1702802&view=markup#l1780
[2] XSSFRow.iterator https://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java?revision=1649107&view=markup#l95
[3] XSSFSheet.removeRow https://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java?revision=1702802&view=markup#l1689
[4] XSSFRow.removeCell https://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFRow.java?revision=1649107&view=markup#l406
[5] Collections.unmodifiableMap https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html
[6] XSSFWorkbook.SheetIterator https://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java?revision=1703573&view=markup#l1058
Comment 1 Javen O'Neal 2015-09-21 06:28:08 UTC 
Created attachment 33121 [details]
failing unit tests

I've written unit tests for Workbook, Sheet, and Row which should test the sheetIterator, rowIterator, and cellIterator, respectively for the following scenarios:
1) iterator.remove() either is unsupported or behaves the same as the removeSheetAt, removeRow, or removeCell method
2) if removeSheetAt, removeRow, or removeCell are called after an iterator has been created, the iterator should throw a ConcurrentModificationException the next time a method is called on it.

The Workbook sheetIterator passes, but the rowIterator and cellIterator fail for HSSF, XSSF, and SXSSF variants.

Summary of test results
                  HSSF   XSSF   SXSSF
BaseTestWorkbook  pass   pass   pass
BaseTestSheet     1fail  1fail  1fail
BaseTestRow       2fails 1fail  1fail

I needed to modify HSSFRow.hashCode in order to have assertNull(HSSFRow) give a more meaningful error.
Comment 2 Javen O'Neal 2015-09-21 06:28:43 UTC 
Created attachment 33122 [details]
unit test results
Comment 3 Javen O'Neal 2015-09-21 06:32:57 UTC 
Additional unit tests are necessary to verify that the side-effects of iterator.remove are identical to removeSheetAt, removeRow, and removeCell.

I propose making iterator.remove() an UnsupportedOperationException for now until someone can find a solution that doesn't duplicate code and works correctly.