Commons Configuration
  1. Commons Configuration
  2. CONFIGURATION-86

[configuration] NullPointerException thrown by AbstractFileConfiguration.getFile()

    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: All
      Platform: All

      Description

      The getFile() method throws a NullPointerException if no file name has been set
      yet. This can also happen after a file has been successfully loaded because most
      of the overloaded load() methods do not store the passed in file name.

      This issue was reported by Mi Zhang on commons user list (see
      http://thread.gmane.org/gmane.comp.jakarta.commons.user/12289).

        Activity

        Hide
        Emmanuel Bourg added a comment -

        The NullPointerException is a bug, but getFile() should return null in this
        case. The load(String filename) method should not store the file in the
        configuration.

        Show
        Emmanuel Bourg added a comment - The NullPointerException is a bug, but getFile() should return null in this case. The load(String filename) method should not store the file in the configuration.
        Hide
        Oliver Heger added a comment -

        Not sure whether getFile() should return null after a file was loaded. What
        about the following:

        FileConfiguration conf = new PropertiesConfiguration();
        conf.load("myfile.properties");
        ...
        conf.save();

        ATM this code will work and store the data at the same place where it was loaded
        from (because the source URL is now stored). This is okay for me, it's what I
        expect. So there is an association between the configuration and the file it was
        loaded from. Why not return this file in the getFile() method and its name in
        the getFileName() method?

        Show
        Oliver Heger added a comment - Not sure whether getFile() should return null after a file was loaded. What about the following: FileConfiguration conf = new PropertiesConfiguration(); conf.load("myfile.properties"); ... conf.save(); ATM this code will work and store the data at the same place where it was loaded from (because the source URL is now stored). This is okay for me, it's what I expect. So there is an association between the configuration and the file it was loaded from. Why not return this file in the getFile() method and its name in the getFileName() method?
        Hide
        Emmanuel Bourg added a comment -

        That's not how I initially designed the FileConfiguration, the idea was to
        attach the configuration to the file only by using the constructors or calling
        setFile/setFilename. The load/save methods were not supposed to change the file
        of the configuration, my bad for not writing a test covering this case.

        Show
        Emmanuel Bourg added a comment - That's not how I initially designed the FileConfiguration, the idea was to attach the configuration to the file only by using the constructors or calling setFile/setFilename. The load/save methods were not supposed to change the file of the configuration, my bad for not writing a test covering this case.
        Hide
        Oliver Heger added a comment -

        If this is the desired behavior, it should not be a problem to achieve it.

        That the file name won't be changed is documented for the save() methods, but
        not for the load() methods, so there is room for confusion. The user who
        initiated this request was obviously on the opinion that after a load the file
        name would be available. For me this would make sense, too.

        Could you please in short explain your motivation why the load() methods should
        not change the file name?

        The only incosistency I can see ATM is that load() can be called multiple times
        constructing a union configuration. Then the first call would determine the
        stored file name. But IMO that's not too bad.

        Show
        Oliver Heger added a comment - If this is the desired behavior, it should not be a problem to achieve it. That the file name won't be changed is documented for the save() methods, but not for the load() methods, so there is room for confusion. The user who initiated this request was obviously on the opinion that after a load the file name would be available. For me this would make sense, too. Could you please in short explain your motivation why the load() methods should not change the file name? The only incosistency I can see ATM is that load() can be called multiple times constructing a union configuration. Then the first call would determine the stored file name. But IMO that's not too bad.
        Hide
        Oliver Heger added a comment -

        I committed a fix, which avoids the NPE. getFile() will return null as long as
        no file has been set using one of the setter methods, even when a file has been
        loaded. So this is inline to comment #1.

        Show
        Oliver Heger added a comment - I committed a fix, which avoids the NPE. getFile() will return null as long as no file has been set using one of the setter methods, even when a file has been loaded. So this is inline to comment #1.
        Hide
        Emmanuel Bourg added a comment -

        (In reply to comment #4)
        > Could you please in short explain your motivation why the load() methods should
        > not change the file name?

        The motivation was to remain symmetric with the semantic of save(), and saving
        without changing the file was a requirement to copy easily a configuration. I
        believe it's also important to have basic methods with the simplest semantic
        possible, that's why it just loads from the source, and not load and set the
        reference to the source. This gives a greater flexibility for composing the
        methods and achieving a more complex behaviour.

        Thank you for the fix Oliver.

        Show
        Emmanuel Bourg added a comment - (In reply to comment #4) > Could you please in short explain your motivation why the load() methods should > not change the file name? The motivation was to remain symmetric with the semantic of save(), and saving without changing the file was a requirement to copy easily a configuration. I believe it's also important to have basic methods with the simplest semantic possible, that's why it just loads from the source, and not load and set the reference to the source. This gives a greater flexibility for composing the methods and achieving a more complex behaviour. Thank you for the fix Oliver.
        Hide
        Oliver Heger added a comment -

        Accepted. Then I will do the following:

        • Add a note to the javadoc of the load() methods that they do not change the
          file name (analogous to the save() methods).
        • Update the save(void) method to throw an exception if no file name has been
          set. This behavior should be compatible to the 1.1 release.
        • Add some tests to verify and document this behavior.

        WDYT?

        Show
        Oliver Heger added a comment - Accepted. Then I will do the following: Add a note to the javadoc of the load() methods that they do not change the file name (analogous to the save() methods). Update the save(void) method to throw an exception if no file name has been set. This behavior should be compatible to the 1.1 release. Add some tests to verify and document this behavior. WDYT?
        Hide
        Emmanuel Bourg added a comment -

        That sounds good

        Show
        Emmanuel Bourg added a comment - That sounds good
        Hide
        Oliver Heger added a comment -

        Done! So I am closing this issue now.

        Show
        Oliver Heger added a comment - Done! So I am closing this issue now.

          People

          • Assignee:
            Unassigned
            Reporter:
            Oliver Heger
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development