search

Adobe-Consulting-Services / acs-aem-commons

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

Revert back to SCR annotations #1741

Closed justinedelson closed 5 years ago

justinedelson commented 5 years ago

Although I appreciate the hard work put in by various people to update the ACS AEM Commons codebase to the standard OSGi annotations, I feel that doing so has jeopardized the stability of the project. At this point, the master branch is not in a releasable state and I don't think the work needed to bring it back to release quality is quantified.

The value in moving to the standard OSGi annotations is honestly unclear to me at this point. It seems to be little to none and there are known backward compatibility issues. But what honestly concerns me more is the unknown issues. I've tried to create a process to compare the old and new generated XML files (see https://github.com/Adobe-Consulting-Services/acs-aem-commons/blob/issue/osgi-annotation-problems/bundle/src/test/java/com/adobe/acs/commons/it/build/ScrMetadataIT.java) and even to get this to run I had to fix a number of problems which were apparently as-of-yet undetected.

In order to maintain the quality standards for this project, I think the only option at this point is to rollback all of these changes.

Specifically, I'm proposing to:

This is a non-trivial amount of work, but it is quantifiable. We know what has been committed to master since 6880fc4b88156c165fd76b60550042721015dbab whereas we do not know the amount of outstanding work necessary to get 4.0 out the door with the current state of master.

kwin commented 5 years ago

Applying https://github.com/Adobe-Consulting-Services/acs-aem-commons/pull/1739 should fix the build of master (without the need for any revert). Then we only have the issue mentioned in https://github.com/Adobe-Consulting-Services/acs-aem-commons/issues/1722#issuecomment-457878232 to solve.

justinedelson commented 5 years ago

@kwin this is not the case. If you run the IT I created and linked to above, you'll see a large number of diffs between the last release and the current state of master.

justinedelson commented 5 years ago

now, some of those results could be false positives, but without going through them 1 by 1, I don't know how anyone could tell.

justinedelson commented 5 years ago

here's the current output (once the fixes in that branch have been applied):

Property sling.servlet.paths on component com.adobe.acs.commons.contentfinder.querybuilder.impl.viewhandler.QueryBuilderViewHandler has been removed.
Property process.label on component com.adobe.acs.commons.dam.audio.impl.FFMpegAudioEncodeProcess has been removed.
Property process.name on component com.adobe.acs.commons.dam.audio.watson.impl.TranscriptionProcess has been removed.
Property process.label on component com.adobe.acs.commons.dam.impl.AddWatermarkToRenditionProcess has been removed.
Property service.ranking on component com.adobe.acs.commons.dam.impl.AssetsFolderPropertiesSupport has been removed.
Property sling.filter.scope on component com.adobe.acs.commons.dam.impl.AssetsFolderPropertiesSupport has been removed.
Property sling.filter.pattern on component com.adobe.acs.commons.dam.impl.AssetsFolderPropertiesSupport has been removed.
Property sling.servlet.methods on component com.adobe.acs.commons.dam.impl.AssetsFolderPropertiesSupport has been removed.
Property sling.servlet.resourceTypes on component com.adobe.acs.commons.dam.impl.AssetsFolderPropertiesSupport has been removed.
Property process.label on component com.adobe.acs.commons.dam.impl.MatteRenditionProcess has been removed.
Property sling.servlet.paths on component com.adobe.acs.commons.designer.impl.OptionsServlet has different values (was: {/apps/acs-commons/components/utilities/designer/clientlibsmanager/options}, is: {/apps/acs-commons/components/utilities/designer/clientlibsmanager/options.json})
Property sling.servlet.extensions on component com.adobe.acs.commons.designer.impl.OptionsServlet has been removed.
Property provider.roots on component com.adobe.acs.commons.genericlists.impl.GenericListJsonResourceProvider has been removed.
Property provider.ownsRoots on component com.adobe.acs.commons.genericlists.impl.GenericListJsonResourceProvider has been removed.
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.hc.impl.HealthCheckStatusEmailer has different values (was: {Health\ Check\ Status\ E-mailer\ running\ every\ [\ {scheduler.expression}\ ]\ using\ Health\ Check\ Tags\ [\ {hc.tags}\ ]\ to\ [\ {recipients.email-addresses}\ ]}, is: {Health Check Status E-mailer running every [ {scheduler.expression} ] using Health Check Tags [ {hc.tags} ] to [ {recipients.email-addresses} ]})
Property hc.tags on component com.adobe.acs.commons.hc.impl.SMTPMailServiceHealthCheck has different values (was: {integrations,smtp,email}, is: {[integrations, smtp, email]})
Property httpcache.config.extension.user-groups.allowed on component com.adobe.acs.commons.httpcache.config.impl.GroupHttpCacheConfigExtension has been removed.
Property httpcache.config.filter-scope on component com.adobe.acs.commons.httpcache.config.impl.HttpCacheConfigImpl has been removed.
Property httpcache.config.cache-handling-rules.pid on component com.adobe.acs.commons.httpcache.config.impl.HttpCacheConfigImpl has been removed.
Property httpcache.config.extension.resource-types.allowed on component com.adobe.acs.commons.httpcache.config.impl.ResourceTypeHttpCacheConfigExtension has been removed.
Property httpcache.config.extension.resource-types.page-content on component com.adobe.acs.commons.httpcache.config.impl.ResourceTypeHttpCacheConfigExtension has been removed.
Property httpcache.engine.cache-handling-rules.global on component com.adobe.acs.commons.httpcache.engine.impl.HttpCacheEngineImpl has been removed.
Property event.topics on component com.adobe.acs.commons.httpcache.invalidator.event.JCRNodeChangeEventHandler has different values (was: {org/apache/sling/api/resource/Resource/CHANGED
org/apache/sling/api/resource/Resource/ADDED
org/apache/sling/api/resource/Resource/REMOVED}, is: {[org/apache/sling/api/resource/Resource/CHANGED,org/apache/sling/api/resource/Resource/ADDED,org/apache/sling/api/resource/Resource/REMOVED]})
Property httpcache.config.jcr.expiretimeinseconds on component com.adobe.acs.commons.httpcache.store.jcr.impl.JCRHttpCacheStoreImpl has been removed.
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.images.impl.NamedImageTransformerImpl has different values (was: {Transformer:\ {name}}, is: {Transformer: {name}})
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.logging.impl.JsonEventLogger has different values (was: {Logger:\ {event.logger.category}\ for\ events\ matching\ '{event.filter}'\ on\ '{event.topics}'}, is: {Logger: {event.logger.category} for events matching '{event.filter}' on '{event.topics}'})
Property event.filter on component com.adobe.acs.commons.logging.impl.JsonEventLogger has different values (was: {}, is: {(event.topics=*)})
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.logging.impl.SyslogAppender has different values (was: {Host:\ {host}\,\ for\ loggers\ [{loggers}]}, is: {Host: {host}, for loggers [{loggers}]})
Property sling.servlet.extensions on component com.adobe.acs.commons.mcp.impl.GenericReportExcelServlet has different values (was: {xlsx
xls}, is: {xlsx,xls})
Property sling.servlet.extensions on component com.adobe.acs.commons.mcp.impl.ProcessErrorReportExcelServlet has different values (was: {xlsx
xls}, is: {xlsx,xls})
Property osgi.http.whiteboard.context.select on component com.adobe.acs.commons.quickly.impl.QuicklyFilter has different values (was: {(osgi.http.whiteboard.context.name=*)}, is: {(osgi.http.whiteboard.context.name=*})
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.replication.dispatcher.impl.DispatcherFlushRulesImpl has different values (was: {Rule:\ {prop.replication-action-type}\,\ for\ Hierarchy:\ [{prop.rules.hierarchical}]\ or\ Resources:\ [{prop.rules.resource-only}]}, is: {Rule: {prop.replication-action-type}, for Hierarchy: [{prop.rules.hierarchical}] or Resources: [{prop.rules.resource-only}]})
Property event.topics on component com.adobe.acs.commons.replication.packages.automatic.impl.ConfigurationUpdateListener has different values (was: {org/apache/sling/api/resource/Resource/ADDED
org/apache/sling/api/resource/Resource/CHANGED
org/apache/sling/api/resource/Resource/REMOVED}, is: {[org/apache/sling/api/resource/Resource/ADDED,org/apache/sling/api/resource/Resource/CHANGED,org/apache/sling/api/resource/Resource/REMOVED]})
Property event.topics on component com.adobe.acs.commons.replication.status.impl.JcrPackageReplicationStatusEventHandler has different values (was: {com/day/cq/replication
com/adobe/granite/replication}, is: {[com/day/cq/replication,com/adobe/granite/replication]})
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.rewriter.impl.ResourceResolverMapTransformerFactory has different values (was: {Pipeline\ Type:\ {pipeline.type}\,\ for\ element:attributes\ [{attributes}]}, is: {Pipeline Type: {pipeline.type}, for element:attributes [{attributes}]})
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.rewriter.impl.StaticReferenceRewriteTransformerFactory has different values (was: {Pipeline:\ {pipeline.type}}, is: {Pipeline: {pipeline.type}})
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.users.impl.EnsureGroup has different values (was: {Ensure\ Group:\ {operation}\ {principalName}}, is: {Ensure Group: {operation} {principalName}})
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.users.impl.EnsureServiceUser has different values (was: {Ensure\ Service\ User:\ {operation}\ {principalName}}, is: {Ensure Service User: {operation} {principalName}})
Property event.topics on component com.adobe.acs.commons.util.impl.ComponentDisabler has different values (was: {org/osgi/framework/BundleEvent/STARTED
org/osgi/framework/ServiceEvent/REGISTERED}, is: {[org/osgi/framework/BundleEvent/STARTED,org/osgi/framework/ServiceEvent/REGISTERED]})
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.util.impl.DelegatingServletFactoryImpl has different values (was: {Target\ type:\ {prop.target-resource-type}}, is: {Target type: {prop.target-resource-type}})
Property service.ranking on component com.adobe.acs.commons.util.impl.UrlFilter has been removed.
Property wcmEditorTouchURL on component com.adobe.acs.commons.wcm.impl.AuthorUIHelperImpl has been removed.
Property wcmEditorClassicURL on component com.adobe.acs.commons.wcm.impl.AuthorUIHelperImpl has been removed.
Property damEditorTouchURL on component com.adobe.acs.commons.wcm.impl.AuthorUIHelperImpl has been removed.
Property damEditorClassicURL on component com.adobe.acs.commons.wcm.impl.AuthorUIHelperImpl has been removed.
Property sling.filter.scope on component com.adobe.acs.commons.wcm.impl.ComponentErrorHandlerImpl has different values (was: {component}, is: {COMPONENT})
Property publish.html on component com.adobe.acs.commons.wcm.impl.ComponentErrorHandlerImpl has been removed.
Property exclude.all on component com.adobe.acs.commons.wcm.impl.DynamicClassicUiClientLibraryServlet has different values (was: {false}, is: {})
Property exclude.all on component com.adobe.acs.commons.wcm.impl.DynamicTouchUiClientLibraryServlet has different values (was: {false}, is: {})
Property sling.servlet.resourceTypes on component com.adobe.acs.commons.wcm.impl.SiteMapServlet has different values (was: {}, is: {sling/servlet/default})
Property webconsole.configurationFactory.nameHint on component com.adobe.acs.commons.wcm.impl.SiteMapServlet has different values (was: {Site\ Map\ for:\ {externalizer.domain}\,\ on\ resource\ types:\ [{sling.servlet.resourceTypes}]}, is: {Site Map for: {externalizer.domain}, on resource types: [{sling.servlet.resourceTypes}]})
Property sling.servlet.methods on component com.adobe.acs.commons.workflow.bulk.execution.impl.servlets.InitFormServlet has been removed.
Property workflow.older-than on component com.adobe.acs.commons.workflow.bulk.removal.impl.WorkflowInstanceRemoverScheduler has different values (was: {0}, is: {})
Property wf-package.types on component com.adobe.acs.commons.workflow.impl.WorkflowPackageManagerImpl has different values (was: {cq:Page
cq:PageContent
dam:Asset}, is: {cq:Page,cq:PageContent,dam:Asset})
Property process.label on component com.adobe.acs.commons.workflow.process.impl.BrandPortalSyncProcess has different values (was: {Brand\ Portal\ Sync}, is: { ACS AEM Commons - Brand Portal Sync})
Property process.label on component com.adobe.acs.commons.workflow.process.impl.DamMetadataPropertyResetProcess has different values (was: {DAM\ Metadata\ Property\ Reset}, is: { ACS AEM Commons - DAM Metadata Property Reset})
Property process.label on component com.adobe.acs.commons.workflow.process.impl.ParameterizedActivatePageProcess has different values (was: {Parameterized\ Activate\ Resource\ Process}, is: { ACS AEM Commons - Parameterized Activate Resource Process})
Property process.label on component com.adobe.acs.commons.workflow.process.impl.ParameterizedDeactivatePageProcess has different values (was: {Parameterized\ Deactivate\ Resource\ Process}, is: { ACS AEM Commons - Parameterized Deactivate Resource Process})
Property process.label on component com.adobe.acs.commons.workflow.process.impl.ReplicateWithOptionsWorkflowProcess has different values (was: {Replicate\ with\ Options}, is: { ACS AEM Commons - Replicate with Options})
Property process.label on component com.adobe.acs.commons.workflow.process.impl.SyntheticWrapperWorkflowProcess has different values (was: {Synthetic\ Workflow\ Wrapper}, is: { ACS AEM Commons - Synthetic Workflow Wrapper})
Property process.label on component com.adobe.acs.commons.workflow.process.impl.WorkflowDelegationStep has different values (was: {Workflow Delegation}, is: { ACS AEM Commons - Workflow Delegation})
kwin commented 5 years ago

I just cross-checked some of them, and all were in fact false positives. Have you found a real issue? I used in the past the tool from https://github.com/jsedding/osgi-ds-metatype-diff to compare metatype and component metadata.

adamcin commented 5 years ago

@justinedelson I think you should also include checking for the presence of the metatype v1.4.0 xmlns in your ITs. While this namespace does not prevent successful builds, it appears to be incompatible with all existing AEM 6.4 service packs at runtime, in that it prevents affected configurations from being edited in Felix Web Console.

justinedelson commented 5 years ago

@kwin this one I know is not a false positive:

Property wcmEditorTouchURL on component com.adobe.acs.commons.wcm.impl.AuthorUIHelperImpl has been removed.
kwin commented 5 years ago

Indeed but quite easy to fix. Right now the annotation is not used properly (i.e. will not end up as component metadata property). Either the activate method needs to leverage the AuthorUIHelperImpl.Config interface or it should be annotated with it. Compare also with https://osgi.org/specification/osgi.cmpn/7.0.0/service.component.html#service.component-component.property.types

Component property types can be used in two ways:

Component property types can be used to annotate the component implementation class, along side the Component annotation. The annotation usage can specify property values which can be different than the default values declared in the component property type.

To be used in this way, the component property type must be annotated with the ComponentPropertyType meta-annotation so that, at build time, the annotation is recognized as a component property type.

Component property types can be used as parameter types in the component's constructor and life cycle methods, or as field types for activation fields. The component implementation can use objects of a component property type at runtime to access component property values in a type safe manner.

To be used in this way, it is recommended the component property type be annotated with the ComponentPropertyType meta-annotation but it is not required.

justinedelson commented 5 years ago

That's not the problem (or at least not the problem I'm seeing) -- the problem in this case is that the property name was changed from wcmEditorTouchURL to wcmEditorTouchUrl. I don't know if this was intentional or accidental.

But the individual issues aren't really my point. My point is that to get master into a releasable state following the current course seems to take an undefined amount of work. And after doing that undefined amount of work, we will still end up with a release which is non-backwards compatible (due to the dash-in-property-name issue). And to what benefit?

@kwin if you (or anyone else) wants to make a concrete proposal for how to bring master into a releasable state, by all means do so. But I don't view the current state of master as acceptable and strongly feel like something needs to be done.

FWIW, I tried @jsedding 's tool and get this result:

Exception in thread "main" java.lang.RuntimeException: org.apache.felix.scr.impl.parser.ParseException: Exception during parsing
    at net.distilledcode.tools.osgi.DeclarativeServices.lambda$toComponentMetadata$3(DeclarativeServices.java:60)
    at java.util.function.Function.lambda$andThen$1(Function.java:88)
    at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
    at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
    at java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
    at net.distilledcode.tools.osgi.DeclarativeServices.readComponentMetadata(DeclarativeServices.java:42)
    at net.distilledcode.tools.osgi.MetadataDiff$BundleMetadata.<init>(MetadataDiff.java:68)
    at net.distilledcode.tools.osgi.MetadataDiff.diff(MetadataDiff.java:33)
    at net.distilledcode.tools.osgi.MetadataDiff.main(MetadataDiff.java:28)
Caused by: org.apache.felix.scr.impl.parser.ParseException: Exception during parsing
    at org.apache.felix.scr.impl.xml.XmlHandler.startElement(XmlHandler.java:439)
    at org.apache.felix.scr.impl.parser.KXml2SAXParser.parseXML(KXml2SAXParser.java:76)
    at net.distilledcode.tools.osgi.DeclarativeServices.lambda$toComponentMetadata$3(DeclarativeServices.java:57)
    ... 13 more
Caused by: org.apache.felix.scr.impl.parser.ParseException: Missing bundle entry sling.servlet.paths=/bin/wcm/contentfinder/qb/view
    at org.apache.felix.scr.impl.xml.XmlHandler.readPropertiesEntry(XmlHandler.java:552)
    at org.apache.felix.scr.impl.xml.XmlHandler.startElement(XmlHandler.java:346)
    ... 15 more

which is why I went a different way.

davidjgonzalez commented 5 years ago

Agree w/ @justinedelson. The "nice" thing about reverting back to Felix Scr, is we can always move to OSGi R# in the future when we come up with a plan based on "today's" learnings, but in the meantime, we can continue to release back-compat fixes/enhancements with confidence.

And FWIW this work is NOT a waste - what we've learned here will be invaluable when we make the inevitable move to OSGi R#.

I spent some time over the weekend "POC"ed reverting to Felix SCR commit and merge/resolve-ing later commits:

Most could be handled by resolving the conflicts vs. creating new code. IIRC, the few exceptions where the new HTTP Cache, and the use of the ClosableQueryBuilder. Most other commits went in w/ relative ease.

adamcin commented 5 years ago

@davidjgonzalez what difficulties did you have with CloseableQueryBuilder? Can I help resolve them?

justinedelson commented 5 years ago

@davidjgonzalez I think you mean that this work wasn’t a waste.

justinedelson commented 5 years ago

Unless a good reason arises in the next two days, I plan to have this issue complete and a releasable branch by Monday.

davidjgonzalez commented 5 years ago

@adamcin it wasn't difficult per say, rather I was only tackling commits that didn't require me "writing code"; rather I was using intellij's "resolve conflicts" feature to selectively include line deltas between "mine" and "yours". I figured the stricter the guardrails on my changes, the better.

I do think we should be careful how we merge things back in to reduce confusion, conflicts. things are a bit tangled over 4 months of commits.

@justinedelson im not sure what your "plan" looks like - and/or how others can help out in an organized fashion. I should have some time this weekend to help out.

justinedelson commented 5 years ago

@davidjgonzalez as far as I can tell, the cherry picking is best done by one person. It'll get messy fast if it is done collaboratively or in multiple branches. I've been using your work a bit as a baseline, but working a bit more methodically. Specifically, I've:

Not the most fun work, but it is what it is.

justinedelson commented 5 years ago

The only thing I would ask of @Adobe-Consulting-Services/acs-aem-commons-contributors is to not commit anything to master for the next few days.

jsedding commented 5 years ago

I have pushed a commit to improve the error message of osgi-ds-metatype-diff for this scenario. With that fix in place, I quickly found that several classes use the @Component's properties field instead of the property field. The former is meant to point to a properties file inside the bundle, whereas the latter allows defining properties as strings in-line. These are easily fixed (essentially s/properties=/property=/g), but there are more issues.

kwin commented 5 years ago

For one such issue I opened now https://github.com/Adobe-Consulting-Services/acs-aem-commons/issues/1744 already. Also I requested a better validation at build time of these issues via https://github.com/bndtools/bnd/issues/2927. Hopefully a future bnd version will already fail at build time in such cases.

justinedelson commented 5 years ago

I have substantially completed this work on the branch release/4.x

I have left any newly created components with the r6 annotations as there is no compatibility concern there. But anything using SCR annotations in 3.19.0 still uses SCR annotations.

I'm now seeing some warnings on installation in AEM related to the AnnotatedMBeans. Since master is unstable, it is hard to tell if this is a problem specific to my cherry picking or a general problem.

jsedding commented 5 years ago

I have made some further improvements to osgi-ds-metatype-diff and at this point can only support @justinedelson in reverting the changes. Without looking into the details, there currently are lots of small issues.

That said, I do see value in migrating to the official OSGi annotations, including promoting latest best practices and allowing updates to the toolchain (I expect tools to eventually drop support for legacy annotations) and benefiting from new DS/MetaType features not supported by legacy annotations. I would therefore recommend to move the code base over to the official OSGi annotations.

A viable strategy is to tackle the work class by class and to re-run the diff tool regularly in order to catch (unintended) differences. Having an easy way to see the differences proved a valuable tool in learning about the finer intricacies of the official OSGi annotations.

justinedelson commented 5 years ago

This work is now complete. I ended up doing it slightly differently. I have executed it as follows:

At last check, the code coverage reported from coveralls on the release/4.x branch was the same as the last successful report from master. CodeClimate is clean locally.

Barring any other concrete solution, on Monday I will do the following:

Note: In general, I have kept the annotation styles used by individual components as they were at commit 84f76f7faccca34062008ea61499e2dd201c7a90, i.e. if a component was using OSGi annotations (for example, the Adobe I/O stuff), that's still using the OSGi annotations.

justinedelson commented 5 years ago

Branches created have been merged to master and removed.