Bug 58432 - Sheet and Row iterators expose remove method which does not correctly remove the row or cell
Summary: Sheet and Row iterators expose remove method which does not correctly remove ...
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: unspecified
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
Depends on:
Reported: 2015-09-20 21:46 UTC by Javen O'Neal
Modified: 2015-09-21 06:32 UTC (History)
0 users

failing unit tests (10.23 KB, patch)
2015-09-21 06:28 UTC, Javen O'Neal
Details | Diff
unit test results (6.17 KB, text/plain)
2015-09-21 06:28 UTC, Javen O'Neal

Note You need to log in before you can comment on or make changes to this bug.
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.