[XMLBEANS-533] Wrong expectations from generated setFooArray(pos, obj) or getFooList().set(pos, obj)

• Type: Bug
• Status: Open
• Priority: Major
• Resolution: Unresolved
• Affects Version/s: Version 3.0.2
• Component/s: Documentation, XmlObject
• Environment: Java 8, Windows 10
• Assignee: Unassigned
• Reporter: Konstantin Kolinko
• Created: 07/Feb/19 14:46
• Updated: 07/Feb/19 14:46

Description

I am tracking down the issue described in StackOverflow question 53211749 "Can't change row text in .docx file once row is added to table".

I am using Apache POI 4.0.1. It uses XmlBeans 3.0.2.jar. The classes generated by XmlBeans are poi-ooxml-schemas-4.0.1.jar (a subset of ooxml-schemas, the source code is ooxml-schemas-1.4-sources.jar, as explained in POI FAQ, item #3).

I found the following:

1. Apache POI 4.0.1 has the following code:

package org.apache.poi.xwpf.usermodel;
public class XWPFTable implements ... {
    ...
    /**
     * add a new Row to the table
     * at position pos
     *
     * @param row the row which should be added
     */
    public boolean addRow(XWPFTableRow row, int pos) {
        if (pos >= 0 && pos <= tableRows.size()) {
            ctTbl.insertNewTr(pos);
            ctTbl.setTrArray(pos, row.getCtRow());
            tableRows.add(pos, row);
            return true;
        }
        return false;
    }
}

CTTbl and CTRow are classes generated by XmlBeans, in package
org.openxmlformats.schemas.wordprocessingml.x2006.main
of ooxml-schemas-1.4-sources.jar

XWPFTable and XWPFTableRow are wrappers / facades for CTTbl and CTRow respectively.

EXPECTED:

Apparently the assumption here is that

a. insertNewTr(pos) inserts a new element into array
b. setTrArray(pos, row.getCtRow()); replaces the element with the given one.
c. row.getCtRow() is the same object that is stored in ctTbl.

ACTUAL:

a. insertNewTr(pos) creates an XML Element and creates a new "user's object" for it, that is a new instance of CtRowImpl implementation class.

b. setTrArray(pos, row.getCtRow()) finds existing XML element at index pos, finds existing "user's object" for it, and copies contents from the source object into the existing one.

Internally: CtTblImpl.setTrArray(pos,obj) is implemented as

    private static final javax.xml.namespace.QName TR$36 = 
        new javax.xml.namespace.QName("http://schemas.openxmlformats.org/wordprocessingml/2006/main", "tr");

    public void setTrArray(int i, org.openxmlformats.schemas.wordprocessingml.x2006.main.CTRow tr)
    {
        generatedSetterHelperImpl(tr, TR$36, i, org.apache.xmlbeans.impl.values.XmlObjectBase.KIND_SETTERHELPER_ARRAYITEM);
    }

XmlObjectBase.generatedSetterHelperImpl(XmlObject src, QName, int index, short kind) passes the call to XmlObjectBase.objSetterHelper(XmlObjectBase srcObj, QName, int index, short kind)

    private TypeStoreUser objSetterHelper(XmlObjectBase srcObj, QName propName, int index, short kindSetterHelper)
    {
        XmlObjectBase target = getTargetForSetter(propName, index, kindSetterHelper);

        [..] // some checks

        return target.get_store().copy_contents_from( srcObj.get_store() ).
                get_store().change_type( srcObj.schemaType() );
    }

The  getTargetForSetter(..) call returns the existing CtRowImpl instance (added by "insertNewTr" call), and this existing instance is being updated.

c. The POI code goes on with tableRows.add(pos, row);, using its existing wrapper object XWPFTableRow, but at this point the object is out of sync with its containing XWPFTable: row.getCtRow() != ctTbl.getTrArray(pos).

This is observed by users of POI library: when one modifies the data in the added row, it does not affect the underlying XML document and is not written out when saving the document. This is the behaviour described in that StackOverflow question.

2. I looked up documentation for the expected behaviour of setFooArray().

I found the following page:
http://xmlbeans.apache.org/docs/3.0.0/guide/conMethodsForGeneratedJavaTypes.html
"Methods for Types Generated From Schema"

It says that "sets the Foo child element at the specified index.".
The actual behaviour is that it "copies the value of newValue, setting the child element at specified index". The assignment is by value, not by reference.

3. The same would happen if one accesses the array via a list facade returned by getFooList().set(pos, obj). Its method set(index, obj) is a facade for setFooArray(obj, index).

SUGGESTED SOLUTION

Looking at similar issues (e.g. XMLBEANS-252) I doubt that behaviour of setFooArray(obj,pos) can be chamged at this point. I think that

1. The documentation [1] in XmlBeans should be updated to make it clear that assignment happens by value, not by reference.

2. I think that in POI one can deprecate the addFoo() methods that return boolean and instead provide methods that return a new wrapper object.

After all, the main use case that people are using is copying existing document content around, e.g. copying existing rows or paragraphs. The method can make it clear.

This concerns not only XWPFTable.addRow(XWPFTableRow), XWPFTable.addRow(XWPFTableRow row, int pos), but similar methods in other classes. E.g. XWPFParagraph.addRun(CTR.

Alternatively, maybe the original method can be fixed by updating the wrapped object. That is, to amend the above POI code with the following line:

    row.setCtRow(ctTbl.getTrArray(pos));

Caveat: method setCtRow() has not been implemented yet, and this means that the wrapped objects must be updated further down the line - in all table cell, in all paragraphs etc. It is doable, but... It would be easier to just create a new wrapper object, reusing existing constructor.

While the bug is observed with Apache POI, I first file it here, as I see incorrect code being written more that once, and it may occur in other projects. I think that the documentation in this project should make it clear to avoid such coding errors.