Bug 66079

Summary: XWPFNumbering.removeAbstractNum still removes by list index, not abstractNumId
Product: POI Reporter: Vladislav Arakelov <arakelov93>
Component: XWPFAssignee: POI Developers List <dev>
Status: RESOLVED FIXED    
Severity: normal CC: arakelov93
Priority: P2    
Version: 5.2.0-FINAL   
Target Milestone: ---   
Hardware: PC   
OS: All   
Attachments: test for r1901083
test for numbering edit

Description Vladislav Arakelov 2022-05-20 09:24:35 UTC
There was bug #63788, bit it wasn't fixed properly. Here is what I am talking about, org.apache.poi.xwpf.usermodel.XWPFNumbering#removeAbstractNum :

    public boolean removeAbstractNum(BigInteger abstractNumID) {
        for (XWPFAbstractNum abstractNum : abstractNums) {
            BigInteger foundNumId = abstractNum.getAbstractNum().getAbstractNumId();
            if(abstractNumID.equals(foundNumId)) {
                ctNumbering.removeAbstractNum(foundNumId.byteValue());  <-- still uses index, not id
                abstractNums.remove(abstractNum);                       <-- works fine
                return true;
            }
        }

        return false;
    }

And so, in test63788 - abstract numbering with id 2 is deleted from abstractNums, but from ctNumbering, that is actually what will be written to file when writing .docx - numbering with id 7 is deleted.
Comment 1 PJ Fanning 2022-05-20 09:43:04 UTC
yes - the original change looks wrong
Comment 2 PJ Fanning 2022-05-20 09:47:42 UTC
I added r1901083 as a first stab at fixing this
Comment 3 PJ Fanning 2022-05-20 15:09:12 UTC
Vladislav - since you understand the issue better than the rest of use after researching it, can you suggest changes to test63788 to better test this issue? That test passes with and without your suggested change.
Comment 4 Vladislav Arakelov 2022-05-20 15:41:08 UTC
(In reply to PJ Fanning from comment #3)
> Vladislav - since you understand the issue better than the rest of use after
> researching it, can you suggest changes to test63788 to better test this
> issue? That test passes with and without your suggested change.

It's complicated, because ctNumbering has private access in XWPFNumbering. Also, there is wider problem about this component: there is a method XWPFNumbering.getAbstractNum(BigInteger abstractNumID), but the returned object has no connection with actual ctNumbering, and therefore abstract numberings cannot be edited with this API. 
Actually, I encountered this problem about removeAbstractNum while trying to workaround this impossibility to edit numberings by deleting them and adding again :) . So, I will try to spend some time this weekend trying to make an appropriate test, but I think this component needs more refactoring.
Comment 5 Vladislav Arakelov 2022-05-21 22:10:48 UTC
Created attachment 38293 [details]
test for r1901083
Comment 6 Vladislav Arakelov 2022-05-21 22:12:08 UTC
Created attachment 38294 [details]
test for numbering edit
Comment 7 Vladislav Arakelov 2022-05-21 22:13:24 UTC
So, I made a test that checks for deleting a numbering (attachment "test for r1901083"), and modified it further to check for editing a numbering ("test for numbering edit"). The first version fails wthout your change (r1901083), second - requires additional work to support editing of numberings.

(I don't have experience in using svn, so this is git patches)
Comment 8 PJ Fanning 2022-05-21 22:31:32 UTC
do you have git branches? - it would be easier to track what your patches are with the context of seeing the commits in a branch

I had a look at https://bz.apache.org/bugzilla/attachment.cgi?id=38294 and it bears no resemblance to the trunk code in POI.
Comment 9 PJ Fanning 2022-05-21 22:39:18 UTC
I added 'test for r1901083' using r1901105
Comment 10 Vladislav Arakelov 2022-05-21 22:53:22 UTC
(In reply to PJ Fanning from comment #9)
> I added 'test for r1901083' using r1901105

Great, thank you! I don't yet understand how the processes work here, does it mean this fix will be in the next poi release? 


(In reply to PJ Fanning from comment #8)
> do you have git branches? - it would be easier to track what your patches
> are with the context of seeing the commits in a branch

I didn't push it anywhere, only on my local repository. You can apply the second patch after the first, and the test will fail because editing does not work (changes are not saved to .docx).
Comment 11 PJ Fanning 2022-05-21 22:58:53 UTC
There are no releases planned - possibly next one will be in Autumn.

I tried to apply your 2nd patch manually but the test fails.
Comment 12 Vladislav Arakelov 2022-05-21 23:12:47 UTC
(In reply to PJ Fanning from comment #11)
> I tried to apply your 2nd patch manually but the test fails.

Yes, and as I said - this is intented to show that editing does not work. But this is another bug