Bug 56454 - XSSFSheet.shiftRows(...) and HSSFSheet.shiftRows(...) incorrectly handle merged regions that do not contain column 0.
Summary: XSSFSheet.shiftRows(...) and HSSFSheet.shiftRows(...) incorrectly handle merg...
Status: NEW
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 3.10-FINAL
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on: 60384
Blocks: 46742
  Show dependency tree
 
Reported: 2014-04-24 16:24 UTC by Jörg Selbach
Modified: 2019-05-30 03:49 UTC (History)
1 user (show)



Attachments
Test case demonstrating the problem. (1.40 KB, text/plain)
2014-04-24 16:24 UTC, Jörg Selbach
Details
Progress towards a fix (17.37 KB, patch)
2015-12-06 21:08 UTC, Javen O'Neal
Details | Diff
Progress towards a fix (13.98 KB, patch)
2015-12-06 21:18 UTC, Javen O'Neal
Details | Diff
Progress towards a fix (15.56 KB, patch)
2015-12-06 21:29 UTC, Javen O'Neal
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jörg Selbach 2014-04-24 16:24:36 UTC
Created attachment 31556 [details]
Test case demonstrating the problem.

XSSFSheet.shiftRows(...) and HSSFSheet.shiftRows(...) incorrectly handle merged regions starting above the first row to shift containing the shifted row, depending on the first column of the merged region.

If the merged region does not contain column 0, the region gets (incorrectly) shifted.

Consider the following example:

Case 1:

- Create merged region in A2:B5 (starting at row 3 col 0, ending at row 6 col 1)
- Shift row 4 down 1 row: sheet.shiftRows(3, sheet.getLastRowNum(), n);

Case 2:

- Create merged region in B2:C5 (starting at row 3 col 1, ending at row 6 col 2)
- Shift row 4 down 1 row: sheet.shiftRows(3, sheet.getLastRowNum(), n);

Observed Result:

In case 1, the merged region is still starting at row 3. In case 2, it starts at row 4.

Expected result:

Merged region should still be starting at row 3 in both cases.



The reason for this behaviour is in XSSFRowShifter line 69:

if (!containsCell(merged, startRow - 1, 0) && !containsCell(merged, endRow + 1, 0)) {

The check for column 0 should not be there. Instead, the line should look like this:

if (!containsRow(merged, startRow - 1) && !containsRow(merged, endRow + 1)) {

A similar check is done in HSSFSheet Line 1286 (using SheetUtil.containsCell(...)).
Comment 1 Javen O'Neal 2015-12-06 20:54:37 UTC
Thanks for finding this bug and providing a code to reproduce this bug. This bug still exists in POI 3.14beta1. I have added a failing unit test in r1718217.
Comment 2 Javen O'Neal 2015-12-06 21:08:52 UTC
Created attachment 33328 [details]
Progress towards a fix

Upon closer inspection, handling of sheet shifting has several issues besides ignoring merged regions that don't include the first column.

Depending on the shifted rows, merged regions may shift (if the merged region is fully contained inside the source move rows), shrink from the top if the first row is a source row and the shift direction is down, shrink from the bottom if the last row is a source row and the shift direction is up, expand from the top if the first row is a source row and the shift direction is up, expand from the top if the last row is a source row and the shift direction is down, or remove the merged region if there is a conflict (merged region in destination or some of merged regions from the source are moved outside the merged region). The current code only handles the first case, where the entire merged region shifts.

I've updated XSSFRowShifter to handle most of these cases. Many more unit tests are needed before this can be committed, then the logic in XSSFRowShifter.shiftMerged can be copied over to HSSFSheet.
Comment 3 Javen O'Neal 2015-12-06 21:18:18 UTC
Created attachment 33329 [details]
Progress towards a fix

rebase to r1718223
Comment 4 Javen O'Neal 2015-12-06 21:29:06 UTC
Created attachment 33330 [details]
Progress towards a fix
Comment 5 Javen O'Neal 2016-06-20 01:50:49 UTC
r1749239 add CellRangeAddress#containsRow and containsColumn
r1749246 add disabled unit test
r1749247 HSSF: replace containsCell with containsRow so that merged region doesn't have to contain column 0. (still not the correct behavior)
Comment 6 David Gauntt 2019-05-30 03:49:32 UTC
Test case for bug 63463 demonstrates this bug too.  In that test case, a merge area is defined for C3:D4.  If row B:B and below are shifted down one row, then the merge area becomes C3:D5 instead of C4:D5.  If column 2:2 and columns to the right are shifted right one column, the merge area becomes C3:E4 instead of D3:E4.