Details

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

      Operating System: All
      Platform: All

      Description

      ConfigurationFactory should support optional configuration files, as suggested
      in COM-1190. It could be implemented with a "required" attribute in the
      configuration descriptor, for example:

      <configuration>
      <properties fileName="test.properties" required="false"/>
      <xml fileName="test.xml" required="true"/>
      </configuration>

        Activity

        Hide
        Oliver Heger added a comment -

        Created an attachment (id=13274)
        Patch for ConfigurationFactory to enable the required attribute

        Show
        Oliver Heger added a comment - Created an attachment (id=13274) Patch for ConfigurationFactory to enable the required attribute
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=13275)
        Additional test for non required configuration sources

        Show
        Oliver Heger added a comment - Created an attachment (id=13275) Additional test for non required configuration sources
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=13276)
        Test configuration definition file with non required configurations; to be added to the conf directory

        Show
        Oliver Heger added a comment - Created an attachment (id=13276) Test configuration definition file with non required configurations; to be added to the conf directory
        Hide
        Oliver Heger added a comment -

        The attached patches implement the proposed required attribute. If present with
        a boolean value of false, an exception caused by loading the corresponding
        configuration source will be ignored. I also added a few log statements which
        could be helpful for debugging.

        Oliver

        Show
        Oliver Heger added a comment - The attached patches implement the proposed required attribute. If present with a boolean value of false, an exception caused by loading the corresponding configuration source will be ignored. I also added a few log statements which could be helpful for debugging. Oliver
        Hide
        Oliver Heger added a comment -

        Patch applied. I leave this open for discussion. The howtos for
        ConfigurationFactory need to be updated, too. I can do this if everybody is
        happy with this patch.

        Oliver

        Show
        Oliver Heger added a comment - Patch applied. I leave this open for discussion. The howtos for ConfigurationFactory need to be updated, too. I can do this if everybody is happy with this patch. Oliver
        Hide
        Emmanuel Bourg added a comment -

        This looks good, thank you Oliver. I don't know if the name of the attribute is
        a good choice, we could have used optional="true" instead of required="false", I
        have no personnal preference.

        Show
        Emmanuel Bourg added a comment - This looks good, thank you Oliver. I don't know if the name of the attribute is a good choice, we could have used optional="true" instead of required="false", I have no personnal preference.
        Hide
        Oliver Heger added a comment -

        The attribute name "optional" was indeed my first choice, but then I adapted it
        to the proposal in the bug description. Well, this is easy enough to change:
        just modify a string constant and negate a condition.

        Show
        Oliver Heger added a comment - The attribute name "optional" was indeed my first choice, but then I adapted it to the proposal in the bug description. Well, this is easy enough to change: just modify a string constant and negate a condition.
        Hide
        yathish added a comment -

        There is a bug in Jakartha commons-net library.I used it to check whether the
        file in the ftpserver is a file or not.
        The bug is that if the directory name is separated by spaces then the file is
        not detected.
        Eg:/lib/lib stand/ is not detected and throws an error saying unspecified file .

        Show
        yathish added a comment - There is a bug in Jakartha commons-net library.I used it to check whether the file in the ftpserver is a file or not. The bug is that if the directory name is separated by spaces then the file is not detected. Eg:/lib/lib stand/ is not detected and throws an error saying unspecified file .
        Hide
        Emmanuel Bourg added a comment -

        yathish I think you changed the wrong bug

        All configurations are optional by default ?

        Show
        Emmanuel Bourg added a comment - yathish I think you changed the wrong bug All configurations are optional by default ?
        Hide
        David Eric Pugh added a comment -

        I think the optional="true" is a bit clearer. The default behavior is
        required, so we should highlight in the config file that we have changed the
        default, which option="true" makes a bit clearer...

        just my 2 cents!

        Show
        David Eric Pugh added a comment - I think the optional="true" is a bit clearer. The default behavior is required, so we should highlight in the config file that we have changed the default, which option="true" makes a bit clearer... just my 2 cents!
        Hide
        Oliver Heger added a comment -

        No, per default all configurations are required, so that if one cannot be found,
        a ConfigurationException will be thrown. If the "required" attribute is
        specified with a value of false, occurring exceptions are just logged and then
        ignored.

        I think, I like the name "optional" for the attribute better. Because optional
        configurations are an additional feature a developer must turn it on by
        specifying optional="true". This seems to be more logical than disabling a
        default behaviour by writing required="false".

        So if all are happy with the name "optional" I will change this and add
        something to the ConfigurationFactory howtos.

        Show
        Oliver Heger added a comment - No, per default all configurations are required, so that if one cannot be found, a ConfigurationException will be thrown. If the "required" attribute is specified with a value of false, occurring exceptions are just logged and then ignored. I think, I like the name "optional" for the attribute better. Because optional configurations are an additional feature a developer must turn it on by specifying optional="true". This seems to be more logical than disabling a default behaviour by writing required="false". So if all are happy with the name "optional" I will change this and add something to the ConfigurationFactory howtos.
        Hide
        Emmanuel Bourg added a comment -

        Weird, I made a test yesterday without specifying the required attribute and I
        just got a warning. I'll try again.

        I think it's more natural to add an attribute when we intend to enable a
        property (optional or required), i.e. setting the attribute to "true". So the
        right attribute name depends on the default behaviour, if all files are required
        by default it's logical to use "optional=true", and if all files are optional by
        default, it's logical to use "required=true".

        So, should all files be required or optional by default ? What is the most
        common use case ?

        Show
        Emmanuel Bourg added a comment - Weird, I made a test yesterday without specifying the required attribute and I just got a warning. I'll try again. I think it's more natural to add an attribute when we intend to enable a property (optional or required), i.e. setting the attribute to "true". So the right attribute name depends on the default behaviour, if all files are required by default it's logical to use "optional=true", and if all files are optional by default, it's logical to use "required=true". So, should all files be required or optional by default ? What is the most common use case ?
        Hide
        Oliver Heger added a comment -

        Good question. IMO most configurations are required. Exceptions are things like
        personal user settings which may or may not be defined. So I would suggest to
        define required configurations as default and call the attribute "optional".

        Another point is backwards compatibilty. ConfigurationFactory used to throw an
        exception if a file could not be found. Don't know which impact it would have on
        existing applications if now only a warning was logged.

        I hope I did not break this mechanism. I did not add a specific test case for
        non existing files that are required because I think other tests in
        TestConfigurationFactory would do this.

        Show
        Oliver Heger added a comment - Good question. IMO most configurations are required. Exceptions are things like personal user settings which may or may not be defined. So I would suggest to define required configurations as default and call the attribute "optional". Another point is backwards compatibilty. ConfigurationFactory used to throw an exception if a file could not be found. Don't know which impact it would have on existing applications if now only a warning was logged. I hope I did not break this mechanism. I did not add a specific test case for non existing files that are required because I think other tests in TestConfigurationFactory would do this.
        Hide
        Oliver Heger added a comment -

        I renamed the attribute from "required" to "optional". There is an additional
        test case that tests if non existing files without the "optional" attribute
        cause an exception. A subsection about optional configuration sources was added
        to the ConfigurationFactory howto.

        Show
        Oliver Heger added a comment - I renamed the attribute from "required" to "optional". There is an additional test case that tests if non existing files without the "optional" attribute cause an exception. A subsection about optional configuration sources was added to the ConfigurationFactory howto.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development