deltaspike-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gerhard Petracek <gerhard.petra...@gmail.com>
Subject Re: Re : Re : Re : ConfigResolver : adding @ConfigProperty injection ?
Date Tue, 13 Mar 2012 22:52:18 GMT
hi adrian,

yes - i moved too many classes during the last refactoring -> thx for the
heads-up

@ converter-type logic:
i haven't implemented too much of it, because it needs further discussions.

@ custom converter:
that's right

i'll add missing todos for further discussions and afterwards i'll push it
to our repository.
furthermore, i already have javadoc and several tests for it - i'll push
them in a 2nd step as soon as all of them are finished.

regards,
gerhard



2012/3/13 Adrian Gonzalez <adr_gonzalez@yahoo.fr>

> Hi Gerhard,
>
> Really nice !
>
> Some remarks :
>  * you return null when config key isn't defined, I would prefer to raise
> an exception.
>    At least by default - if you really want to allow optional
> configuration keys, I would add an optional attribute in ConfigProperty.
>    Otherwise we'll have in most case a NPE further down in our apps.
>  * missing eager attribute in ConfigProperty.
>  * for Converter/ConverterFactory : with the current impl, a given
> converter can map from only one type to another.
>
>      * can this work for javabean to javabean converter ? (converter will
> be registered to convert from Object to Object - so calling
> ConverterService.create(A.class, B.class) will fail)
>      * can I code a single converter doing conversion String to 0..n types
> ?
>  * ConverterFactory.create should raise an exception if no converter is
> found.
>  * ConverterExtension should be renamed ConfigExtension ?
>  * ConverterKey, DefaultConverterFactory, StringToIntegerConverter should
> be in a converter package, not in config.
>
> Just to be sure :
>  * about converter precedence : if I code a custom converter in my app, it
> will take precedence of the default ones (those provided by Deltaspike) :
> correct ?
>
> Further use cases are based on more out of the box conversion
> possibilities :
>  * String to any primitive / wrapper type.
>  * String to Locale ?
>  * String to Collection<X> ?
>
> Regards,
>
> P.S. understood about 3rd party projects ;)
>
>
> ----- Mail original -----
> De : Gerhard Petracek <gpetracek@apache.org>
> À : deltaspike-dev@incubator.apache.org
> Cc :
> Envoyé le : Mardi 13 mars 2012 19h27
> Objet : Re: Re : Re : ConfigResolver : adding @ConfigProperty injection ?
>
> hi adrian,
>
> at [1] you can find my >first< draft including some examples. the current
> draft supports everything we have discussed including 1-2 additional
> features.
> so far i haven't looked at your suggestion to avoid that we get in
> implementations of spring.
> -> i would suggest that you check e.g. the examples -> if needed we can
> discuss further use-cases (which might be supported by your draft already).
>
> for the future:
> please never use source-code of 3rd party projects (= projects outside of:
> apache and officially donated projects)
> -> we can avoid such a redundant effort with other features.
>
> regards,
> gerhard
>
> [1] http://s.apache.org/Adk
>
>
>
> 2012/3/13 Gerhard Petracek <gerhard.petracek@gmail.com>
>
> > hi adrian,
> >
> > thx for starting with it! i'll have a look at it tomorrow.
> > fyi: we don't copy parts of other projects without official
> > permission/donation.
> >
> > regards,
> > gerhard
> >
> >
> >
> > 2012/3/13 Adrian Gonzalez <adr_gonzalez@yahoo.fr>
> >
> >> Hello,
> >>
> >> I've implemented ConfigProperty feature [1]
> >> Not sure if it's fine, I'm just a CDI newbie.
> >> I just hope this can help.
> >> My notes are here : [2]
> >>
> >>
> >> Best regards,
> >>
> >> [1]
> https://github.com/gonzalad/incubator-deltaspike/tree/DELTASPIKE-114
> >> [2] notes :
> >>
> >> 1. ConfigProperty
> >> Implemented both approaches : @ConfigProperty and custom annotation.
> >> Some questions :
> >>  * for now I'm throwing an exception if property is missing.
> >>     I think this is the right way to go (I don't want to have a
> >> NullPointerException somewhere after).
> >>     But perhaps we can add a required attribute in ConfigProperty.
> >>  * implemented hack for converter attribute which should be optionnal
> and
> >> default value should be null.
> >>     Needed to create special class for null value (annotation attribute
> >> default value cannot be null).
> >>  * not sure about equals impl for ConfigPropertyBean
> >>  * is there a simpler approach for ConfigProperty implementation ?
> >>    For now I have an extension observing ProcessInjectionTarget
> >> and AfterBeanDiscovery.
> >>     * in ProcessInjectionTarget it scans every injectionPoint looking at
> >> @ConfigAttribute or at an annotation annotated with @ConfigAttribute
> >>        if found ti stores this injectionPoint in a list.
> >>     * in AfterBeanDiscovery, it adds a CDI bean for every item
> >> (configAttribute) in the previous list.
> >>    Could a parameterized producer be used instead ? Don't think so,
> but...
> >>  * FYI : I think unit tests for extension can collide (i.e Excludes and
> >> ConfigPropertyExtension).
> >>
> >> 2. Converters
> >> An explicit converter class can be set with converter attribute.
> >> Otherwise, we use default conversion service (should handle any
> primitive
> >> or wrapper type conversion).
> >> The explicit converter must implement Converter interface (in api).
> >> ConversionService is packaged in impl for now.
> >> I'm not too happy about conversionService.
> >> It's a brutal copy/paste from Spring (I removed without reading all that
> >> - I was really in a hurry)
> >> Not happy about it.
> >>
> >> 3 options then :
> >>  * we try a really minimal implementation (basically
> >> hashmap<KeyPair<sourceClass,targetClass>,Converter>).
> >>  * we try to implement it well and completely - IMO this will be long.
> >>  * we copy / paste all Spring conversion code - soso.
> >>
> >> Some questions :
> >>  * some code has been copy/paste from Spring
> >> (i.e. StringToBooleanConverter) : donno how to do.
> >> I've left initial authors (of course !), but changed header file.
> >> Donno if you want to change code impl, or if I need to contact Spring...
> >>
> >>
> >>
> >>
> >> ________________________________
> >> De : Gerhard Petracek <gerhard.petracek@gmail.com>
> >> À : deltaspike-dev@incubator.apache.org
> >> Cc : Pete Muir <pmuir@redhat.com>
> >> Envoyé le : Jeudi 8 mars 2012 14h26
> >> Objet : Re: Re : ConfigResolver : adding @ConfigProperty injection ?
> >>
> >> hi adrian,
> >>
> >> @ #2:
> >> since it's the easiest approach and i would have suggested the same
> >> (independent of other libs): +1 for the interface
> >> right now we just have an use-case for specifying the converter manually
> >> ->
> >> we can add e.g. a ConverterFactory later on (as soon as we need it for
> an
> >> use-case).
> >>
> >> @ #4:
> >> if i remember correctly, someone in an open-source community mentioned
> >> such
> >> a plan (for the "near" future).
> >>
> >> regards,
> >> gerhard
> >>
> >>
> >>
> >> 2012/3/7 Adrian Gonzalez <adr_gonzalez@yahoo.fr>
> >>
> >> > Noticed the JIRA issues for this issue, thanks Gerhard  !
> >> > Sorry for the delay (quite struggling in my day to day work for now
> ;( )
> >> >
> >> >
> >> > About conversion API (for converter = StringToIntegerConverter.class),
> >> > just googled a bit :
> >> >  1. there's the javabean specification (soso...)
> >> >  2. Spring has a really nice conversion API :
> >> >
> >>
> http://static.springsource.org/spring/docs/3.1.x/spring-framework-reference/html/validation.html#core-convert
> >> >     built on top of :
> >> >     public interface Converter<S, T> {
> >> >       T convert(S source);
> >> >     }
> >> >     the upper layers are ConversionService, GenericConverter - not
> sure
> >> if
> >> > it's too early to introduce similar functionnality
> >> >  3. of course JSF converters but really tied to JSF (soso...)
> >> >  4. didn't found any info about the official Conversion JSR
> >> >
> >> >
> >> >
> >> > ________________________________
> >> > De : Jason Porter <lightguard.jp@gmail.com>
> >> > À : deltaspike-dev@incubator.apache.org
> >> > Cc : Adrian Gonzalez <adr_gonzalez@yahoo.fr>
> >> > Envoyé le : Mardi 6 mars 2012 22h25
> >> > Objet : Re: ConfigResolver : adding @ConfigProperty injection ?
> >> >
> >> > +1 to the idea. I also think some more discussion about this is in
> >> order.
> >> >
> >> > On Tue, Mar 6, 2012 at 04:09, Pete Muir <pmuir@redhat.com> wrote:
> >> >
> >> > > +1
> >> > >
> >> > > On 5 Mar 2012, at 16:17, Adrian Gonzalez wrote:
> >> > >
> >> > > > What about having both options ?
> >> > > >
> >> > > > 1. be able to use directly @ConfigProperty
> >> > > > @Produces
> >> > > > public LoginContext
> >> > > > produceLoginContext(@ConfigProperty("loginConfigFile") String
> >> > > loginConfigFileName, @ConfigProperty("loginModuleName") String
> >> > > loginModuleName)
> >> > > >   }
> >> > > > }
> >> > > > Handy when all config values are only used in a centralized class
> >> (e.g.
> >> > > MyAppConfig).
> >> > > >
> >> > > > 2. type safe config annotations
> >> > > > @ConfigProperty(
> >> > > >   name = "pool_size",
> >> > > >   eager = true, //true is also the default value -> the value
gets
> >> > > converted during the bootstrapping process
> >> > > >   converter = StringToIntegerConverter.class
> >> > > > )
> >> > > > public @interface PoolSize {
> >> > > > }
> >> > > >
> >> > > > ----- Mail original -----
> >> > > > De : Gerhard Petracek <gerhard.petracek@gmail.com>
> >> > > > À : deltaspike-dev@incubator.apache.org
> >> > > > Cc :
> >> > > > Envoyé le : Lundi 5 mars 2012 17h08
> >> > > > Objet : Re: Re : ConfigResolver : adding @ConfigProperty
> injection ?
> >> > > >
> >> > > > hi adrian,
> >> > > >
> >> > > > @#1:
> >> > > > it isn't the default value of the property.
> >> > > > eager should be true by default -> the configured value gets
> >> converted
> >> > > > during bootstrapping (if the value has an invalid format the
> >> > > bootstrapping
> >> > > > process fails).
> >> > > > if eager is false, the configured value will be converted directly
> >> > before
> >> > > > the injection (e.g. for values stored in dynamic config-sources).
> >> > > >
> >> > > > @#2:
> >> > > > afaik there is an upcoming jsr about it.
> >> > > >
> >> > > > regards,
> >> > > > gerhard
> >> > > >
> >> > > >
> >> > > >
> >> > > > 2012/3/5 Adrian Gonzalez <adr_gonzalez@yahoo.fr>
> >> > > >
> >> > > >> Hi Gerhard,
> >> > > >>
> >> > > >> 1. didn't understood the meaning of eager attribute
> >> > > >> If eager = default value, it should have been some integer
value
> in
> >> > your
> >> > > >> sample
> >> > > >>
> >> > > >>
> >> > > >> 2. StringToIntegerConverter converter
> >> > > >> Is there an existing conversion API (standard, in deltaspike,
> ...)
> >> ?
> >> > > >> (otherwise we're gonna need one here - and end up with another
> >> > > conversion
> >> > > >> API ;) )
> >> > > >>
> >> > > >> Thanks !
> >> > > >>
> >> > > >> ----- Mail original -----
> >> > > >> De : Gerhard Petracek <gerhard.petracek@gmail.com>
> >> > > >> À : deltaspike-dev@incubator.apache.org
> >> > > >> Cc :
> >> > > >> Envoyé le : Lundi 5 mars 2012 12h57
> >> > > >> Objet : Re: ConfigResolver : adding @ConfigProperty injection
?
> >> > > >>
> >> > > >> hi adrian,
> >> > > >>
> >> > > >> if we agree also on adding a bit more to allow e.g.:
> >> > > >>
> >> > > >> //...
> >> > > >> @ConfigProperty(
> >> > > >>    name = "pool_size",
> >> > > >>    eager = true, //true is also the default value -> the
value
> gets
> >> > > >> converted during the bootstrapping process
> >> > > >>    converter = StringToIntegerConverter.class
> >> > > >> )
> >> > > >> public @interface PoolSize
> >> > > >> {
> >> > > >> }
> >> > > >>
> >> > > >> @Inject
> >> > > >> @PoolSize
> >> > > >> private int configuredPoolSize;
> >> > > >>
> >> > > >> then i would vote +1
> >> > > >> (for sure the details need further discussions.)
> >> > > >>
> >> > > >> regards,
> >> > > >> gerhard
> >> > > >>
> >> > > >>
> >> > > >>
> >> > > >> 2012/3/5 Adrian Gonzalez <adr_gonzalez@yahoo.fr>
> >> > > >>
> >> > > >>> Hello,
> >> > > >>>
> >> > > >>> Deltaspike config module is based on ConfigResolver usage
:
> >> > > >>>    ConfigResolver.getPropertyValue("test")
> >> > > >>>
> >> > > >>>
> >> > > >>> Wouldn't it be interesting to add on top of it some injection
> >> > > >>> capacity ? (i.e. providing @ConfigProperty annotation)
> >> > > >>>
> >> > > >>> Sample usage [1] :
> >> > > >>> @Produces
> >> > > >>> public LoginContext
> >> > > >> produceLoginContext(@ConfigProperty("loginConfigFile")
> >> > > >>> String loginConfigFileName,
> >> > > >>>
> >> > > >>   @ConfigProperty("loginModuleName")
> >> > > >>> String loginModuleName)
> >> > > >>>     blabla
> >> > > >>> }
> >> > > >>>
> >> > > >>> This approach is based on Antonio's petstore application
-
> config
> >> > code
> >> > > is
> >> > > >>> available in [2]
> >> > > >>>
> >> > > >>> [1]
> >> > > >>>
> >> > > >>
> >> > >
> >> >
> >>
> https://github.com/agoncal/agoncal-application-petstore-ee6/blob/master/src/main/java/org/agoncal/application/petstore/security/LoginContextProducer.java
> >> > > >>>
> >> > > >>> [2]
> >> > > >>>
> >> > > >>
> >> > >
> >> >
> >>
> https://github.com/agoncal/agoncal-application-petstore-ee6/blob/master/src/main/java/org/agoncal/application/petstore/util/ConfigProperty.java
> >> > > >>>
> >> > > >>>
> >> > > >>
> >> > >
> >> >
> >>
> https://github.com/agoncal/agoncal-application-petstore-ee6/blob/master/src/main/java/org/agoncal/application/petstore/util/ConfigPropertyProducer.java
> >> > > >>>
> >> > > >>>
> >> > > >>
> >> > > >>
> >> > > >
> >> > >
> >> > >
> >> >
> >> >
> >> > --
> >> > Jason Porter
> >> > http://lightguard-jp.blogspot.com
> >> > http://twitter.com/lightguardjp
> >> >
> >> > Software Engineer
> >> > Open Source Advocate
> >> > Author of Seam Catch - Next Generation Java Exception Handling
> >> >
> >> > PGP key id: 926CCFF5
> >> > PGP key available at: keyserver.net, pgp.mit.edu
> >> >
> >>
> >
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message