Cloud Manager sonar analysis is failing due to critical issues related with CQRules:CQBP-84

silvamiguelez commented 3 years ago

Required Information

AEM Version: 6.5.5
ACS Commons Version: 5.0.4
Reproducible on Latest? yes

Expected Behavior

Cloud Manager code analysis is ok

Actual Behavior

Cloud Manager code analysis is failing and breaking the build as a result due to Sonar critical issues

Steps to Reproduce

Build a project including acs-commons library dependency (included in the "all" package) and suddenly since the last Cloud Manager release (today morning) builds are failing with the following critical sonar issues:

The product interface com.day.cq.replication.Replicator annotated with @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.mcp.impl.processes.renovator.ReplicatorQueue contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.4.jar. (Critical) CQRules:CQBP-84 -> Tracked in #2614
The product interface org.apache.sling.api.SlingHttpServletRequest annotated with @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.redirectmaps.impl.FakeSlingHttpServletRequest contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.4.jar. (Critical) CQRules:CQBP-84 -> Tracked in #2648
The product interface org.apache.sling.api.request.RequestPathInfo annotated with @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.synth.impl.SynthesizedSlingHttpServletRequest$WrappedRequestPathInfo contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.4.jar. (Critical) CQRules:CQBP-84 -> Tracked in #2658
The product interface com.adobe.granite.workflow.exec.WorkItem annotated with @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.workflow.synthetic.impl.granite.SyntheticWorkItem contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.4.jar. (Critical) CQRules:CQBP-84

Links

joerghoh commented 3 years ago

While I would accept item (1) and (4) (well, these are AEM product interfaces, and it doesn't make that much sense to implement them here), I would question (2) and (3), the usecase looks more reasonable.

We probably should move this code to Sling and maintain it there alongside the interfaces.

YegorKozlov commented 3 years ago

(2) can be ignored. The Redirect map feature is not AEM as a Cloud Service compatible and is disabled on this platform.

kwin commented 3 years ago

Even if 2 is not compatible with AEMaaCS it will still trigger the sonar issue (because the code is included). @YegorKozlov Do you suggest that we build an AEMaaCS specific bundle stripping certain classes?

YegorKozlov commented 3 years ago

It might make sense to define a AEMaaCS Maven profile which will exclude not compatible code and corresponding tests.

martinnoble commented 3 years ago

We get the same 4 error reports on Cloud Manager for Adobe Manager Services enterprise (eg non-cloud service) running AEM 6.5.5 where we embed ACS commons, so a AEMaaCS profile would not resolve this for all use cases.

by-4x1 commented 3 years ago

Got below from our Adobe Support person ... Which is this 2604 issue ...

the latest release of the Cloud Manager on June 10th 1 has introduced changes that detect rule CQBP-84 better in embedded bundles, resulting in some annotations in the ACS Commons package being flagged as critical and failing the cloud pipeline builds.

In consequence, this is an issue with ACS Commons that needs to be fixed, and not with the embedding of the library or its support with AEMaaCS (the version 5.0.4 of ACS Commons is supported by AEMaaCS).

This will need to be fixed in ACS Commons, you can see the bug report here 2.

shsteimer commented 3 years ago

I took a brief look at these and I'm wondering if 2 & 3 could potentially be addressed by extending SlingHttpServletRequestWrapper rather than implementing SlingHttpServletRequest.

Also regarding 3, I don't see any usages in the project for synthesizer or the entire com.adobe.acs.commons.synth package, and it isn't documented on https://adobe-consulting-services.github.io/acs-aem-commons/. Should we consider deprecating that functionality for eventual removal?

justinedelson commented 3 years ago

FWIW, this

the latest release of the Cloud Manager on June 10th has introduced a change in the severity of the rule CQBP-84, resulting in some annotations in the ACS Commons package being flagged as critical and failing the tests.

Is not entirely correct. The severity of this rule has always been Critical -- implementing interfaces which are only intended to be implemented by AEM was and remains an important issue and creates a variety of risks for customers.

What changed in last week's release is that certain ways of embedding/referencing dependencies were not consistently handled. In other words, these issues were there all along but undetected because of gaps in how the build-time dependency graph was materialized during quality analysis.

YegorKozlov commented 3 years ago

For (1) , com.adobe.acs.commons.mcp.impl.processes.renovator.ReplicationQueue does not need to implement Replicator at all. Removing 'implements Replicator' does not change the behavior and functionality. Unit tests have a couple of assumptions that ReplicationQueue_ implements Replicator, but not the main code.

kristian-wright commented 3 years ago

Hi, can someone confirm that this issue (and 2606, 2607 and 2608) is being addressed by ACS? I's affecting multiple implementations. Thanks!

Rk-F5 commented 3 years ago

Yeah please someone let us know the recent update on this issue?#2609

Kiratsingh20 commented 3 years ago

Hi, are the issues 2606, 2607 and 2608 being addressed by ACS. Thank you!

kwin commented 3 years ago

This is open-source, if you urgently need a fix for this, please come up with PRs. Only the issue 2. from above has been fixed meanwhile (but not released yet) in #2615 .

prakashkadhati commented 3 years ago

is there any update on this issue? we are using ACS Commons 5.0.12 and still we are seeing those critical issues in cloud manager Sonar analysis.

adamcin commented 3 years ago

A couple of these items are addressed in #2734 and #2732

tauba commented 2 years ago

Currently facing these errors on the pipeline

adobe/consulting:acs-aem-commons-ui.apps:5.0.14,0,  The product interface com.day.cq.search.Query annotated with                        @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.search.CloseableQuery                                                 contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.14.jar.           
adobe/consulting:acs-aem-commons-ui.apps:5.0.14,0,  The product interface org.apache.sling.api.request.RequestPathInfo annotated with   @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.synth.impl.SynthesizedSlingHttpServletRequest$WrappedRequestPathInfo  contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.14.jar.           
adobe/consulting:acs-aem-commons-ui.apps:5.0.14,0,  The product interface org.apache.sling.api.request.RequestPathInfo annotated with   @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.wcm.vanity.impl.ExtensionlessRequestWrapper$RequestPathInfoWrapper    contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.14.jar.           
adobe/consulting:acs-aem-commons-ui.apps:5.0.14,0,  The product interface com.adobe.granite.workflow.exec.WorkItem annotated with       @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.workflow.synthetic.impl.granite.SyntheticWorkItem                     contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.14.jar.           
adobe/consulting:acs-aem-commons-ui.apps:5.0.14,0,  The product interface com.adobe.granite.workflow.WorkflowSession annotated with     @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.workflow.synthetic.impl.granite.SyntheticWorkflowSession              contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.14.jar.           
adobe/consulting:acs-aem-commons-ui.apps:5.0.14,0,  The product interface com.day.cq.search.Query annotated with                        @ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.wrap.cqsearch.QueryIWrap                                              contained in /apps/acs-commons/install/acs-aem-commons-bundle-5.0.14.jar.

any update for this? (currently using 5.0.14)

davidjgonzalez commented 2 years ago

@tauba some of these should be fixed in 5.1.0 - not sure about the Query one though ...

adamcin commented 2 years ago

@davidjgonzalez @justinedelson I see @tauba has pointed out that CloseableQuery (which extends com.day.cq.search.Query to add AutoCloseable) is now flagged by CQBP-84 because the @ProviderType annotation has been added to all the cq-search interfaces in recent cloudready sdks. This is not present in uber-jar 6.5.8, for example. We can probably isolate the com.adobe.acs.commons.search and com.adobe.acs.commons.wrap.cqsearch packages into separate classic-only bundles here, so that CloseableQuery can be opted into for classic project deployments without a major API version change, but this would leave cloudservice projects without an AutoCloseable Query. Since Adobe has modified the source for cq-search to restrict implementation, can't Adobe also just implement AutoCloseable on the Query interface in the next cloudready sdk version to eliminate the need for a wrapper type?

justinedelson commented 2 years ago

@adamcin This seems like a valid feature request. IIRC, this package is now considered an Assets feature but I could be wrong about that.

Adobe-Consulting-Services / acs-aem-commons