Commons Configuration
  1. Commons Configuration
  2. CONFIGURATION-80

[configuration] ConfigurationFactory not working as expected with include path resolution

    Details

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

      Operating System: All
      Platform: All

      Description

      Documentation states that ConfigurationFactory searches includes in a directory
      relative to the one including conf is located...

      Actually, some problem in various initializer methods of ConfigurationFactory
      makes it happen a way that is not natural.

      ConfigurationFactory() empty constructor sets the basePath to ".". This is okay,
      but when calling setConfigurationFileName,

      public void setConfigurationFileName(String configurationFileName)

      { File file = new File(configurationFileName).getAbsoluteFile(); this.configurationFileName = file.getName(); implicitBasePath = file.getParent(); }

      basePath is not reset to null... so a call to getBasePath will not return the
      implicitBasePath as stated.

      public String getBasePath()

      { String path = StringUtils.isEmpty(basePath) ? implicitBasePath : basePath; return StringUtils.isEmpty(path) ? "." : path; }

      using ConfigurationFactory(String configurationFileName) does not help, since it
      does not use setConfigurationFileName , so it does not set implicitBasePath

      public ConfigurationFactory(String configurationFileName)

      { this.configurationFileName = configurationFileName; }

      A work-around solution to these problems is to set the basePath to null
      explicitely after instanciation/setup...

      a possible correction is to modify the constructor and the setter like this:

      public ConfigurationFactory(String configurationFileName)

      { setConfigurationFileName(configurationFileName); }

      public void setConfigurationFileName(String configurationFileName)

      { File file = new File(configurationFileName).getAbsoluteFile(); this.configurationFileName = file.getName(); this.basePath = null; implicitBasePath = file.getParent(); }

        Activity

        Hide
        Oliver Heger added a comment -

        Fix applied and a new test case added. The test configuration files that rely on
        the wrong behavior were updated, too.

        Show
        Oliver Heger added a comment - Fix applied and a new test case added. The test configuration files that rely on the wrong behavior were updated, too.
        Hide
        Oliver Heger added a comment -

        I thought again about this problem and came to the same results. So thanks for
        the confirmation.

        Show
        Oliver Heger added a comment - I thought again about this problem and came to the same results. So thanks for the confirmation.
        Hide
        Jacob Kjome added a comment -

        Seems odd to me that one expect that file-based resources referenced from within
        a config.xml file would need to be specified with a full relative path from
        which the config.xml was originally loaded. But that seems to be what you say
        your unit tests expect. That's very weird to me. I wouldn't expect many people
        to have made that assumption. So, you are saying that you load up a config.xml
        file as such...

        ConfigurationFactory factory = new ConfigurationFactory("etc/props/config.xml");

        And then the config.xml would look something like this?...

        <configuration>
        <properties fileName="etc/props/usergui.properties"/>
        </configuration>

        First, I can't fathom anyone else making this assumption. Second, it makes the
        config.xml file location dependent. Put config.xml in any other path such as
        "etc/config" and all of a sudden, usergui.properties can't be found anymore
        without changing the file to look like...

        <configuration>
        <properties fileName="etc/config/usergui.properties"/>
        </configuration>

        Files referenced with a relative path within config.xml should be relative to
        wherever config.xml happens to be. That's clearly correct behavior and existing
        commons-configuration behavior is clearly incorrect. I don't think there is
        much to think about here except fixing the bug. Just make sure that the release
        notes make it very clear that this change has been made. Of course, for those
        following my suggestion in comment #3, any change in behavior will be entirely
        inconsequential, as opposed to the recommendation that one set the base path to
        null, which takes advantage of existing behavior, but doesn't hold up well if
        behavior changes.

        Jake

        Show
        Jacob Kjome added a comment - Seems odd to me that one expect that file-based resources referenced from within a config.xml file would need to be specified with a full relative path from which the config.xml was originally loaded. But that seems to be what you say your unit tests expect. That's very weird to me. I wouldn't expect many people to have made that assumption. So, you are saying that you load up a config.xml file as such... ConfigurationFactory factory = new ConfigurationFactory("etc/props/config.xml"); And then the config.xml would look something like this?... <configuration> <properties fileName="etc/props/usergui.properties"/> </configuration> First, I can't fathom anyone else making this assumption. Second, it makes the config.xml file location dependent. Put config.xml in any other path such as "etc/config" and all of a sudden, usergui.properties can't be found anymore without changing the file to look like... <configuration> <properties fileName="etc/config/usergui.properties"/> </configuration> Files referenced with a relative path within config.xml should be relative to wherever config.xml happens to be. That's clearly correct behavior and existing commons-configuration behavior is clearly incorrect. I don't think there is much to think about here except fixing the bug. Just make sure that the release notes make it very clear that this change has been made. Of course, for those following my suggestion in comment #3, any change in behavior will be entirely inconsequential, as opposed to the recommendation that one set the base path to null, which takes advantage of existing behavior, but doesn't hold up well if behavior changes. Jake
        Hide
        Oliver Heger added a comment -

        I had a look into this one and agree that there is a problem with the base path.

        However the proposed solution to set the base path to null whenever a file name
        was set would break the following use case:

        factory.setBasePath("my base path");
        factory.setConfigurationFileName("myFile.xml");

        I.e. if a base path was set first, a following call to
        setConfigurationFileName() must not override this path.

        What I have tested so far is to change the getBasePath() implementation to
        return the implicit base path (which is derived from the file name) if the base
        path is either null or ".". This should work in both cases.

        But after that a couple of unit tests are failing. Obviously some of our test
        files for ConfigurationFactory rely on this wrong behavior (these files include
        other files with a relative path like "conf/test.xml" instead of simply
        "test.xml" - and I sometimes asked myself why they did work).

        So the question is how to deal with this situation. The clean solution IMO would
        be to update the affected test files and make the change to
        ConfigurationFactory. But I am not sure about the impact for existing code.

        What do others think?

        Show
        Oliver Heger added a comment - I had a look into this one and agree that there is a problem with the base path. However the proposed solution to set the base path to null whenever a file name was set would break the following use case: factory.setBasePath("my base path"); factory.setConfigurationFileName("myFile.xml"); I.e. if a base path was set first, a following call to setConfigurationFileName() must not override this path. What I have tested so far is to change the getBasePath() implementation to return the implicit base path (which is derived from the file name) if the base path is either null or ".". This should work in both cases. But after that a couple of unit tests are failing. Obviously some of our test files for ConfigurationFactory rely on this wrong behavior (these files include other files with a relative path like "conf/test.xml" instead of simply "test.xml" - and I sometimes asked myself why they did work). So the question is how to deal with this situation. The clean solution IMO would be to update the affected test files and make the change to ConfigurationFactory. But I am not sure about the impact for existing code. What do others think?
        Hide
        Fabien Nisol added a comment -

        If you look into the code, calling factory.setBasePath(null) will have the same
        effect

        Fabien

        (In reply to comment #3)
        > This is the only problem I ran into using commons-configuration. To work around
        > it, I simply did something like...
        >
        > ConfigurationFactory factory = new ConfigurationFactory("etc/props/config.xml");
        > factory.setBasePath("etc/props");
        >
        > It would be nice not to have to make the setBasePath() call as it seems pretty
        > unnecessary. Seems like this should be an easy fix.
        >
        >
        > Jake

        Show
        Fabien Nisol added a comment - If you look into the code, calling factory.setBasePath(null) will have the same effect Fabien (In reply to comment #3) > This is the only problem I ran into using commons-configuration. To work around > it, I simply did something like... > > ConfigurationFactory factory = new ConfigurationFactory("etc/props/config.xml"); > factory.setBasePath("etc/props"); > > It would be nice not to have to make the setBasePath() call as it seems pretty > unnecessary. Seems like this should be an easy fix. > > > Jake
        Hide
        Jacob Kjome added a comment -

        This is the only problem I ran into using commons-configuration. To work around
        it, I simply did something like...

        ConfigurationFactory factory = new ConfigurationFactory("etc/props/config.xml");
        factory.setBasePath("etc/props");

        It would be nice not to have to make the setBasePath() call as it seems pretty
        unnecessary. Seems like this should be an easy fix.

        Jake

        Show
        Jacob Kjome added a comment - This is the only problem I ran into using commons-configuration. To work around it, I simply did something like... ConfigurationFactory factory = new ConfigurationFactory("etc/props/config.xml"); factory.setBasePath("etc/props"); It would be nice not to have to make the setBasePath() call as it seems pretty unnecessary. Seems like this should be an easy fix. Jake
        Hide
        Emmanuel Bourg added a comment -
            • COM-2147 has been marked as a duplicate of this bug. ***
        Show
        Emmanuel Bourg added a comment - COM-2147 has been marked as a duplicate of this bug. ***
        Hide
        Fabien Nisol added a comment -

        Created an attachment (id=15368)
        correction to 155408

        minor corrections to ConfigurationFactory(String) and
        setConfigurationFileName()

        Show
        Fabien Nisol added a comment - Created an attachment (id=15368) correction to 155408 minor corrections to ConfigurationFactory(String) and setConfigurationFileName()

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development