Commons Configuration
  1. Commons Configuration
  2. CONFIGURATION-127

[configuration] Lists in a CompositeConfiguration

    Details

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

      Operating System: All
      Platform: All

      Description

      Not sure whether this is a bug, but at least it's an issue where the
      documentation differs from the actul behavior:

      The CompositeConfiguration javadoc states:
      "If you add Configuration1, and then Configuration2, any properties shared will
      mean that Configuration1 will be returned.... If Configuration1 doesn't have the
      property, then Configuration2 will be checked."

      So this would mean, if a use getList or getStringArray I only get the list
      elements from conf1 but not those from conf2, if the requested property is
      contained in both. In reality, however, I get a combined list of the values
      specified in conf1 + conf2 + any others.

      I think it would be better to behave according to the documentation and to
      return only the elements from the first matching configuration + any dynamically
      added elements from the InMemoryConfiguration.

      So, is this a bug, and will it be fixed?

        Activity

        Hide
        David Eric Pugh added a comment -

        Can you submit a unit test that demonstrates the problem? Basically getList
        should work just like getString does.. So, if you declared that conf 1
        overrides conf 2, then you would get conf1.. if you said conf 1 was additive
        to conf2, then you would get both..

        Eric

        Show
        David Eric Pugh added a comment - Can you submit a unit test that demonstrates the problem? Basically getList should work just like getString does.. So, if you declared that conf 1 overrides conf 2, then you would get conf1.. if you said conf 1 was additive to conf2, then you would get both.. Eric
        Hide
        Christian Siefkes added a comment -

        This I don't understand – how can I specify whether conf1 overrides conf2 or
        whether it is additive?

        All is see in the online javadocs for CompositeConfiguration is the single
        "addConfiguration(Configuration config)" method, and there you cannot specify
        whether it should override or add; neither can you specify this in the
        constructor. If there was such an option, it would be great, but currently I
        don't know it...

        From the documentation I understand that contained configurations are consulted
        in the order they were added, and the first match is returned (the first, not
        the last one, so newly added configs neither override nor add, but are only
        consulted as a fallback if previously configs don't contain this property). For
        getString etc. this work as expected because a result is returned as soon as a
        match is found.

        But getList is different – it doesn't stop when a match is found but iterates
        over all configurations and adds the list elements from all of them – you can
        easily check this by looking at the code.

        Show
        Christian Siefkes added a comment - This I don't understand – how can I specify whether conf1 overrides conf2 or whether it is additive? All is see in the online javadocs for CompositeConfiguration is the single "addConfiguration(Configuration config)" method, and there you cannot specify whether it should override or add; neither can you specify this in the constructor. If there was such an option, it would be great, but currently I don't know it... From the documentation I understand that contained configurations are consulted in the order they were added, and the first match is returned (the first, not the last one, so newly added configs neither override nor add, but are only consulted as a fallback if previously configs don't contain this property). For getString etc. this work as expected because a result is returned as soon as a match is found. But getList is different – it doesn't stop when a match is found but iterates over all configurations and adds the list elements from all of them – you can easily check this by looking at the code.
        Hide
        David Eric Pugh added a comment -

        Actually, I think you are right.. What you need for the override versus add
        behavior is to use the ConfigurationFactory. It deals with sorting all of
        that stuff out when the CompositeConfiguration is returned.

        Show
        David Eric Pugh added a comment - Actually, I think you are right.. What you need for the override versus add behavior is to use the ConfigurationFactory. It deals with sorting all of that stuff out when the CompositeConfiguration is returned.
        Hide
        Christian Siefkes added a comment -

        I have a patch that solves the problem. Add this method to CompositeConfiguration:

        /**

        • Adds a configuration. A call to
        • {@link CompositeConfiguration#addConfiguration(Configuration)}

          is

        • equalivant to calling this method with <code>append</code> set to
        • <code>true</code>.
          *
        • @param config the configuration to add
        • @param append if <code>true</code> list/array entries are added at the
        • end of existing lists; otherwise they are only used as a fallback, i.e.
        • if no list/array entry with the same key exists in previously added
        • configurations (for non-list/array entries the first match is used in
        • any case, so there is no difference)
          */
          public void addConfiguration(final Configuration config,
          final boolean append) {
          if (!append) {
          // getList does append, so if this isn't the requested behavior
          // we delete any entries from the new configuration that are
          // already present
          final Iterator keys = getKeys();
          String key;

        while (keys.hasNext())

        { key = (String) keys.next(); config.clearProperty(key); }

        }

        // delegate
        addConfiguration(config);
        }

        This way users can either call addConfiguration(conf) resp.
        addConfiguration(conf, true) to get the append behavior: list elements from
        different configurations will be joined.

        Or they can call addConfiguration(conf, false) to get the list elements from the
        first matching configuration only.

        It's probably not the optimal solution but at least it fixes this bug without
        introducing incompatibilites.

        BTW, it there any changes that a 1.0 version will come out soon? Or at least a
        new -devX version that will be published at http://www.ibiblio.org/maven/ ? That
        would be very useful for Maven users (such as me)...

        Show
        Christian Siefkes added a comment - I have a patch that solves the problem. Add this method to CompositeConfiguration: /** Adds a configuration. A call to {@link CompositeConfiguration#addConfiguration(Configuration)} is equalivant to calling this method with <code>append</code> set to <code>true</code>. * @param config the configuration to add @param append if <code>true</code> list/array entries are added at the end of existing lists; otherwise they are only used as a fallback, i.e. if no list/array entry with the same key exists in previously added configurations (for non-list/array entries the first match is used in any case, so there is no difference) */ public void addConfiguration(final Configuration config, final boolean append) { if (!append) { // getList does append, so if this isn't the requested behavior // we delete any entries from the new configuration that are // already present final Iterator keys = getKeys(); String key; while (keys.hasNext()) { key = (String) keys.next(); config.clearProperty(key); } } // delegate addConfiguration(config); } This way users can either call addConfiguration(conf) resp. addConfiguration(conf, true) to get the append behavior: list elements from different configurations will be joined. Or they can call addConfiguration(conf, false) to get the list elements from the first matching configuration only. It's probably not the optimal solution but at least it fixes this bug without introducing incompatibilites. BTW, it there any changes that a 1.0 version will come out soon? Or at least a new -devX version that will be published at http://www.ibiblio.org/maven/ ? That would be very useful for Maven users (such as me)...
        Hide
        Emmanuel Bourg added a comment -

        I don't think we want to remove elements from the added configuration here, if
        the configuration is then saved we will lose information.

        Show
        Emmanuel Bourg added a comment - I don't think we want to remove elements from the added configuration here, if the configuration is then saved we will lose information.
        Hide
        Christian Siefkes added a comment -

        OK, might be better to make a copy of the passed-in configuration before
        deleting elements to avoid that the original is destroyed.

        Alternatively addConfiguration could be left as is; and getList could be
        modified to stop at the first match (aside from the InMemoryConfig). This way we
        would get the documented behavior, but we'll lose the possibility to differ
        between override and append modes.

        Show
        Christian Siefkes added a comment - OK, might be better to make a copy of the passed-in configuration before deleting elements to avoid that the original is destroyed. Alternatively addConfiguration could be left as is; and getList could be modified to stop at the first match (aside from the InMemoryConfig). This way we would get the documented behavior, but we'll lose the possibility to differ between override and append modes.
        Hide
        Emmanuel Bourg added a comment -

        Some thoughts about this issue, maybe some refactoring is needed:

        • remove the getList methods from CompositeConfiguration and rely on the default
          implementation of AbstractConfiguration, that means the first list found is
          returned just like the other properties.
        • rename CompositeConfiguration into CascadeConfiguration. CascadeConfiguration
          would have 2 modes, a "first declared element win" and a "last declared element
          win" mode.
        • create a UnionConfiguration or CombinedConfiguration similar to
          CompositeConfiguration but with an additive semantic to the addConfiguration()
          method.

        Does this cover all use cases ?

        Show
        Emmanuel Bourg added a comment - Some thoughts about this issue, maybe some refactoring is needed: remove the getList methods from CompositeConfiguration and rely on the default implementation of AbstractConfiguration, that means the first list found is returned just like the other properties. rename CompositeConfiguration into CascadeConfiguration. CascadeConfiguration would have 2 modes, a "first declared element win" and a "last declared element win" mode. create a UnionConfiguration or CombinedConfiguration similar to CompositeConfiguration but with an additive semantic to the addConfiguration() method. Does this cover all use cases ?
        Hide
        Christian Siefkes added a comment -

        I suppose you still have to provide a specialized getList implementation because
        the default implementation would probably stop when finding a match in the
        InMemoryConfiguration. But if the user has used _add_Property to add list
        elements, they would expect the elements from the first matching
        non-InMemoryConfiguration to be returned as well.

        Otherwise that sould very good; and I think it would cover all relevant cases.

        Show
        Christian Siefkes added a comment - I suppose you still have to provide a specialized getList implementation because the default implementation would probably stop when finding a match in the InMemoryConfiguration. But if the user has used _add_Property to add list elements, they would expect the elements from the first matching non-InMemoryConfiguration to be returned as well. Otherwise that sould very good; and I think it would cover all relevant cases.
        Hide
        Emmanuel Bourg added a comment -

        getList() now returns the elements from the first matching configuration and the
        elements from the InMemoryConfiguration. It's a simple copy though, changes to
        the list will not affect the lists in the original configurations.

        Show
        Emmanuel Bourg added a comment - getList() now returns the elements from the first matching configuration and the elements from the InMemoryConfiguration. It's a simple copy though, changes to the list will not affect the lists in the original configurations.
        Hide
        Christian Siefkes added a comment -

        Now it works as documented.

        Show
        Christian Siefkes added a comment - Now it works as documented.

          People

          • Assignee:
            Unassigned
            Reporter:
            Christian Siefkes
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development