Possible bug in deepClone()- or insertAfter()- or

Hi,

I was checking out the BETA of Aspose.Words 10.0.0 and found it mostly compatible with our existing code. As mentioned in other posts, the loss of generics in e.g. getChildren() is a bit annoying but bearable.

Also, you seem to have obfuscated a few more of your classes than intended; in order to do replacing, I had to instantiate the class asposewobfuscated.rn:

doc.getRange().replace(new asposewobfuscated.rn(text), text)

The only serious thing so far, though, is the following: my code takes a CompositeNode from a document, copies it, and inserts its children (which should be copies, obviously) back into the document. Unfortunately, adding the copied Nodes into the document changes the iterator over the parent copy’s child nodes. Iterating over its children thus goes on forever.

The following test case illustrates the problem:

package com.mgmtp.asposetest;

import com.aspose.words.Body;
import com.aspose.words.Document;
import com.aspose.words.Node;
import org.testng.Assert;
import org.testng.annotations.Test;

/**
 * Demonstrates a problem with Aspose.Words 10.0.0-BETA
 *
 * @author Johannes Bauer
 * @date $LastChangedDate$
 * @revision $LastChangedRevision$
 * @since 1.4
 */
public class AsposeTest {
    @Test
    public void test() throws Exception{
        Document doc = new Document(getClass().getResourceAsStream("testfile.doc"));
        
        Body body = doc.getFirstSection().getBody();
        Body bodyCopy = (Body) body.deepClone(true);
        
        int cntr = 0;
        for(Object node : bodyCopy.getChildNodes()) {
            cntr++;
        }
        
        for(Object node : bodyCopy.getChildNodes()) {
            body.insertAfter((Node) node, null);
            cntr–;
            Assert.assertTrue(cntr >= 0, "The number of children of bodyCopy increased while inserting its children into body.");
        }
    }
}

I realize I could just collect all children of bodyCopy in a List and insert them into body, but that’d be a hack and dangerous.

I’m attaching the Word document I created for this test; it’s just a one-liner containing only the text “Some Text.”.

As always grateful for any suggestions,
Johannes Bauer

Hi
Thanks for your request. I see the problem with Replace method. I logged it in our defect tracking system. You will be notified as soon as it is resolved.
Regarding the problem with iteraterator. I think you should use code like the following:

// Get array of child nodes.
Node[] children = bodyCopy.getChildNodes().toArray();
for (Node node: children)
{
    body.insertAfter(node, null);
    cntr--;
    Assert.assertTrue(cntr>= 0, "The number of children of bodyCopy increased while inserting its children into body.");
}

Hope this helps.
Best regards,

Hi,

thanks a lot for your reply. I’ve come up with and tested a workaround for the iterator problem that is quite similar to what you’re suggesting. It does work, but I still believe what I found is a bug. Let me explain:

Normally, I would think getChildNodes() should return a Collection that doesn’t change just because its elements are added to another parent. In particular, the number of times I should be able to call next() on the iterator returned by getChildNodes().iterator() should stay the same regardless of what I do to the elements returned by those calls to next().

Now, I could imagine that adding those elements to a different parent implicitly removes them from bodyCopy and thus from the NodeCollection returned by bodyCopy.getChildNodes(). In that case, however, the iterator should just throw a ConcurrentModificationException to indicate that the Iterable over whose elements it is iterating has changed and its behaviour is thus undefined. It should definetly not iterate over a different Iterable or do God knows what it is doing and stay at it forever. That’s highly unexpected behaviour and it makes for terrible to debug problems.

The problem I seem to be onto here is pretty involved and I really hope I managed to make my point clear. If not, tell me and I’ll think about how to explain it any simpler.

Cheers,
Johannes

Hi Johannes,
Thanks for this additional information.
There is an overload of the GetChildNodes method which accepts an extra parameter - isLive. When this is false the collection returned is static and does not change even when a node is moved. I think you can use this in your case to avoid these sorts of errors.
However I do not think this is a bug, even if it can result in unexpected behaviour. Also we try to avoid throwing exceptions where they are not expected.
Thanks,

Hi Adam,

thanks for your reply; useful information, that. Doesn’t solve the problem though. I updated my test method and it still fails:

public void test() throws Exception{
    Document doc = new Document(getClass().getResourceAsStream("testfile.doc"));

    Body body = doc.getFirstSection().getBody();
    Body bodyCopy = (Body) body.deepClone(true);

    int cntr = 0;
    final NodeCollection childNodes = bodyCopy.getChildNodes();
    for(Object node : childNodes) {
        cntr++;
    }

    final NodeCollection childNodes1 = bodyCopy.getChildNodes(NodeType.ANY, false, false);
    for(Object node : childNodes1) {
        body.insertAfter((Node) node, null);
        cntr–;
        Assert.assertTrue(cntr >= 0, "The number of children of bodyCopy increased while inserting its children into body.");
    }
}

I believe the best way to state the problem is this: say nodeCollection is the NodeCollection returned by childNodes.getChildNodes(). Then I expect the iterator returned from nodeCollection.iterator() to iterate over nodeCollection (as per the usual understanding of java.util.Iterable).

Since childNodes’ size doesn’t grow in my test method, the iteration should be finite. The fact that the iteration goes on forever indicates that the iterator is not iterating childNodes.

I’m pretty sure this behaviour is a bug since it contradicts the expected behaviour of Iterable. I’m changing my code, though, since it was making wrong assumptions in the first place and I’m leaving the design decisions in your architects’ capable hands

Cheers, thanks for putting up with my pickyness,
Johannes

Hi
Thank you for additional information. But test code is not quite correct. bodyCopy.getChildNodes() and bodyCopy.getChildNodes(NodeType.ANY, false, false) actually returns different collections. The first method returns the collection of direct children of the node, and the second one returns collection of all nodes of specified type inside the composite node (including nested). That is why counter becomes negative.
Adam is right getChildNodes() is a ‘live’ collection. If you need ‘not live’ collection, you should use ToArray method as I mentioned.
Best regards,

Hi,

according to getChildNodes()'s JavaDoc, getChildNodes(…, false, false) should return a non-live NodeCollection that only contains immediate children (as Adam pointed out).

Anyway: I get the same results if I replace

childNodes = bodyCopy.getChildNodes()

by

childNodes = bodyCopy.getChildNodes(NodeType.ANY, false, false)

The iterator supports a different number of calls to next() depending on what I do to the returned Nodes.

It really doesn’t matter to me; I’m using the toArray() version, and that’s cleaner than what I had so far. I just think this is a broken implementation of the Iterable interface. If you disagree or think that’s true but ok, that’s fine. I just want to make sure that bug, if it is one, isn’t overlooked just because I’m not making myself clear enough.

Cheers,
Johannes

Hi Johannes,
Thank you for additional information. I logged your suggestion in our defect database. We will consider changing the behavior. We will let you know once the issue is resolved.
Best regards,

Hi Johannes,

Thanks for your patience. It is to update you that our development team has finished analysing your issue (WORDSJAVA-68) and has come to a conclusion that your issue and the behaviour you’re observing is actually not a bug. The behaviour of NodeCollection is correct and it won’t be changed. Please let us know if you need more information, we are always glad to help you.

Best Regards,

Hi Johannes,

In addition to Awais’s feedback, we decided to opt for a “hot remove” feature instead of throwing an exception if a node is removed during interaction. This is mainly due to the performance benefits.

You can read further information about this in the list of the Changes to the Public API in Aspose.Words 11.8.0.

Thanks,