Hi,
I am not sure about the best names for the annotations. Sling is not really
consistent whether the resource has properties (which smells a bit like JCR) or
values. Actually the name ValueMap indicates, that the proper name for Sling is
rather value than property, but even the Javadoc mentions “property" (e.g. at
http://sling.apache.org/apidocs/sling6/org/apache/sling/api/resource/ValueMap.html).
What do you think about the annotations @Value (for source value-map),
@OsgiService, @ChildResource, @ScriptVariable, @RequestAttribute?
Regards,
Konrad
On 19 Mar 2014, at 09:19, Georg Henzler <sl...@cq-eclipse-plugin.net> wrote:
> Hi all,
>
> +1 for having meta annotations, I think they make model classes a lot more
> readable and give a small performance boost (outweighs the use of
> standardised @Inject IMHO).
>
> Regarding the naming I'd choose @ResourceProperty avoiding the name clash
> with @org.apache.felix.scr.annotations.Property and making it immediately
> visible to new developers that @ResourceProperty is different from @Property.
> I think @ChildResource and @OsgiService are good names for the other two.
>
> Regards
> Georg
>
> Am 18.03.2014 18:12, schrieb Justin Edelson:
>> Hi Konrad,
>> How about just @Property and @ChildResource ? As with the @Model
>> annotation, I don't see a reason to add "sling" as it is already in
>> the package name.
>>
>> @Property has overlap with SCR annotations, but I can't think of a
>> reason you would have them in the same class (and in any case, that's
>> what packages are for).
>>
>> While I'm not opposed to the optional attributes, I'm worried that the
>> extra attributes will make the code more convoluted. If you have the
>> time, I say try to do it, just be on the lookout for adding
>> complexity. And also make sure that there aren't any dependencies on
>> these new annotations in the core AdaptorFactory. Someone else will
>> need to implement their own annotations to imply @Inject
>> @Source("my-custom-source") with @MyCustomSource
>>
>> Regards,
>> Justin
>>
>> On Tue, Mar 18, 2014 at 11:55 AM, Konrad Windszus <konra...@gmx.de> wrote:
>>> 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 <jus...@justinedelson.com> 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 <konra...@gmx.de> 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 <jus...@justinedelson.com> 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 <konra...@gmx.de> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
