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 <[email protected]>
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 <[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
