Commons Configuration
  1. Commons Configuration
  2. CONFIGURATION-121

[configuration] Included properties w/ relative path fails in v1.1

    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: Windows 2000
      Platform: PC

      Description

      The "include" property fails in 1.1 when a PropertiesConfiguration is
      instantiated with a relative path/file name instead of an absolute path, but it
      works correctly in version 1.0.

      As far as I can tell this bug was introduced by the refactoring of the load()
      behavior from the PropertiesConfiguration constructor to the constructor of
      AbstractFileConfiguration. The load() method relies on the instance variable
      "includesAllowed" which is declared and initialized in PropertiesConfiguration.
      However, because in v1.1 load() is invoked from the superclass constructor, the
      instance variables in the subclass have not yet been initialized when load() is
      invoked. Consequently "includesAllowed" evaluates to false and the include fails.

      I have attached a simple app to illustrate the issue. It attempts to load the
      property "bar" via an included properties file from the config dir. By
      manipulating the classpath in a batch script the app can be run twice, once
      using version 1.0 and once using 1.1. Here is sample output:

      Configuration version 1.0...
      file name = myapp.properties
      base path = file:/C:/myapp/config/
      includes? = true
      foo=foo from properties
      bar=included bar

      Configuration version 1.1...
      file name = myapp.properties
      base path = null
      includes? = true
      foo=foo from properties
      bar=null

      Note that the output contains "includes? = true" (which displays the result of
      PropertiesConfiguration.getIncludesAllowed()), but this occurs after the
      configuration has been completely initialized.

        Activity

        Hide
        Mark W added a comment -

        Created an attachment (id=15601)
        Sample app to illustrate reported bug.

        Show
        Mark W added a comment - Created an attachment (id=15601) Sample app to illustrate reported bug.
        Hide
        Oliver Heger added a comment -

        Mark,

        thanks for your analysis. You are right that the includesAllowed flag is not yet
        set when the load() method is called by the constructor.

        However PropertiesConfiguration overloads the setBasePath() method to set
        includesAllowed to true if a base path is available. This works if the
        properties are loaded from an absolute or relative path, but unfortunately not
        when loaded from the classpath as in your test application. This is the reason
        why our unit test for include properties works, though the file is loaded in the
        constructor (TestPropertiesConfiguration.setUp()).

        So the real problem seems to be once more the handling of the source file name
        and path. I tend to believe that we need a completely new solution (Locators!)
        to get rid off all of these problems.

        Thank you for spotting this.

        Show
        Oliver Heger added a comment - Mark, thanks for your analysis. You are right that the includesAllowed flag is not yet set when the load() method is called by the constructor. However PropertiesConfiguration overloads the setBasePath() method to set includesAllowed to true if a base path is available. This works if the properties are loaded from an absolute or relative path, but unfortunately not when loaded from the classpath as in your test application. This is the reason why our unit test for include properties works, though the file is loaded in the constructor (TestPropertiesConfiguration.setUp()). So the real problem seems to be once more the handling of the source file name and path. I tend to believe that we need a completely new solution (Locators!) to get rid off all of these problems. Thank you for spotting this.
        Hide
        Mark W added a comment -

        Oliver,

        Thanks for the clarification. You're right, I didn't dig deeply enough, I missed
        the override of setBasePath().

        FYI, as a consumer of the API I found it confusing that, after the constructor
        PropertiesConfiguration(String) is called, getIncludesAllowed() may not return
        the state that was in effect while load() was executing. This strikes me as
        suboptimal; perhaps the design of this component could be revisited.

        Show
        Mark W added a comment - Oliver, Thanks for the clarification. You're right, I didn't dig deeply enough, I missed the override of setBasePath(). FYI, as a consumer of the API I found it confusing that, after the constructor PropertiesConfiguration(String) is called, getIncludesAllowed() may not return the state that was in effect while load() was executing. This strikes me as suboptimal; perhaps the design of this component could be revisited.
        Hide
        Oliver Heger added a comment -

        I fully agree with you. It's a bug that the load() method is called before some
        member fields that can influence the load process have been initialized. I will
        see what I can do about it.

        The fact that the includesAllowed flag is modified by the setBasePath() method
        seems to me like a quick hack. Now the following can happen:

        1. A PropertiesConfiguration object is created
        2. setIncludesAllowed(false) is called on this object
        3. The configuration file is set (which invokes setBasePath)
        4. load() is called

        Step 3 might enable the includesAllowed flag again, so that the results might be
        different than expected. This behavior of setBasePath() is documented, but I
        think it is indeed very confusing and it should be possible to find a better
        solution.

        Would anybody object if I changed this? This will be an incompatible change and
        can have impact on existing code, but IMO it would make usage of this API clearer.

        Show
        Oliver Heger added a comment - I fully agree with you. It's a bug that the load() method is called before some member fields that can influence the load process have been initialized. I will see what I can do about it. The fact that the includesAllowed flag is modified by the setBasePath() method seems to me like a quick hack. Now the following can happen: 1. A PropertiesConfiguration object is created 2. setIncludesAllowed(false) is called on this object 3. The configuration file is set (which invokes setBasePath) 4. load() is called Step 3 might enable the includesAllowed flag again, so that the results might be different than expected. This behavior of setBasePath() is documented, but I think it is indeed very confusing and it should be possible to find a better solution. Would anybody object if I changed this? This will be an incompatible change and can have impact on existing code, but IMO it would make usage of this API clearer.
        Hide
        Oliver Heger added a comment -

        I committed a fix that should resolve the original problem. The main idea is
        that AbstractFileConfiguration in its load(URL) method now always sets a base
        path if none has been set before. The base path is simply set to the current
        source URL. This works for relative include paths because ConfigurationUtils is
        able to resolve a relative path in the context of a base URL.

        About my last comment: I think I was a bit on the wrong track. The original
        authors of PropertiesConfiguration did not support a way of disabling the
        includes feature. setIncludesAllowed() is protected, and the flag's value is
        only determined by the file to be loaded. I removed the initialization of the
        includesAllowed flag (it was set to true), so now isIncludesAllowed() will
        return the flag's value at the time the load() method was executed.

        Mark, can you check if now everything works for you?

        The question remains whether the includesAllowed flag makes sense any longer. My
        fix causes the base path to be always valid when load() was successful. So the
        flag will then always be true.

        Show
        Oliver Heger added a comment - I committed a fix that should resolve the original problem. The main idea is that AbstractFileConfiguration in its load(URL) method now always sets a base path if none has been set before. The base path is simply set to the current source URL. This works for relative include paths because ConfigurationUtils is able to resolve a relative path in the context of a base URL. About my last comment: I think I was a bit on the wrong track. The original authors of PropertiesConfiguration did not support a way of disabling the includes feature. setIncludesAllowed() is protected, and the flag's value is only determined by the file to be loaded. I removed the initialization of the includesAllowed flag (it was set to true), so now isIncludesAllowed() will return the flag's value at the time the load() method was executed. Mark, can you check if now everything works for you? The question remains whether the includesAllowed flag makes sense any longer. My fix causes the base path to be always valid when load() was successful. So the flag will then always be true.
        Hide
        Mark W added a comment -

        Thanks, Oliver, the change seems to fix this bug. Nice work!

        As an aside, I might suggest that the Javadoc for the PropertiesConfiguration
        no-arg constructor be changed, as it is possible to load included files after
        using the no-arg constructor:

        PropertiesConfiguration pc = new PropertiesConfiguration();
        pc.setPath("foo.properties");
        pc.load();

        ...assuming foo.properties has an include property, of course.

        -Mark

        Show
        Mark W added a comment - Thanks, Oliver, the change seems to fix this bug. Nice work! As an aside, I might suggest that the Javadoc for the PropertiesConfiguration no-arg constructor be changed, as it is possible to load included files after using the no-arg constructor: PropertiesConfiguration pc = new PropertiesConfiguration(); pc.setPath("foo.properties"); pc.load(); ...assuming foo.properties has an include property, of course. -Mark
        Hide
        Oliver Heger added a comment -

        I removed this sentence from the no-arg constructor. So this issue can be closed
        now.

        Show
        Oliver Heger added a comment - I removed this sentence from the no-arg constructor. So this issue can be closed now.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development