Bug 50795 - Moving cell comment crashes Excel file
Summary: Moving cell comment crashes Excel file
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.7-FINAL
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-16 09:30 UTC by andrei
Modified: 2023-03-07 14:53 UTC (History)
1 user (show)



Attachments
CellCommentTest.xlsx (11.32 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2011-02-16 09:30 UTC, andrei
Details
CellCommentTest.java (1.14 KB, application/octet-stream)
2011-02-16 09:30 UTC, andrei
Details
jvmmonitor showing bottleneck (28.25 KB, image/png)
2023-03-03 12:48 UTC, javifinarfin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description andrei 2011-02-16 09:30:02 UTC
Created attachment 26668 [details]
CellCommentTest.xlsx

Steps:
1. Create Excel file.
2. Add cell comment.
3. Process with POI. Get our created cell comment and try to set to another cell.
  XSSFCell cell = row.getCell(0);
  XSSFComment comment = cell.getCellComment();
  XSSFCell cellWithoutComment = row.getCell(1);
  cellWithoutComment.setCellComment(comment);
4. Try to save changes
  FileOutputStream out = new FileOutputStream(testFilePath);
  wb.write(out);
  out.close();

Actual result: NPE appears, file size = 0 bytes
Expected: comment moved successfull (in 3.6 it works)

See attachments to test
Comment 1 andrei 2011-02-16 09:30:42 UTC
Created attachment 26669 [details]
CellCommentTest.java
Comment 2 Nick Burch 2011-02-18 10:31:05 UTC
Thanks for the test case, it made it much quicker to investigate

I'm rather confused by this one, but it's either a bug in xmlbeans, or we're doing something too odd in our processing of the vml diagram part. However, by adding a .toString() call in after the update, it seems to make xmlbeans re-compute whatever was broken. I'm sure there should be a better solution, but this seems to work fine as a workaround...

Test and workaround added in r1072022.
Comment 3 javifinarfin 2023-03-03 12:48:52 UTC
Created attachment 38517 [details]
jvmmonitor showing bottleneck

This workaround introduces a bottleneck when the sheet contains thousands of commits

See attached picture

Is this bug still an issue on last versions? Can we remove this?
Comment 4 PJ Fanning 2023-03-03 13:37:07 UTC
We're not going to remove code that's been there for 12 years.

POI is not very active dev wise any more.

If you want to investigate a solution, please go ahead. Open a new issue if you want to go this route. Any existing tests will need to continue to pass. We will merge a PR or a patch if it looks right.
Comment 5 javifinarfin 2023-03-03 13:58:05 UTC
hahah the patch could be to remove the function hahaha

if the case is tested, maybe that would be enough

regarding POI being active, you mean that is stable enough that doesn´t receive much changes or that maybe it would be better to check other libraries?
Comment 6 PJ Fanning 2023-03-03 14:04:38 UTC
The code is pretty stable and widely used.

The volunteer community is not big enough to make every change that people request. Generally, if you want a feature or a bug fixed, you should probably think about doing it yourself and submitting a PR or patch.

There are alternative libs, if you want to go this route. I don't use them, so I won't make any suggestions.
Comment 7 javifinarfin 2023-03-07 14:25:24 UTC
For the record, I moved from this bug. Even it is a bottleneck, the worst bottleneck comes from writing comments, because they are stored all on memory (lots of ram needed) and a xml copy method that invoked lots of time is inneficient. I haven't been able to found discussions regarding moving cell comments to streaming

For the record, we are talking about 500 000 comments...
Comment 8 PJ Fanning 2023-03-07 14:53:53 UTC
https://github.com/pjfanning/poi-shared-strings has an alternative implementation of the POI CommentsTable. You could try using that to see it helps.