Commons Configuration
  1. Commons Configuration
  2. CONFIGURATION-15

[configuration] PropertyConfiguration.save() does not take basePath into account

    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: other
      Platform: Other

      Description

      When using ConfigurationFactory with a config.xml file that contans multiple
      properties files one can iterate through is and get each Configuration. You can
      then use one of these (Typecasting it to the proper type) directly. However,
      when for instance trying to do saveProperty with the PropertiesConfiguration it
      uses the fileName property instead of the basePath (which contains the absolute
      reference) from its superclass. This results in the file being created at
      whatever the current path is, instead of where it actually got the file in the
      first place. Propesed fix is :
      /**

      • Save the configuration to the file specified by the fileName attribute.
        */
        public void save() throws ConfigurationException { // save(fileName); save(getBasePath()); }

        Activity

        Error rendering 'com.atlassian.jirafisheyeplugin:fisheye-issuepanel'. Please contact your JIRA administrators.

        Henri Yandell made changes -
        Affects Version/s Nightly Builds [ 12311710 ]
        Henri Yandell made changes -
        Key COM-1508 CONFIGURATION-15
        Affects Version/s Nightly Builds [ 12311648 ]
        Project Commons [ 12310458 ] Commons Configuration [ 12310467 ]
        Component/s Configuration [ 12311107 ]
        Assignee Jakarta Commons Developers Mailing List [ commons-dev@jakarta.apache.org ]
        Henri Yandell made changes -
        Field Original Value New Value
        issue.field.bugzillaimportkey 30858 12341660
        Hide
        Oliver Heger added a comment -

        I have performed a little cleanup on the way the base path was handled. The
        internal url field was removed. I added some more tests and improved the
        javadoc. The unit tests all run, but I had a problem with the test for
        FileChangedReloadingStrategy: To get it running after the modifications I had to
        increase the sleep interval in testAutomaticReloading(). Don't know why, but on
        my old and slow Linux box this test was also sometimes failing before the update.

        The tests indicate that the problem is now solved (refer to
        TestFileConfiguration). So I mark this bug as fixed. Feel free to reopen if
        there are still issues.

        Show
        Oliver Heger added a comment - I have performed a little cleanup on the way the base path was handled. The internal url field was removed. I added some more tests and improved the javadoc. The unit tests all run, but I had a problem with the test for FileChangedReloadingStrategy: To get it running after the modifications I had to increase the sleep interval in testAutomaticReloading(). Don't know why, but on my old and slow Linux box this test was also sometimes failing before the update. The tests indicate that the problem is now solved (refer to TestFileConfiguration). So I mark this bug as fixed. Feel free to reopen if there are still issues.
        Hide
        Oliver Heger added a comment -

        ATM the internal url is not always updated, so there are inconsitencies. For me
        to fix this, it is easier to remove the field at all than to find all places
        where an update should take place. This will also reduce code and concentrate
        logic. I think, the URL is only referred in some of the load() methods.

        Show
        Oliver Heger added a comment - ATM the internal url is not always updated, so there are inconsitencies. For me to fix this, it is easier to remove the field at all than to find all places where an update should take place. This will also reduce code and concentrate logic. I think, the URL is only referred in some of the load() methods.
        Hide
        Emmanuel Bourg added a comment -

        I don't see how removing the internal URL will help, updating it when the
        basepath/filename changes or computing it when the getter is accessed is equivalent.

        Show
        Emmanuel Bourg added a comment - I don't see how removing the internal URL will help, updating it when the basepath/filename changes or computing it when the getter is accessed is equivalent.
        Hide
        Oliver Heger added a comment -

        Well, I will try to do the following: get rid of the internal url field in
        AbstractFileConfiguration. Everywhere the url field is accessed, it can be
        calculated from the base path and file name. The base path can then contain a
        full URL, which has to be taken into account when constructing a File object in
        save() operations.

        So this modification won't influence the class's interface. Where to put the
        utility methods for translating from URLs to Files and vice versa is an
        interesting question. I don't think that a static utility class is such a bad
        idea because this makes them easier to access (maybe in future other parts of
        [configuration] needs this stuff, too?).

        Show
        Oliver Heger added a comment - Well, I will try to do the following: get rid of the internal url field in AbstractFileConfiguration. Everywhere the url field is accessed, it can be calculated from the base path and file name. The base path can then contain a full URL, which has to be taken into account when constructing a File object in save() operations. So this modification won't influence the class's interface. Where to put the utility methods for translating from URLs to Files and vice versa is an interesting question. I don't think that a static utility class is such a bad idea because this makes them easier to access (maybe in future other parts of [configuration] needs this stuff, too?).
        Hide
        Emmanuel Bourg added a comment -

        (In reply to comment #5)
        > What can we do about it? I would suggest dropping (deprecating) either the
        > get/setURL() or get/setBasePath() methods. The remaining methods should be able
        > to deal with both URLs and files. This could be implemented in
        > ConfigurationUtils, in a central place. Are there any other ideas/opinions?

        The basepath stuff is quite complicated I agree and it's not a part I like to
        deal with, however it's still a useful feature and I believe we should keep it.
        I'm not favorable to drop the URL, it's the reference to locate the file, I see
        the basepath and the filename as byproducts of the URL.

        I placed the utility methods getBasePath and getFileName in ConfigurationUtils
        but this may not be the best place for them. The logic dealing with paths and
        urls may be best placed in AbstractFileConfiguration.

        Show
        Emmanuel Bourg added a comment - (In reply to comment #5) > What can we do about it? I would suggest dropping (deprecating) either the > get/setURL() or get/setBasePath() methods. The remaining methods should be able > to deal with both URLs and files. This could be implemented in > ConfigurationUtils, in a central place. Are there any other ideas/opinions? The basepath stuff is quite complicated I agree and it's not a part I like to deal with, however it's still a useful feature and I believe we should keep it. I'm not favorable to drop the URL, it's the reference to locate the file, I see the basepath and the filename as byproducts of the URL. I placed the utility methods getBasePath and getFileName in ConfigurationUtils but this may not be the best place for them. The logic dealing with paths and urls may be best placed in AbstractFileConfiguration.
        Hide
        Oliver Heger added a comment -

        I wrote a test case for saving a file based configuration with a base path, and
        this still fails. The test should reproduce a situation as it occurs in
        ConfigurationFactory: When a file based configuration is loaded, first its
        setBasePath() method gets called and then setFileName(). To the former a URL may
        be passed. If later the file based configuration should be saved using the
        save() method or save(String) with a relative file name, conversion of this URL
        to a File object fails.

        A solution would be to make ConfigurationUtils.constructFile() aware of URLs, so
        that a correct File object is constructed even if URLs are involved (given that
        all URLs have the file protocol).

        After looking at the code for a while I find our actual solution for dealing
        with file names, base paths and URLs very confusing. The fact that we
        distinguish between a base path and an URL makes the whole stuff hard to
        maintain and error prone (just think about the many places in
        ConfigurationFactory, AbstractFileConfiguration and elsewhere where special
        checks are performed). Besides it is hard for the users to understand.

        What can we do about it? I would suggest dropping (deprecating) either the
        get/setURL() or get/setBasePath() methods. The remaining methods should be able
        to deal with both URLs and files. This could be implemented in
        ConfigurationUtils, in a central place. Are there any other ideas/opinions?

        Show
        Oliver Heger added a comment - I wrote a test case for saving a file based configuration with a base path, and this still fails. The test should reproduce a situation as it occurs in ConfigurationFactory: When a file based configuration is loaded, first its setBasePath() method gets called and then setFileName(). To the former a URL may be passed. If later the file based configuration should be saved using the save() method or save(String) with a relative file name, conversion of this URL to a File object fails. A solution would be to make ConfigurationUtils.constructFile() aware of URLs, so that a correct File object is constructed even if URLs are involved (given that all URLs have the file protocol). After looking at the code for a while I find our actual solution for dealing with file names, base paths and URLs very confusing. The fact that we distinguish between a base path and an URL makes the whole stuff hard to maintain and error prone (just think about the many places in ConfigurationFactory, AbstractFileConfiguration and elsewhere where special checks are performed). Besides it is hard for the users to understand. What can we do about it? I would suggest dropping (deprecating) either the get/setURL() or get/setBasePath() methods. The remaining methods should be able to deal with both URLs and files. This could be implemented in ConfigurationUtils, in a central place. Are there any other ideas/opinions?
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=13583)
        Test case for saving with base path

        Show
        Oliver Heger added a comment - Created an attachment (id=13583) Test case for saving with base path
        Hide
        Emmanuel Bourg added a comment -

        Hermod, the persistence mechanism of file based configurations has been reworked
        in RC2, could you test again and tell us if it fixed your issue ?

        Show
        Emmanuel Bourg added a comment - Hermod, the persistence mechanism of file based configurations has been reworked in RC2, could you test again and tell us if it fixed your issue ?
        Hide
        Oliver Heger added a comment -

        The basePath can indeed contain a full URL, and not necessarily a file URL. Only
        removing the file: prefix will probably not work in all constellations (just
        consider that in windows environments the backslash is used as path separator).

        I would suggest to implement the save() method the same way as the load()
        method, i.e. obtaining the URL for storing from ConfigurationUtils (which takes
        the base path into account and can deal with both absolute and relative URLs)
        and then opening an OutputStream on this URL.

        Oliver

        Show
        Oliver Heger added a comment - The basePath can indeed contain a full URL, and not necessarily a file URL. Only removing the file: prefix will probably not work in all constellations (just consider that in windows environments the backslash is used as path separator). I would suggest to implement the save() method the same way as the load() method, i.e. obtaining the URL for storing from ConfigurationUtils (which takes the base path into account and can deal with both absolute and relative URLs) and then opening an OutputStream on this URL. Oliver
        Hide
        Hermod Opstvedt added a comment -

        I discovered that the basePath can contain 'file:/' in the beginning, so I
        propose to add a new method to BasePathConfiguration :

        /**

        • Returns the Base path from which this Configuration Factory operates.
        • This is never null. If you set the BasePath to null, then "."
        • is returned. If the basePath is prefixed with a protocol then remove it
          *
        • @return The base Path of this configuration factory.
          */
          public String getAbsoluteBasePath()
          Unknown macro: { if(basePath.indexOf("file}

        and then in PropertiesConfiguration :

        /**

        • Save the configuration to the file specified by the fileName attribute.
          */
          public void save() throws ConfigurationException { // save(fileName); save(getAbsoluteBasePath()); }

        Again : I don't have CVS access, so I submit it here

        Hermod

        Show
        Hermod Opstvedt added a comment - I discovered that the basePath can contain 'file:/' in the beginning, so I propose to add a new method to BasePathConfiguration : /** Returns the Base path from which this Configuration Factory operates. This is never null. If you set the BasePath to null, then "." is returned. If the basePath is prefixed with a protocol then remove it * @return The base Path of this configuration factory. */ public String getAbsoluteBasePath() Unknown macro: { if(basePath.indexOf("file} and then in PropertiesConfiguration : /** Save the configuration to the file specified by the fileName attribute. */ public void save() throws ConfigurationException { // save(fileName); save(getAbsoluteBasePath()); } Again : I don't have CVS access, so I submit it here Hermod
        Hermod Opstvedt created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development