Adobe-Consulting-Services / acs-aem-commons

http://adobe-consulting-services.github.io/acs-aem-commons/
Apache License 2.0
453 stars 600 forks source link

Switch to OSGI annotations #1560

Closed joerghoh closed 5 years ago

joerghoh commented 5 years ago

Although the SCR annotations are still supported, we should switch over to OSGI annotations.

Carsten Ziegeler's blog:

Specific for AEM:

Registering Sling Servlets (new annotations): https://sling.apache.org/documentation/the-sling-engine/servlets.html#registering-a-servlet-using-java-annotations

References:

Learnings regarding Labels and Descriptions

Things I learned when dealing with Configurations and properties:

Regarding Java package versions:

In general:

heervisscher commented 5 years ago

thanks @joerghoh , happy to help to implement this

davidjgonzalez commented 5 years ago

Have you run into the scenario where OSGi property names have hypens in them? IIRC you can't do that w/ the new approach?

heervisscher commented 5 years ago

Good one! I thought this could be done via the name-attribute... @AttributeDefinition(name = "Multiple-Values", description = "Multi Configuration values")

But this is not the case.

joerghoh commented 5 years ago

No, the id is determined according to some rules, which described in chapter 105.15.2 (The \@AtributeDefinition in the metatype spec of R6). In R7 it's basically still the same.

The ID is deduced from the name of the method in the ObjectClassDefinition interface.

davidjgonzalez commented 5 years ago

@joerghoh correct - so can we move ErrorPageHandler to use the new annotations when there is this existing OSGi Property: error-page.system-path ? If so, what does that ObjectClassDefinition method name look like? (I dont know how to do that)

heervisscher commented 5 years ago

Just checked with carsten, conclusion: don't use hyphens for property names

joerghoh commented 5 years ago

R7 allows to use "$_$" which is replaced by a single hyphen (105.15.2). Not supported on R6 though.

joerghoh commented 5 years ago

Nevertheless, we need to find a way how we can deal with these properties on already existing components. I created issue #1567 to specifically deal with this case

heervisscher commented 5 years ago

first batch of change: https://github.com/Adobe-Consulting-Services/acs-aem-commons/pull/1580

heervisscher commented 5 years ago

When moving to R6 I see the following change in behaviour: In both cases no config was saved, but with the felix-annotations the defaults get written into the DS-properties image image

New code: https://github.com/Adobe-Consulting-Services/acs-aem-commons/pull/1580/files#diff-20b6e15068c1d1500bdfc90f156d2071 Old code: https://github.com/Adobe-Consulting-Services/acs-aem-commons/blob/master/bundle/src/main/java/com/adobe/acs/commons/email/impl/EmailServiceImpl.java

joerghoh commented 5 years ago

@heervischer: I guess that this is a feature of the SCR plugin, which mixes up DS and Metatype in many ways. Just checked the R6 spec, but I have not found a way to add metatype data also to the DS properties (just the other way around). From my point of view it's also not a show stopper.

WDYT?

heervisscher commented 5 years ago

No issue indeed, using this is making it fail-safe: this.profileName = StringUtils.defaultIfEmpty(config.profile(), DEFAULT_PROFILE);

heervisscher commented 5 years ago

also how can this replaced in r6? @References({ @Reference( name = "namedImageTransformers", referenceInterface = NamedImageTransformer.class, policy = ReferencePolicy.DYNAMIC, cardinality = ReferenceCardinality.OPTIONAL_MULTIPLE ),

davidjgonzalez commented 5 years ago

@heervisscher you should be able to do something like this:

https://github.com/Adobe-Marketing-Cloud/asset-share-commons/blob/master/core/src/main/java/com/adobe/aem/commons/assetshare/configuration/impl/AssetDetailsResolverImpl.java#L38

heervisscher commented 5 years ago

thanks @davidjgonzalez

badvision commented 5 years ago

How much of this do we have remaining?

heervisscher commented 5 years ago

around 40 classes I think

heervisscher commented 5 years ago

factories can be done with : @Designate(ocd=StaticReferenceRewriteTransformerFactory.Config.class,factory=true)

heervisscher commented 5 years ago

https://github.com/Adobe-Consulting-Services/acs-aem-commons/pull/1626

heervisscher commented 5 years ago

when specifying Integer and Boolean values in @Component, you need to do this with :Boolean=false or :Integer=100

kwin commented 5 years ago

@joerghoh Is there any reason why with this switch you not also switched to bnd-maven-plugin (instead of maven-bundle-plugin). The latter is much better supported right now and Sling is also slowly moving away from maven-bundle-plugin.

kwin commented 5 years ago

Also it seems to me that with the current master Felix SCR annotations are no longer in use, but still the maven-scr-plugin is defined in the root pom.xml and also the maven-bundle-plugin is still leveraging the org.apache.felix.scrplugin.bnd.SCRDescriptorBndPlugin. The latter causes issue for some people (#1675) and also makes the build slower than necessary. Can we get rid of those legacy plugins now? Also there is still the usage of aQute.bnd.annotation.ProviderType. This should be replaced by the official OSGi annotation as well. I will try to come up with a PR fixing those remaining leftovers.