Hi Justin, great. Thanks a lot for that fix. What do you think of the following annotations:
@SlingProperty (implies @Inject and @Source(“valuemap”)) @OsgiService (implies @Inject and @Source(“osgi-service”)) @SlingChildResource (implies @Inject and @Source(“child-resource”)) In my regard they could also provide the optional attributes - name (for SlingProperty and ChildResource only) - filter (for OsgiService only) - and via (for SlingProperty and ChildResource only) but that would require additional refactoring on either the injector or the adapterFactory side. The advantage is, that it is easier for the developer to look up what is supported for a specific injector (by leveraging code completion in his IDE), instead of remembering annotations (even if those are standard CDI annotations) and look up in the documentation whether that feature is supported for a specific injector. WDYT? Thanks, Konrad On 18 Mar 2014, at 16:08, Justin Edelson <[email protected]> wrote: > Hi Konrad, > I don't know about those names (@InjectSlingValue specifically - > what's a "Sling Value"?), but I think making @Inject work via > meta-annotations make sense. That support already exists for @Source, > would just need to be extended to work with @Inject as well. I just > did a little refactoring to make the meta-annotation support easier to > extend in the future. > > Whatever you submit, please just ensure there are tests included. > > Regards, > Justin > > On Tue, Mar 18, 2014 at 10:54 AM, Konrad Windszus <[email protected]> wrote: >> Hi Justin, >> thanks for your answer. What about if I come up with a patch for additional >> annotations like >> @InjectSlingValue and @InjectOsgiService >> which are just another way of annotating fields/methods and combine >> logically both the Inject and the Source. In case of InjectOsgiService one >> could even include the optional attribute filter which would add the @Filter >> then. For that I would like to use the meta annotation concept (i.e. >> annotation the @InjectSlingValue with @Inject and @Source). Since Java does >> not come with support for meta annotations out of the box, one could just >> copy the according method from the Spring Core: >> https://github.com/spring-projects/spring-framework/blob/master/spring-core/src/main/java/org/springframework/core/annotation/AnnotationUtils.java#L94 >> and use that in the ModelAdapterFactory. >> >> Would you accept such a patch which would basically comprise of >> a) an additional annotation per injector >> b) additionally evaluating the annotations on annotations within the >> ModelAdapterFactory >> >> Thanks, >> Konrad >> >> On 17 Mar 2014, at 19:01, Justin Edelson <[email protected]> wrote: >> >>> Hi Konrad, >>> (a) is correct and is intentional. Is there an actual situation where >>> this would happen accidentially? In my experience, a new BVP is not >>> added every day and has a broad impact. If you add a BVP without >>> regressing your application, that's a problem into itself. >>> >>> (b) and (c) would only be correct if the type of the OSGi service is >>> something to which your child resource or request attribute could be >>> adapted, which seems highly unlikely to me. >>> >>> The JCR operations (property lookup and child node lookups) should be >>> well-optimized by implementations. If there was an injector which >>> executed a query, that would be an example of a place where the >>> injector *should* require @Source or some other annotation to >>> explicitly include the injector. >>> >>> If you want to introduce a "strict" mode which requires @Source, feel >>> free to submit a patch. But I don't think this makes sense as the >>> default. >>> >>> Regards, >>> Justin >>> >>> On Mon, Mar 17, 2014 at 1:24 PM, Konrad Windszus <[email protected]> wrote: >>>> Hi, >>>> I am a little bit worried that model classes which leverage the Sling >>>> Models annotations might be slow and break fast. >>>> >>>> If you use the annotation @Inject without @Source all injectors are asked >>>> until one returns a value. >>>> Since almost all injectors depend on the fieldname and cover the same >>>> namespace, models can break very easily. >>>> Let me give three examples for broken models: >>>> >>>> a) If I use in my model >>>> @Inject >>>> String myattribute; >>>> which used to be resolved by the ValueMap injector and I just introduced a >>>> new Scripting variable with the name "myattribute" >>>> (https://cwiki.apache.org/confluence/display/SLING/Adding+New+Scripting+Variables), >>>> my model would no longer work. >>>> b) The same would apply, if I have a Sling resource with a value named >>>> "service" which used to be (by coincidence) also the field name of my >>>> injected OSGi service in the model. >>>> c) The same for a new request attribute, whose name conflicts with the >>>> field name of my injected OSGi service in the model >>>> >>>> Regarding the performance, I fear that the lookup in the value map from >>>> the second injector might take some time (since JCR access is necessary in >>>> most cases to do that). For example if I have a model class only relying >>>> on OSGi services a lot of time would be wasted by asking other injectors. >>>> >>>> To summarize: Isn't it always advisable to use the @Source annotation to >>>> prevent those kind of name clashes and performance issues? Shouldn't that >>>> be explicitly stated in the documentation? >>>> What is your opinion on that? >>>> >>>> What about making the @Source mandatory for all injectors but the ValueMap >>>> injector? >>>> Thanks, >>>> Konrad >>>> >>>> >>>> >>
