Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
silvamiguelez opened this issue Jun 10, 2021 · 19 comments

Comments

@silvamiguelez
Copy link

silvamiguelez commented Jun 10, 2021

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:

  1. 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 ReplicatorQueue should not implement the Replicator interface #2614

  2. 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 Don't implement Provider type SlingHttpServletRequest in FakeSlingHttpServletRequest #2648

  3. 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 Replace implementation of Provider Type RequestPathInfo #2658

  4. 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

Related CQ rule documentation: https://experienceleague.adobe.com/docs/experience-manager-cloud-manager/using/how-to-use/custom-code-quality-rules.html?lang=en#product-apis-annotated-with-providertype-should-not-be-implemented-or-extended-by-customers

@joerghoh
Copy link
Contributor

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
Copy link
Contributor

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

@kwin
Copy link
Contributor

kwin commented Jun 13, 2021

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
Copy link
Contributor

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

@martinnoble
Copy link

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
Copy link

by-4x1 commented Jun 14, 2021

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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
Copy link

Rk-F5 commented Jul 8, 2021

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

@Kiratsingh20
Copy link

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

@kwin
Copy link
Contributor

kwin commented Jul 12, 2021

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
Copy link

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
Copy link
Contributor

adamcin commented Oct 27, 2021

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

@tauba
Copy link

tauba commented Nov 30, 2021

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
Copy link
Contributor

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

@adamcin
Copy link
Contributor

adamcin commented Dec 14, 2021

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests