Bug 64791 - [PATCH] getWriteAccess() call in InternalWorkbook always creates a new record (for in-memory workbooks)
Summary: [PATCH] getWriteAccess() call in InternalWorkbook always creates a new record...
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: HSSF (show other bugs)
Version: unspecified
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2020-10-06 19:03 UTC by rwhitcomb
Modified: 2020-10-26 21:19 UTC (History)
1 user (show)



Attachments
XLS file with two different writeAccess records (4.00 KB, application/vnd.ms-excel)
2020-10-06 22:34 UTC, rwhitcomb
Details
Patch file with changes to InternalWorkbook and TestWorkbook (unit test) (18.93 KB, application/gzip)
2020-10-07 00:03 UTC, rwhitcomb
Details
Results of "ant test-main" before the change to InternalWorkbook (10.39 KB, text/plain)
2020-10-07 00:04 UTC, rwhitcomb
Details
Results of "ant test-main" after the InternalWorkbook change. (70.69 KB, text/plain)
2020-10-07 00:05 UTC, rwhitcomb
Details

Note You need to log in before you can comment on or make changes to this bug.
Description rwhitcomb 2020-10-06 19:03:50 UTC
The "getWriteAccess()" method in InternalWorkbook (org.apache.poi.hssf.model.InternalWorkbook) checks if the "writeAccess" member is non-null, and if not calls "createWriteAccess" and sets the "writeAccess" member. 

However, "createWorkbook()" (that is, the in-memory version) just calls "createWriteAccess" which does NOT set the "writeAccess" member, meaning that a subsequent call to "getWriteAccess" creates a second record. This same paradigm / bug may apply to other records as well (I haven't checked).

Anyway, I think the best way to address this is to make this change to "InternalWorkbook.createWorkbook()" (Apache POI 4.1.2 code):
@@ -336,7 +336,7 @@
          records.add(new InterfaceHdrRecord(CODEPAGE));
          records.add(createMMS());
          records.add(InterfaceEndRecord.instance);
-         records.add(createWriteAccess());
+         getWriteAccess();
          records.add(createCodepage());
          records.add(createDSF());
          records.add(createTabId());

which will correctly set "writeAccess" and still put the record in the correct position in the BIFF stream.
Comment 1 rwhitcomb 2020-10-06 20:31:31 UTC
Real patch file and unit tests are forthcoming.
Comment 2 rwhitcomb 2020-10-06 22:34:35 UTC
Created attachment 37480 [details]
XLS file with two different writeAccess records

XLS file was created with "new HSSFWorkbook", etc. then did:
HSSFWorkbook wb = new HSSFWorkbook();
...
InternalWorkbook internalWorkbook = wb.getInternalWorkbook();
WriteAccessRecord writeAccess = internalWorkbook.getWriteAccess();
writeAccess.setUsername("gvtrwhit");

then write out the bytes with "wb.write(...)"

So, simply the call to "getWriteAccess()" created the second record (which then had its username field set).

BiffViewer shows this:
Offset=0x00000106(262) recno=5 sid=0x00E2 size=0x0000(0)
[INTERFACEEND/]

Offset=0x0000010A(266) recno=6 sid=0x005C size=0x0070(112)
[WRITEACCESS]
    .name = gvtrwhit
[/WRITEACCESS]

Offset=0x0000017E(382) recno=7 sid=0x005C size=0x0070(112)
[WRITEACCESS]
    .name = gvtsstag
[/WRITEACCESS]

Offset=0x000001F2(498) recno=8 sid=0x0042 size=0x0002(2)
[CODEPAGE]
    .codepage        = 4b0
[/CODEPAGE]
Comment 3 rwhitcomb 2020-10-07 00:03:39 UTC
Created attachment 37481 [details]
Patch file with changes to InternalWorkbook and TestWorkbook (unit test)
Comment 4 rwhitcomb 2020-10-07 00:04:36 UTC
Created attachment 37482 [details]
Results of "ant test-main" before the change to InternalWorkbook
Comment 5 rwhitcomb 2020-10-07 00:05:06 UTC
Created attachment 37483 [details]
Results of "ant test-main" after the InternalWorkbook change.
Comment 6 rwhitcomb 2020-10-07 00:07:06 UTC
Patch file created with "ant -f patch.xml"
Comment 7 Dominik Stadler 2020-10-25 07:23:23 UTC
Applied via r1882829, thanks for the patch, test and detailed explanation!
Comment 8 rwhitcomb 2020-10-26 21:15:53 UTC
(In reply to Dominik Stadler from comment #7)
> Applied via r1882829, thanks for the patch, test and detailed explanation!

You're welcome. Just as a curiosity, do you guys ever backport fixes? I mean are you considering a 4.1.3 at any time? So, would there be any need to merge this change anywhere else besides trunk?
Comment 9 rwhitcomb 2020-10-26 21:16:51 UTC
(In reply to Dominik Stadler from comment #7)
> Applied via r1882829, thanks for the patch, test and detailed explanation!

You're welcome. Just as a curiosity, do you guys ever backport fixes? I mean are you considering a 4.1.3 at any time? So, would there be any need to merge this change anywhere else besides trunk?
Comment 10 PJ Fanning 2020-10-26 21:19:43 UTC
We don't typically release patches for old code lines. We would probably only consider it if there was a major bug or security issues.
POI 5.0.0 should be released by year end.