Details

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

      Operating System: other
      Platform: All

      Description

      I've attached a patch for the commons-configuration for some classes I've
      written to implement a DynaBean/DynaClass wrapper interface for a Configuration
      object.

        Activity

        Hide
        Ricardo Gladwell added a comment -

        Created an attachment (id=12373)
        Patch file proposed.

        Show
        Ricardo Gladwell added a comment - Created an attachment (id=12373) Patch file proposed.
        Hide
        David Eric Pugh added a comment -

        Hi,

        This is looking really good. Could you also submit some unit tests? If you
        look at the other configurations you can see how they have fairly comprehensive
        test suites. Look at DataConfigurationTest for example. Right now the
        JCoverage tool says Configuraiton is at 88% which is a nice place to be!

        Also, what do you think about putting the support classes in o.a.c.c.dynabean?
        or putting all of it in .dynabean? Also, is this basically two patches? One
        for dynabean, and one for the map to be used in expressions?

        Unit tests also help people like me understand how to use the new code!

        Eric

        Show
        David Eric Pugh added a comment - Hi, This is looking really good. Could you also submit some unit tests? If you look at the other configurations you can see how they have fairly comprehensive test suites. Look at DataConfigurationTest for example. Right now the JCoverage tool says Configuraiton is at 88% which is a nice place to be! Also, what do you think about putting the support classes in o.a.c.c.dynabean? or putting all of it in .dynabean? Also, is this basically two patches? One for dynabean, and one for the map to be used in expressions? Unit tests also help people like me understand how to use the new code! Eric
        Hide
        Ricardo Gladwell added a comment -

        Created an attachment (id=12403)
        dynabeans classes now in own package with complete junit tests

        Show
        Ricardo Gladwell added a comment - Created an attachment (id=12403) dynabeans classes now in own package with complete junit tests
        Hide
        David Eric Pugh added a comment -

        Another great patch! If you don't mind, bumping the test coverage would be
        nice as well.. I'll be updating the website w/ the latests maven generated docs!

        Show
        David Eric Pugh added a comment - Another great patch! If you don't mind, bumping the test coverage would be nice as well.. I'll be updating the website w/ the latests maven generated docs!
        Hide
        Ricardo Gladwell added a comment -

        I'm not sure how the JCoverage utility works. When you say "bumping the test
        coverage would be nice as well" do you mean rename some of the test methods so
        they explicitly match method names in the tested class?

        Show
        Ricardo Gladwell added a comment - I'm not sure how the JCoverage utility works. When you say "bumping the test coverage would be nice as well" do you mean rename some of the test methods so they explicitly match method names in the tested class?
        Hide
        David Eric Pugh added a comment -

        If you are using Maven, run Maven site. One of the things it does is runs a
        report called JCoverage that highlights which code has been unit tested. Here
        is a link the online version:
        http://jakarta.apache.org/commons/configuration/jcoverage/index.html.

        Right now some of the code has methods that haven't been heavily tested.
        According to Jcoverage, many of the lines in the set method of
        ConfigurationDynaBean haven't been called via a unit test.

        While not perfect, it is one yardstick to judge the completeness of the unit
        tests. I'll upload the current site once I finish up with your patches.

        Show
        David Eric Pugh added a comment - If you are using Maven, run Maven site. One of the things it does is runs a report called JCoverage that highlights which code has been unit tested. Here is a link the online version: http://jakarta.apache.org/commons/configuration/jcoverage/index.html . Right now some of the code has methods that haven't been heavily tested. According to Jcoverage, many of the lines in the set method of ConfigurationDynaBean haven't been called via a unit test. While not perfect, it is one yardstick to judge the completeness of the unit tests. I'll upload the current site once I finish up with your patches.
        Hide
        Ricardo Gladwell added a comment -

        Created an attachment (id=12444)
        Patch with 90%+ test coverage

        Show
        Ricardo Gladwell added a comment - Created an attachment (id=12444) Patch with 90%+ test coverage
        Hide
        Emmanuel Bourg added a comment -

        Reopening until this configuration is fully tested.

        Show
        Emmanuel Bourg added a comment - Reopening until this configuration is fully tested.
        Hide
        Ricardo Gladwell added a comment -

        I have a question about the behavior of ConfigurationDynaBean.
        I'm not sure what the intended behavior should be, but looking at the
        code in the CVS repository makes me think the intent is different from
        the current behavior. However, it appears this is fairly new code, so
        I don't know if the problem is in the code behavior or if I am just
        misinterpreting the intent from the code.

        In looking at the code, it appears the intent was for the get method
        to throw an IllegalArgumentException in the case where the desired
        property does not exist in the backing Configuration instance. This
        would cause the method to meet the specification of get in the
        DynaBean interface. However, get only throws the exception if the
        specified name is null. If the name is non-null but does not exist,
        then an empty ConfigurationDynaBean is returned.

        The reason for this is in the call to configuration.subset in the get
        method. The relevant code is shown below (this is version 1.2 from the
        CVS repository):

        // get configuration property
        Object result = configuration.getProperty(name);
        if(result == null && name != null)

        { // otherwise attempt to create bean from configuration subset Configuration subset = configuration.subset(name); if(subset != null) result = new ConfigurationDynaBean(configuration.subset(name)); }

        If the extracted subset is not null, then a new ConfigurationDynaBean
        is constructed from that subset. However, I don't think
        Configuration.subset ever returns null. If no properties from the
        configuration begin with the specified prefix, then the return value
        from subset is an empty Configuration, not null (this is true even if
        the prefix argument is null). Therefore, the "if(subset != null)" test
        always succeeds in the above code, and result is always set to a new
        ConfigurationDynaBean.

        The only way result will remain null is if it is set to null in the
        initial configuration.getProperty() call, and then if it bypasses the
        if-clause by having (name == null). Therefore, that is the only case
        where the exception is thrown.

        I didn't notice any JUnit test cases for checking the behavior when
        accessing a non-existing property name (although there is a case for a
        null name).

        So, my question is: should the get method throw an exception when
        called with a non-existent property name? If so, then the get method
        needs to be changed. If not, I might recommend documenting this
        difference from the DynaBean interface in some way, and perhaps
        changing the above code to remove the superfluous if-test. I'd be
        happy to assist if needed.


        Bill Vollers

        Show
        Ricardo Gladwell added a comment - I have a question about the behavior of ConfigurationDynaBean. I'm not sure what the intended behavior should be, but looking at the code in the CVS repository makes me think the intent is different from the current behavior. However, it appears this is fairly new code, so I don't know if the problem is in the code behavior or if I am just misinterpreting the intent from the code. In looking at the code, it appears the intent was for the get method to throw an IllegalArgumentException in the case where the desired property does not exist in the backing Configuration instance. This would cause the method to meet the specification of get in the DynaBean interface. However, get only throws the exception if the specified name is null. If the name is non-null but does not exist, then an empty ConfigurationDynaBean is returned. The reason for this is in the call to configuration.subset in the get method. The relevant code is shown below (this is version 1.2 from the CVS repository): // get configuration property Object result = configuration.getProperty(name); if(result == null && name != null) { // otherwise attempt to create bean from configuration subset Configuration subset = configuration.subset(name); if(subset != null) result = new ConfigurationDynaBean(configuration.subset(name)); } If the extracted subset is not null, then a new ConfigurationDynaBean is constructed from that subset. However, I don't think Configuration.subset ever returns null. If no properties from the configuration begin with the specified prefix, then the return value from subset is an empty Configuration, not null (this is true even if the prefix argument is null). Therefore, the "if(subset != null)" test always succeeds in the above code, and result is always set to a new ConfigurationDynaBean. The only way result will remain null is if it is set to null in the initial configuration.getProperty() call, and then if it bypasses the if-clause by having (name == null). Therefore, that is the only case where the exception is thrown. I didn't notice any JUnit test cases for checking the behavior when accessing a non-existing property name (although there is a case for a null name). So, my question is: should the get method throw an exception when called with a non-existent property name? If so, then the get method needs to be changed. If not, I might recommend documenting this difference from the DynaBean interface in some way, and perhaps changing the above code to remove the superfluous if-test. I'd be happy to assist if needed. – Bill Vollers
        Hide
        Ricardo Gladwell added a comment -

        Created an attachment (id=12545)
        Fix for bug voller indicated.

        Show
        Ricardo Gladwell added a comment - Created an attachment (id=12545) Fix for bug voller indicated.
        Hide
        Ricardo Gladwell added a comment -

        Could someone submit my latest patches for this?

        Show
        Ricardo Gladwell added a comment - Could someone submit my latest patches for this?
        Hide
        David Eric Pugh added a comment -

        Can you double check CVS head? I think I actually had applied this patch file
        already.. At least, looking at CVS, it appears so.. I'm closing this bug
        again, can you reopen if anything else is needed?

        Show
        David Eric Pugh added a comment - Can you double check CVS head? I think I actually had applied this patch file already.. At least, looking at CVS, it appears so.. I'm closing this bug again, can you reopen if anything else is needed?
        Hide
        Ricardo Gladwell added a comment -

        I don't think patches 12444 and 12545 have been applied.

        Show
        Ricardo Gladwell added a comment - I don't think patches 12444 and 12545 have been applied.
        Hide
        David Eric Pugh added a comment -

        Alright.. Hopefully this time we can close this guy! I had applied half the
        patches, but not the other half.. additionally, some wouldn't apply cleanly, so
        I ended up doing it manually. However, all the unit tests (old and new) now pass.

        Can you verify it one last time and we can put this to bed? Thanks for the
        patience.

        Show
        David Eric Pugh added a comment - Alright.. Hopefully this time we can close this guy! I had applied half the patches, but not the other half.. additionally, some wouldn't apply cleanly, so I ended up doing it manually. However, all the unit tests (old and new) now pass. Can you verify it one last time and we can put this to bed? Thanks for the patience.
        Hide
        Ricardo Gladwell added a comment -

        Verified, ta

        Show
        Ricardo Gladwell added a comment - Verified, ta

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development