Bug 54920 - [PATCH] XSSFDrawing.createCellComment causes CommentsTable to lose reference to comment in cell A1
Summary: [PATCH] XSSFDrawing.createCellComment causes CommentsTable to lose reference ...
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.9-FINAL
Hardware: PC Mac OS X 10.4
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
Depends on:
Blocks: 58175
  Show dependency tree
Reported: 2013-05-02 16:56 UTC by ericleecarroll
Modified: 2018-12-07 06:45 UTC (History)
1 user (show)

Unit Test (2.26 KB, application/octet-stream)
2013-05-02 16:56 UTC, ericleecarroll
Updated unit-test which shows the problem more clearly, i.e. comment in A1 is there before and gone after comment in B2 is set (3.20 KB, text/x-java)
2013-05-25 14:19 UTC, Dominik Stadler
Proposed fix for (5.61 KB, patch)
2013-05-25 14:25 UTC, Dominik Stadler
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ericleecarroll 2013-05-02 16:56:59 UTC
Created attachment 30250 [details]
Unit Test

XSSFDrawing.createCellComment calls CommentsTable.newComment, which creates a temporary "A1" cell reference, and then updates the cell reference with the proper row and column.  But that temporary "A1" is placed in the commentRefs map, and can overwrite a legitimate comment in "A1".

I've attached a unit test that shows the issue.  There are a pair of 'NOTES' in the source that show the required steps to reproduce.
Comment 1 Dominik Stadler 2013-05-25 14:19:51 UTC
Created attachment 30322 [details]
Updated unit-test which shows the problem more clearly, i.e. comment in A1 is there before and gone after comment in B2 is set
Comment 2 Dominik Stadler 2013-05-25 14:25:41 UTC
Created attachment 30323 [details]
Proposed fix for

CommentsTable is dangerous as it overwrites comments in A1 because it uses this column as default reference in newComment(). 

This proposed patch changes this so a reference is always passed to newComment() and thus can be set immediately. This also avoids having to adjust the shape later on in the setCol()/setRow() columns.

Adjusted tests and the original test in this bug run fine then.

I am not sure if this breaks API compatibility, is newComment() exported to the outside? We could add a second newComment(String ref) to fix this in a compatible way, but this keeps a method which is dangerous to use anyway...

Another option to fix this would be to use a reference which does not exist in normal sheets, e.g. something like A0, but I did not look into this yet, any thoughts?
Comment 3 Nick Burch 2013-06-18 22:37:14 UTC
We normally try to maintain binary compatibility, unless there's a very good reason

If some situations will work fine with the current method signature, then I'd suggest we @deprecate it, add a note about the new one, and change all POI code to use the new call

If the old method signature will never work, then remove it, add the new one, and add something to the release notes.
Comment 4 Dominik Stadler 2013-06-23 21:34:55 UTC
Fixed with revision 1495894, we now deprecated the newComment() method without arguments and added a new one which receives the target reference for the comment as parameter. All uses in POI itself are adjusted accordingly.