groovy
  1. groovy
  2. GROOVY-5257

Node.ReplaceNode method fails cannot remove itself

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.0
    • Fix Version/s: 1.8.6, 2.0-beta-3
    • Component/s: XML Processing
    • Labels:
      None
    • Environment:
      ubuntu 11.04, eclipse 3.6, groovy eclipse plugin2.6.0.xx-20111212-0800-e36-RELEASE
    • Testcase included:
      yes
    • Number of attachments :
      1

      Description

      Perhaps this is not a frequent operation, but replaceNode method fails because of a nullpointer exception. The case corresponds to removing a root node from a xml document. This node has no parent (parent==null) and that is the point where the error is triggered. This may be considered a bad programming, but, anyway, the code should return something different from an exception.

        Activity

        Hide
        Paul King added a comment -

        It might be appropriate to return an Exception but probably not the NPE currently seen - possibly an UnsupportedOerationException with a clearer message. Incidentally, just noticed that the same applies to the plus(Closure) method as well.

        Show
        Paul King added a comment - It might be appropriate to return an Exception but probably not the NPE currently seen - possibly an UnsupportedOerationException with a clearer message. Incidentally, just noticed that the same applies to the plus(Closure) method as well.
        Hide
        Jorge Gomez added a comment -

        I agree. A minimum should be an exception signalling "you cannot remove the document's root node". Nevertheless, this restriction may constraint too much the kind of traversal you make over a DOM tree.

        In my example, I removed the node. However, if one intends to modify the root node, the same exception happens. Then, operations like removing an attribute or adding an attribute to a DOM tree cannot be programmed if they imply to modify the root node.

        I think it is possible a more generic still robust solution, but need to study further the sources.

        Show
        Jorge Gomez added a comment - I agree. A minimum should be an exception signalling "you cannot remove the document's root node". Nevertheless, this restriction may constraint too much the kind of traversal you make over a DOM tree. In my example, I removed the node. However, if one intends to modify the root node, the same exception happens. Then, operations like removing an attribute or adding an attribute to a DOM tree cannot be programmed if they imply to modify the root node. I think it is possible a more generic still robust solution, but need to study further the sources.
        Hide
        Jorge Gomez added a comment -

        Sorry, when I meant "adding an attribute" I should have said "replacing the node by another having different attributes" which is what the replaceNode is for.

        Show
        Jorge Gomez added a comment - Sorry, when I meant "adding an attribute" I should have said "replacing the node by another having different attributes" which is what the replaceNode is for.
        Hide
        Paul King added a comment -

        Given that attempting to "replace" the root node currently fails, I am inclined to return an UnsupportedOperationException for now until some better semantics is defined.

        It would certainly be possible to define the semantics for XmlParser and probably the DOM Category as well so that if the building closure returned a single node that it was returned as a "root" node. However, given that in other cases the replaceNode method mutates some existing data structure whereas the new semantics would just return a new node in the case of being a root node, then it might be a little unexpected.

        Just on the use case of changing a node's attributes, for both XmlParser and XmlSlurper you can get access to the attributes() map and make changes that way. For XmlSlurper this is actually kind of cheating because for nearly all other cases we treat the tree as mostly immutable and have changes kept as a tree of closure updates - so we may fix that hole in a future version of XmlSlurper.

        Show
        Paul King added a comment - Given that attempting to "replace" the root node currently fails, I am inclined to return an UnsupportedOperationException for now until some better semantics is defined. It would certainly be possible to define the semantics for XmlParser and probably the DOM Category as well so that if the building closure returned a single node that it was returned as a "root" node. However, given that in other cases the replaceNode method mutates some existing data structure whereas the new semantics would just return a new node in the case of being a root node, then it might be a little unexpected. Just on the use case of changing a node's attributes, for both XmlParser and XmlSlurper you can get access to the attributes() map and make changes that way. For XmlSlurper this is actually kind of cheating because for nearly all other cases we treat the tree as mostly immutable and have changes kept as a tree of closure updates - so we may fix that hole in a future version of XmlSlurper.
        Hide
        Jorge Gomez added a comment -

        You are more than right.

        I am not a groovy expert, but the behavior I suggested may contradict the replaceNode semantics when the root node is regarded. ReplaceNode requests are delivered because a there is a reference to the DOM root you are using. It replaceNode creates a new node for root, then the reference to the root node you were using becomes invalid and it is not possible to retrieve by regular means unless some additional programming is inserted. This happens as well with intermediate nodes, but those cases are not extreme since references to the new nodes can always be retrieved if there is a reference to the DOM root node.

        Thus, it makes sense to warn developers that, when replaceNode affects a root node of a DOM, an exception will be raised and make this the default behaviour in the API.

        Show
        Jorge Gomez added a comment - You are more than right. I am not a groovy expert, but the behavior I suggested may contradict the replaceNode semantics when the root node is regarded. ReplaceNode requests are delivered because a there is a reference to the DOM root you are using. It replaceNode creates a new node for root, then the reference to the root node you were using becomes invalid and it is not possible to retrieve by regular means unless some additional programming is inserted. This happens as well with intermediate nodes, but those cases are not extreme since references to the new nodes can always be retrieved if there is a reference to the DOM root node. Thus, it makes sense to warn developers that, when replaceNode affects a root node of a DOM, an exception will be raised and make this the default behaviour in the API.
        Hide
        Paul King added a comment - - edited

        This is fixed for XmlParser and DOMCategory (and the underlying groovy.util.Node) by returning an UnsupportedOperationException but not so for XmlSlurper which attempts to allow you to create XML fragments at any place including (conceptually) the root.

        Arguably, replaceNode should actually return the replaced node not the last node added while replacing - but that should be raised as a separate issue with appropriate feedback from the mailing list.

        Show
        Paul King added a comment - - edited This is fixed for XmlParser and DOMCategory (and the underlying groovy.util.Node) by returning an UnsupportedOperationException but not so for XmlSlurper which attempts to allow you to create XML fragments at any place including (conceptually) the root. Arguably, replaceNode should actually return the replaced node not the last node added while replacing - but that should be raised as a separate issue with appropriate feedback from the mailing list.

          People

          • Assignee:
            Paul King
            Reporter:
            Jorge Gomez
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: