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 <[email protected]> À : [email protected] 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 <[email protected]> > 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 <[email protected]> > >> 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 <[email protected]> >> À : [email protected] >> Cc : Pete Muir <[email protected]> >> 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 <[email protected]> >> >> > 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 <[email protected]> >> > À : [email protected] >> > Cc : Adrian Gonzalez <[email protected]> >> > 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 <[email protected]> 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 <[email protected]> >> > > > À : [email protected] >> > > > 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 <[email protected]> >> > > > >> > > >> 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 <[email protected]> >> > > >> À : [email protected] >> > > >> 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 <[email protected]> >> > > >> >> > > >>> 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 >> > >> > >
