Commons Configuration
  1. Commons Configuration
  2. CONFIGURATION-103

[configuration] subset() method alters XMLConfiguration when invoked

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      Invoking subset() on an XMLConfiguration causes the underlying hierarchy to
      change. For example, using the database example on the webpage, performing
      the following and then serializing the XMLConfiguration file will show the
      structure has been altered:

      XMLConfiguration xmlConfig = new XMLConfiguration(databaseDotXmlUrl);
      xmlConfig.subset("tables.table(0");
      xmlConfig.save("C:/AlteredDatabase.xml");

      Not sure this was the intent of the method... actually the method returns a
      new HierarchicalConfiguration object but this method has undesired side effect
      on the original configuration.

        Activity

        Henri Yandell made changes -
        Affects Version/s unspecified [ 12311647 ]
        Component/s Configuration [ 12311107 ]
        Assignee Jakarta Commons Developers Mailing List [ commons-dev@jakarta.apache.org ]
        Project Commons [ 12310458 ] Commons Configuration [ 12310467 ]
        Key COM-2245 CONFIGURATION-103
        Henri Yandell made changes -
        Field Original Value New Value
        issue.field.bugzillaimportkey 35858 12342397
        Hide
        Oliver Heger added a comment -

        The original problem has been fixed, so I am closing this ticket now.

        For the discussion about the subset in HierarchicalConfiguration not being
        connected to its original configuration I opened a different ticket: 37081.

        Show
        Oliver Heger added a comment - The original problem has been fixed, so I am closing this ticket now. For the discussion about the subset in HierarchicalConfiguration not being connected to its original configuration I opened a different ticket: 37081.
        Hide
        Emmanuel Bourg added a comment -

        (In reply to comment #8)
        > About removing the subset() method at all: I think, this would work.
        > SubsetConfiguration only adds the correct prefix and passes the key then to
        > HierarchicalConfiguration, which correctly interprets indices. The subset test
        > in TestHierarchicalConfiguration also tests such complex keys.

        I added a test that fails if the subset() implementation is removed:

        assertEquals(fields[0][0], subset.getProperty("name(0)"));

        I haven't looked why it doesn't work with a SubsetConfiguration.

        Show
        Emmanuel Bourg added a comment - (In reply to comment #8) > About removing the subset() method at all: I think, this would work. > SubsetConfiguration only adds the correct prefix and passes the key then to > HierarchicalConfiguration, which correctly interprets indices. The subset test > in TestHierarchicalConfiguration also tests such complex keys. I added a test that fails if the subset() implementation is removed: assertEquals(fields [0] [0] , subset.getProperty("name(0)")); I haven't looked why it doesn't work with a SubsetConfiguration.
        Hide
        Oliver Heger added a comment -

        I committed a fix, which should solve the original problem (there was a bug in
        the CloneVisitor, which also affects the clone() method).

        About removing the subset() method at all: I think, this would work.
        SubsetConfiguration only adds the correct prefix and passes the key then to
        HierarchicalConfiguration, which correctly interprets indices. The subset test
        in TestHierarchicalConfiguration also tests such complex keys.

        Just adding the subset's nodes to a new HierarchicalConfiguration as you do in
        your alternative implementation will probably cause trouble with clearing
        property values: Here a node's parent reference is used for removing the child node.

        Show
        Oliver Heger added a comment - I committed a fix, which should solve the original problem (there was a bug in the CloneVisitor, which also affects the clone() method). About removing the subset() method at all: I think, this would work. SubsetConfiguration only adds the correct prefix and passes the key then to HierarchicalConfiguration, which correctly interprets indices. The subset test in TestHierarchicalConfiguration also tests such complex keys. Just adding the subset's nodes to a new HierarchicalConfiguration as you do in your alternative implementation will probably cause trouble with clearing property values: Here a node's parent reference is used for removing the child node.
        Hide
        Emmanuel Bourg added a comment -

        (In reply to comment #6)
        > Another approach would be to remove this special implementation of subset() in
        > HierarchicalConfiguration and rely on the implementation in
        > AbstractConfiguration. Will have to check if the tests then pass. IIRC
        > HierarchicalConfiguration.subset() had been once removed before the 1.0 release
        > and was later re-introduced because of problems with the
        > ConfigurationXMLDocument class, which later became obsolet. I somehow thought
        > that subset() after that was removed again, so I am now a bit surprised that it
        > exists in HierarchicalConfiguration at all.

        You are right, the implementation in AbstractConfiguration pass the subset test
        for HierarchicalConfiguration. The benefit of this implementation is to return a
        HierarchicalConfiguration, it implies a slight difference that is not covered by
        our tests. For example with the following structure:

        foo
        bar = value1
        foo
        bar = value2

        the subset on 'foo' returns a HierarchicalConfiguration that can be queried on
        the 'bar(0)' and 'bar(1)' keys. It wouldn't work if it was just a
        SubsetConfiguration translating the keys.

        Show
        Emmanuel Bourg added a comment - (In reply to comment #6) > Another approach would be to remove this special implementation of subset() in > HierarchicalConfiguration and rely on the implementation in > AbstractConfiguration. Will have to check if the tests then pass. IIRC > HierarchicalConfiguration.subset() had been once removed before the 1.0 release > and was later re-introduced because of problems with the > ConfigurationXMLDocument class, which later became obsolet. I somehow thought > that subset() after that was removed again, so I am now a bit surprised that it > exists in HierarchicalConfiguration at all. You are right, the implementation in AbstractConfiguration pass the subset test for HierarchicalConfiguration. The benefit of this implementation is to return a HierarchicalConfiguration, it implies a slight difference that is not covered by our tests. For example with the following structure: foo bar = value1 foo bar = value2 the subset on 'foo' returns a HierarchicalConfiguration that can be queried on the 'bar(0)' and 'bar(1)' keys. It wouldn't work if it was just a SubsetConfiguration translating the keys.
        Hide
        Oliver Heger added a comment -

        I will have to dig a little deeper into this. First I'd like to know exactly how
        a call to subset() changes the original configuration.

        Generally I agree with your patch, but we will have to check whether it causes
        problems if one Node object is added as child to two different parent nodes (the
        nodes contain a reference to their parent).

        Another approach would be to remove this special implementation of subset() in
        HierarchicalConfiguration and rely on the implementation in
        AbstractConfiguration. Will have to check if the tests then pass. IIRC
        HierarchicalConfiguration.subset() had been once removed before the 1.0 release
        and was later re-introduced because of problems with the
        ConfigurationXMLDocument class, which later became obsolet. I somehow thought
        that subset() after that was removed again, so I am now a bit surprised that it
        exists in HierarchicalConfiguration at all.

        Show
        Oliver Heger added a comment - I will have to dig a little deeper into this. First I'd like to know exactly how a call to subset() changes the original configuration. Generally I agree with your patch, but we will have to check whether it causes problems if one Node object is added as child to two different parent nodes (the nodes contain a reference to their parent). Another approach would be to remove this special implementation of subset() in HierarchicalConfiguration and rely on the implementation in AbstractConfiguration. Will have to check if the tests then pass. IIRC HierarchicalConfiguration.subset() had been once removed before the 1.0 release and was later re-introduced because of problems with the ConfigurationXMLDocument class, which later became obsolet. I somehow thought that subset() after that was removed again, so I am now a bit surprised that it exists in HierarchicalConfiguration at all.
        Hide
        Emmanuel Bourg added a comment -

        Here is an alternative implementation:

        public Configuration subset(String prefix)
        {
        Collection nodes = fetchNodeList(prefix);

        HierarchicalConfiguration result = new HierarchicalConfiguration();

        for (Iterator it = nodes.iterator(); it.hasNext()
        {
        Node node = (Node) it.next();
        for (Iterator children = node.getChildren().iterator();
        children.hasNext()

        { result.getRoot().addChild((Node) children.next()); }

        }

        return result;
        }

        Show
        Emmanuel Bourg added a comment - Here is an alternative implementation: public Configuration subset(String prefix) { Collection nodes = fetchNodeList(prefix); HierarchicalConfiguration result = new HierarchicalConfiguration(); for (Iterator it = nodes.iterator(); it.hasNext() { Node node = (Node) it.next(); for (Iterator children = node.getChildren().iterator(); children.hasNext() { result.getRoot().addChild((Node) children.next()); } } return result; }
        Hide
        Emmanuel Bourg added a comment -

        Oliver, I looked quickly at HierarchicalConfiguration.subset(String), it returns
        a copy of the sub tree, correct ? Is it possible to return a configuration with
        the selected node as root instead ? Thus any change to the subset would also
        affect the parent configuration, just like the other subset. That may also fix
        this issue.

        Show
        Emmanuel Bourg added a comment - Oliver, I looked quickly at HierarchicalConfiguration.subset(String), it returns a copy of the sub tree, correct ? Is it possible to return a configuration with the selected node as root instead ? Thus any change to the subset would also affect the parent configuration, just like the other subset. That may also fix this issue.
        Hide
        Jeffrey Mock added a comment -

        Created an attachment (id=15780)
        Source file with main()

        Copied the XML layout from the "How To" page of XML Configuration into a file
        called "resources/database.xml" on my classpath. The first three lines in the
        go() method is what is described in the bug. I first dump the contents of the
        XMLConfiguration to a file, then invoke the subset() method, and finally redump
        the contents to another XML file. The content of the two saved XML files is
        different.

        Show
        Jeffrey Mock added a comment - Created an attachment (id=15780) Source file with main() Copied the XML layout from the "How To" page of XML Configuration into a file called "resources/database.xml" on my classpath. The first three lines in the go() method is what is described in the bug. I first dump the contents of the XMLConfiguration to a file, then invoke the subset() method, and finally redump the contents to another XML file. The content of the two saved XML files is different.
        Hide
        Emmanuel Bourg added a comment -
            • COM-2246 has been marked as a duplicate of this bug. ***
        Show
        Emmanuel Bourg added a comment - COM-2246 has been marked as a duplicate of this bug. ***
        Hide
        Oliver Heger added a comment -

        Hi,

        could you please attach the test configuration file you use before and after the
        invocation of subset() to demonstrate how the structure was changed?

        Thanks.

        Show
        Oliver Heger added a comment - Hi, could you please attach the test configuration file you use before and after the invocation of subset() to demonstrate how the structure was changed? Thanks.
        Jeffrey Mock created issue -

          People

          • Assignee:
            Unassigned
            Reporter:
            Jeffrey Mock
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development