Would that be in a separate branch to start with?

Werner


On Tue, Oct 18, 2016 at 4:42 PM, Anatole Tresch <atsti...@gmail.com> wrote:

> Hi all
>
> based on the work to see how easy some of the ideas of Dimitry (the
> designated Oracle spec lead for config) could be implemented I would like
> to propose some adaptions and changes:
>
>    - I would basically resolve the current duplication of the builder
>    pattern (in the builder module as well as in the core API). The core API
>    itself is already based on an interface, which if done right IMO gives
> all
>    the flexibility needed and also would make some of the more implicit
>    mechanism more explicit.
>    - The main API changes required are on the *ConfigurationContextBuilder*
>    SPI interface:
>       - this interface as of now already contains methods for adding and
>       removing of property sources, filters and converters. Most of the
> methods
>       are implemented in two variants as of now: once taking a Collection,
> once
>       using an ellipse operator taking single items or arrays.
> Unfortunately some
>       of the methods are not fully present in both variants, so I
> propoase to add
>       the missing counterparts. This basically is not a functional change,
> but
>       makes the API more concise.
>       - currently the implementation of the builder implicitly applies
>       sorting of property sources and filters using its internal sorting
>       algorithm. This works well, but in case, where I dont want to use the
>       ordinal pattern, e.g. because I have configured a different
> hierarchy in a
>       XML meta-config file, this implicit sorting makes it hard to build
> up a
>       property source chain in a transparent and clear way (using the
> builder). I
>       would expect the builder to NOT perform any sorting implicitly.
> Instead
>       sorting is controlled by calling the appropriate sort methods hereby
>       passing a *Comparator*. So I propose to add to methods
> *sortPropertySources(Comparator)
>       *and *sortPropertyFilters(Comparator) *to the builder interface.
>       - Given that the *ConfigurationContextBuilder* actually manages an
>       ordered list of property sources. In case I want to increase/decrese
> the
>       priority of a property source or make it the most/least significant
> one.
>       Therefore I propose to add the methods *(incresePriority(
> PropertySource),
>       decresePriority(PropertySource), lowestPriority(PropertySource),
>       highestPriority(PropertySource)*. One use case is creating a builder
>       from an existing *ConfigurationContext* and change the priority of
>       some of its property sources. Also these methods do strictly
> operate on the
>       ordering only, they will not change implicitly any ordinals.
>       - For implementing module it has shown that access tot he current
>       property sources, filters and converters of a builder is useful.
> Creating a
>       context and accessing later does not work, since the builder supports
>       building an instance only once. So I propose to add the methods
> *getPropertySources(),getPropertyFilters(),
>       getPropertyConverter()*.
>    - Finally given a *ConfigurationContext* built with the builder, I want
>    to create a *Configuration* instance. The corresponding method
> *createConfiguration(ConfigurationContext)
>    *is already defined on the *ConfigurationProviderSpi* interface. So I
>    would like to add it as well on the *ConfigurationProvider*. Obviously
>    building a configuration MUST resepct/ensure the precendece of property
>    sources as defined by its context (which by now, was not the case).
>
> With these changes we have a much better separation of concerns:
>
>    - we have an ordered list of property sources. The ordering hereby can
>    be ordinal based (used by default).
>    - But using the ConfigurationContextBuilder alternate strategies can be
>    easily defined by implementing a different Comparator being used or
>    applying the ordering required using the methods offerend on the
> builder.
>    - Using ordinals is the default way provided, but using modules we can
>    easily provide alternate strategies as well.
>    - This will quite probably also help with contextual aspects, once it is
>    more clear, what contextuality means regarding configuration and also
> make
>    implementation of a company specific strategy much more easier.
>
> There is one disadvantage I dont want to omit: the changes in the
> implementation are not fully behavioural compatible wuth code that already
> is using the builder pattern. Since the method for creating a new
> Configuration instance has not been exposed I expect the changes only
> affect code within Tamaya itself (basically mostly some testing and code in
> the core, but not much).
>
> ​Feedback is welcome. The changes described are actually implemented,
> testing and are all working and have proven to work well. So if nobody
> complains I will checkin the changes on Thursday-WE the latest.
>
> J Anatole​
>
>
> ​Following the source of the ConfigurationContextBuilder with marks for
> additions:
>
> /**
>  * A builder for creating new or adapting instances of {@link
> ConfigurationContext}.
>  * Builders can be obtained in exactly two ways:
>  * <ol>
>  *     <li>By accessing a preinitialized builder from an existing
> {@link ConfigurationContext},
>  *     by calling {@link
> org.apache.tamaya.spi.ConfigurationContext#toBuilder()}.</li>
>  *     <li>By accessing an empty builder instance from
>  *     {@link org.apache.tamaya.ConfigurationProvider#
> getConfigurationContextBuilder()}.</li>
>  * </ol>
>  * After all changes are applied to a builder a new {@link
> ConfigurationContext} instance can
>  * be created and can be applied by calling
>  * {@link org.apache.tamaya.ConfigurationProvider#
> setConfigurationContext(org.apache.tamaya.spi.ConfigurationContext)}.
>  *
>  */
> public interface ConfigurationContextBuilder {
>
>     /**
>      * Init this builder instance with the given {@link
> ConfigurationContext} instance. This
>      * method will use any existing property sources, filters,
> converters and the combination
>      * policy of the given {@link ConfigurationContext} and initialize
> the current builder
>      * with them.
>      *
>      * @param context the {@link ConfigurationContext} instance to be
> used, not null.
>      * @return this builder, for chaining, never null.
>      */
>     ConfigurationContextBuilder setContext(ConfigurationContext context);
>
>     /**
>      * This method can be used for adding {@link PropertySource}s.
>      * Hereby the property source is added to the tail of property sources
> with
>      * lowest priority  regardless of its current ordinal value. To
> sort the property
>      * sources based on their ordinals call {@kink #sortPropertySources}.
>      *
>      * @param propertySources the PropertySources to add
>      * @return this builder, for chaining, never null.
>      * @throws IllegalArgumentException If a property source with a
> given name already
>      * exists.
>      */
>     ConfigurationContextBuilder addPropertySources(PropertySource...
> propertySources);
>
>     /**
>      * This method can be used for programmatically adding {@link
> PropertySource}s.
>      * Hereby the property source is added to the tail of property sources
> with
>      * lowest priority regardless of its current ordinal value. To
> sort the property
>      * sources based on their ordinals call {@kink #sortPropertySources}.
>      *
>      * @param propertySources the PropertySources to add
>      * @return this builder, for chaining, never null.
>      * @throws IllegalArgumentException If a property source with a
> given name already
>      * exists.
>      */
>     ConfigurationContextBuilder
> addPropertySources(Collection<PropertySource> propertySources);
>
>     /**
>      * Removes the given property sources, if existing. The existing
> order of property
>      * sources is preserved.
>      *
>      * @param propertySources the property sources to remove, not null.
>      * @return the builder for chaining.
>      */
>     ConfigurationContextBuilder
> removePropertySources(PropertySource... propertySources);
>
>     /**
>      * Removes the given property sources, if existing. The existing
> order of property
>      * sources is preserved.
>      *
>      * @param propertySources the property sources to remove, not null.
>      * @return the builder for chaining.
>      */
>     ConfigurationContextBuilder
> removePropertySources(Collection<PropertySource> propertySources);
>
>     /**
>      * Access the current chain of property sources. Items at the end
> of the list have
>      * precedence/more significance.
>      *
>      * @return the property source chain, never null.
>      */
>     List<PropertySource> getPropertySources();
>
>     /**
>      * Access the current chain of property filters. Items at the end
> of the list have
>      * precedence/more significance.
>      *
>      * @return the property source chain, never null.
>      */
>     List<PropertyFilter> getPropertyFilters();
>
>     /**
>      * Access the current registered property converters.
>      *
>      * @return the current registered property converters.
>      */
>     Map<TypeLiteral<?>, Collection<PropertyConverter<?>>>
> getPropertyConverter();
>
>     /**
>      * Increases the priority of the given property source, by moving
> it towards the end
>      * of the chain of property sources. If the property source given
> is already at the end
>      * this method has no effect. This operation does not change any
> ordinal values.
>      *
>      * @param propertySource the property source to be incresed
> regarding its significance.
>      * @return the builder for chaining.
>      * @throws IllegalArgumentException If no such property source
> exists in the current
>      * chain.
>      */
>     ConfigurationContextBuilder increasePriority(PropertySource
> propertySource);
>
>     /**
>      * Decreases the priority of the given property source, by moving
> it towards the start
>      * of the chain of property sources. If the property source given
> is already the first
>      * this method has no effect. This operation does not change any
> ordinal values.
>      *
>      * @param propertySource the property source to be decresed
> regarding its significance.
>      * @return the builder for chaining.
>      * @throws IllegalArgumentException If no such property source
> exists in the current
>      * chain.
>      */
>     ConfigurationContextBuilder decreasePriority(PropertySource
> propertySource);
>
>     /**
>      * Increases the priority of the given property source to be
> maximal, by moving it to
>      * the tail of the of property source chain. If the property source
> given is
>      * already the last item this method has no effect. This operation
> does not change
>      * any ordinal values.
>      *
>      * @param propertySource the property source to be maximized
> regarding its significance.
>      * @return the builder for chaining.
>      * @throws IllegalArgumentException If no such property source
> exists in the current
>      * chain.
>      */
>     ConfigurationContextBuilder highestPriority(PropertySource
> propertySource);
>
>     /**
>      * Decreases the priority of the given property source to be
> minimal, by moving it to
>      * the start of the chain of property source chain. If the
> property source given is
>      * already the first item this method has no effect. This
> operation does not change
>      * any ordinal values.
>      *
>      * @param propertySource the property source to be minimized
> regarding its significance.
>      * @return the builder for chaining.
>      * @throws IllegalArgumentException If no such property source
> exists in the current
>      * chain.
>      */
>     ConfigurationContextBuilder lowestPriority(PropertySource
> propertySource);
>
>     /**
>      * Adds the given PropertyFilter instances, hereby the instances are
> added
>      * to the end of the list with highest priority. The ordering of
> existing
>      * property filters remains unchanged. To sort the property
>      * filters call {@kink #sortPropertyFilter}.
>      *
>      * @param filters the filters to add
>      * @return this builder, for chaining, never null.
>      */
>     ConfigurationContextBuilder addPropertyFilters(PropertyFilter...
> filters);
>
>     /**
>      * Adds the given PropertyFilter instances, hereby the instances are
> added
>      * to the end of the list with highest priority. The ordering of
> existing
>      * property filters remains unchanged. To sort the property
>      * filters call {@kink #sortPropertyFilter}.
>      *
>      * @param filters the filters to add
>      * @return this builder, for chaining, never null.
>      */
>     ConfigurationContextBuilder
> addPropertyFilters(Collection<PropertyFilter> filters);
>
>     /**
>      * Removes the given PropertyFilter instances, if existing. The
> order of the remaining
>      * filters is preserved.
>      *
>      * @param filters the filter to remove
>      * @return this builder, for chaining, never null.
>      */
>     ConfigurationContextBuilder
> removePropertyFilters(PropertyFilter... filters);
>
>     /**
>      * Removes the given PropertyFilter instances, if existing. The
> order of the remaining
>      * filters is preserved.
>      *
>      * @param filters the filter to remove
>      * @return this builder, for chaining, never null.
>      */
>     ConfigurationContextBuilder
> removePropertyFilters(Collection<PropertyFilter> filters);
>
>     /**
>      * This method can be used for adding {@link PropertyConverter}s.
>      * Converters are added at the end after any existing converters.
>      * For converters already registered for the current target type the
>      * method has no effect.
>      *
>      * @param typeToConvert     the type for which the converter is for
>      * @param propertyConverters the PropertyConverters to add for this
> type
>      * @return this builder, for chaining, never null.
>      */
>     <T> ConfigurationContextBuilder
> addPropertyConverters(TypeLiteral<T> typeToConvert,
>
> PropertyConverter<T>... propertyConverters);
>
>     /**
>      * This method can be used for adding {@link PropertyConverter}s.
>      * Converters are added at the end after any existing converters.
>      * For converters already registered for the current target type the
>      * method has no effect.
>      *
>      * @param typeToConvert     the type for which the converter is for
>      * @param propertyConverters the PropertyConverters to add for this
> type
>      * @return this builder, for chaining, never null.
>      */
>     <T> ConfigurationContextBuilder
> addPropertyConverters(TypeLiteral<T> typeToConvert,
>
> Collection<PropertyConverter<T>> propertyConverters);
>
>     /**
>      * Removes the given PropertyConverter instances for the given type,
>      * if existing.
>      *
>      * @param typeToConvert the type which the converter is for
>      * @param propertyConverters    the converter to remove
>      * @return this builder, for chaining, never null.
>      */
>     <T> ConfigurationContextBuilder
> removePropertyConverters(TypeLiteral<T> typeToConvert,
>
> PropertyConverter<T>... propertyConverters);
>
>     /**
>      * Removes the given PropertyConverter instances for the given type,
>      * if existing.
>      *
>      * @param typeToConvert the type which the converter is for
>      * @param propertyConverters    the converter to remove
>      * @return this builder, for chaining, never null.
>      */
>     <T> ConfigurationContextBuilder
> removePropertyConverters(TypeLiteral<T> typeToConvert,
>
> Collection<PropertyConverter<T>> propertyConverters);
>
>     /**
>      * Removes all converters for the given type, which actually
> renders a given type
>      * unsupported for type conversion.
>      *
>      * @param typeToConvert the type which the converter is for
>      * @return this builder, for chaining, never null.
>      */
>     ConfigurationContextBuilder
> removePropertyConverters(TypeLiteral<?> typeToConvert);
>
>     /**
>      * Sorts the current registered property sources using the given
> comparator.
>      *
>      * NOTE: property sources at the beginning have minimal significance.
>      *
>      * @param comparator the comparator to be used, not null.
>      * @return this instance for chaining.
>      */
>     ConfigurationContextBuilder
> sortPropertySources(Comparator<PropertySource> comparator);
>
>     /**
>      * Sorts the current registered property filters using the given
> comparator.
>      *
>      * NOTE: property filters at the beginning have minimal significance.
>      *
>      * @param comparator the comparator to be used, not null.
>      * @return this instance for chaining.
>      */
>     ConfigurationContextBuilder
> sortPropertyFilter(Comparator<PropertyFilter> comparator);
>
>     /**
>      * Sets the {@link PropertyValueCombinationPolicy} used to
> evaluate the final
>      * property values.
>      *
>      * @param policy the {@link PropertyValueCombinationPolicy} used, not
> null
>      * @return this builder, for chaining, never null.
>      */
>     ConfigurationContextBuilder
> setPropertyValueCombinationPolicy(PropertyValueCombinationPolicy
> policy);
>
>     /**
>      * Builds a new {@link ConfigurationContext} based on the data in
> this builder. The ordering of property
>      * sources and property filters is not changed, regardless of
> their ordinals. For ensure a certain
>      * ordering/significance call {@link
> #sortPropertyFilter(Comparator)} and/or {@link
> #sortPropertySources(Comparator)}
>      * before building the context.
>      *
>      * @return the final context to be used to create a configuration.
>      * @see org.apache.tamaya.ConfigurationProvider#createConfiguration(
> ConfigurationContext)
>      */
>     ConfigurationContext build();
>
> }​
>

Reply via email to