Commons Configuration
  1. Commons Configuration
  2. CONFIGURATION-132

[configuration] HierarchicalConfigurationXMLReader stores comments as text nodes

    Details

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

      Operating System: Windows XP
      Platform: PC

      Description

      I am seeing behavior in the HierarchicalConfigurationXMLReader class (v 1.5
      2004/07/05 09:54:17) where XML Comments are inappropriately stored as text
      nodes.

      When using ConfigurationXMLDocument (ConfigurationXMLDocument.java,v 1.6
      2004/06/24 14:01:03) to write the configuration back out to a file, the
      original XML comments are written out as concatenated text nodes.

      Here are the steps for recreation:

      1) Read in this configuration using the hierarchicalXml designator:

      <config>
      <!-- Settings for the Wizard, now externalized via Perl. -->
      <Wizard>
      <!-- Location of perl binary. -->
      <PerlPath>C:/cygwin/bin/perl</PerlPath>

      <!-- Perl script and arguments for the wizard. -->
      <WizardScript>C:/foo/wizard.pl</WizardScript>
      </Wizard>
      </config>

      2) Pass the HierarchicalXMLConfiguration instance to ConfigurationXMLDocument
      to write() to a file:

      ConfigurationXMLDocument configDoc = new ConfigurationXMLDocument(config);
      Writer out = new BufferedWriter(new FileWriter(outputFile));
      configDoc.write(out);

      3) The resulting XML (written out by Dom4J) demonstrates that all comments were
      read in and concatenated into text nodes:

      <config>Settings for the Wizard, now externalized via Perl.
      <Wizard>Location of perl binary. Perl script and arguments for the wizard.
      <PerlPath>C:/cygwin/bin/perl</PerlPath>
      <WizardScript>C:/foo/wizard.pl</WizardScript>
      </Wizard>
      </config>

        Activity

        Hide
        David Eric Pugh added a comment -

        Mark,
        I have applied the patches. One other quibbling detail. When you do a patch,
        include new files as well. I had to guess at the xml files name (from looking
        at the testcase) and create the xml from scratch. Most ide's I think have an
        option for "include new files". Thanks for the pretty unit tests! Bumps our
        unit test coverage from 87 to 88 percent!

        Show
        David Eric Pugh added a comment - Mark, I have applied the patches. One other quibbling detail. When you do a patch, include new files as well. I had to guess at the xml files name (from looking at the testcase) and create the xml from scratch. Most ide's I think have an option for "include new files". Thanks for the pretty unit tests! Bumps our unit test coverage from 87 to 88 percent!
        Hide
        woodman added a comment -

        Re-opening for consideration of diff patch (attachment 12445) and the XML test
        document it uses (12446).

        Show
        woodman added a comment - Re-opening for consideration of diff patch (attachment 12445) and the XML test document it uses (12446).
        Hide
        woodman added a comment -

        Created an attachment (id=12446)
        New XML file to go in jakarta-commons\configuration\conf for patch

        Show
        woodman added a comment - Created an attachment (id=12446) New XML file to go in jakarta-commons\configuration\conf for patch
        Hide
        woodman added a comment -

        Created an attachment (id=12445)
        Test case additions to verify XML node types correctly interpreted.

        Show
        woodman added a comment - Created an attachment (id=12445) Test case additions to verify XML node types correctly interpreted.
        Hide
        David Eric Pugh added a comment -

        I have applied the fix. I am going to go ahead and close this bug as I want to
        get as many bugs closed for the RC1 as possible. However, don't let me disuade
        you from adding a unit test! You may want to at that time add your getXML or
        toXML method to help with testing.

        Go ahead and either reopen this bug or submit a fresh bug.

        Oh, and I view (in good XP fashion) as everythign being a shared responbility.
        It isn't just my responbility or yours to supply a testcase, but instead a
        shared responsibility! My only responsibility as a committer is to do my best
        to encourage people to contribute and ensure that contributions are in good
        shape and enhance the codebase! Glad to have your participation.

        Eric

        Show
        David Eric Pugh added a comment - I have applied the fix. I am going to go ahead and close this bug as I want to get as many bugs closed for the RC1 as possible. However, don't let me disuade you from adding a unit test! You may want to at that time add your getXML or toXML method to help with testing. Go ahead and either reopen this bug or submit a fresh bug. Oh, and I view (in good XP fashion) as everythign being a shared responbility. It isn't just my responbility or yours to supply a testcase, but instead a shared responsibility! My only responsibility as a committer is to do my best to encourage people to contribute and ensure that contributions are in good shape and enhance the codebase! Glad to have your participation. Eric
        Hide
        David Eric Pugh added a comment -

        I would love a unit test! they are the only sure way of preventing any
        regression in the code.

        Show
        David Eric Pugh added a comment - I would love a unit test! they are the only sure way of preventing any regression in the code.
        Hide
        woodman added a comment -

        I tested Oliver's suggestion, and that did the trick.

        <newbie_alert>
        Eric, do you still want a test case? Will one of you be making the fix?
        (Sorry, I'm new to the Apache world, so I'm not sure where the responsibility
        lies for these things.)
        </newbie_alert>

        Show
        woodman added a comment - I tested Oliver's suggestion, and that did the trick. <newbie_alert> Eric, do you still want a test case? Will one of you be making the fix? (Sorry, I'm new to the Apache world, so I'm not sure where the responsibility lies for these things.) </newbie_alert>
        Hide
        Oliver Heger added a comment -

        The problem seems to be in HierarchicalXMLConfiguration which also stores
        comments as configuration keys. In the constructHierarchy() method there is the
        statement

        else if (w3cNode instanceof CharacterData)

        IMO it should be sufficient to change this into

        else if (w3cNode instanceof Text)

        (because Comment is a subinterface of CharacterData, too).

        Oliver

        Show
        Oliver Heger added a comment - The problem seems to be in HierarchicalXMLConfiguration which also stores comments as configuration keys. In the constructHierarchy() method there is the statement else if (w3cNode instanceof CharacterData) IMO it should be sufficient to change this into else if (w3cNode instanceof Text) (because Comment is a subinterface of CharacterData, too). Oliver
        Hide
        David Eric Pugh added a comment -

        Hi, can you supply this bug as a testcase? That way we have something concrete
        to verify the fix against. Look at the other examples in /src/test.. in fact,
        if you can add it to TestHierarchialXmlConfiguration that would be great.

        Show
        David Eric Pugh added a comment - Hi, can you supply this bug as a testcase? That way we have something concrete to verify the fix against. Look at the other examples in /src/test.. in fact, if you can add it to TestHierarchialXmlConfiguration that would be great.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development