Learn details about the custom code quality rules executed by Cloud Manager as part of code quality testing, based on best practices from AEM Engineering.
The code samples provided here are for illustrative purposes only. See SonarQube’s Concepts documentation to learn about its concepts and quality rules.
Full SonarQube rules are not available for download due to Adobe proprietary information. You can download the complete list of rules using this link. Continue reading this document for descriptions and examples of the rules.
The following section details SonarQube rules executed by Cloud Manager.
The methods Thread.stop()
and Thread.interrupt()
can produce hard-to-reproduce issues and, sometimes, security vulnerabilities. Their usage should be tightly monitored and validated. In general, message passing is a safer way to accomplish similar goals.
public class DontDoThis implements Runnable {
private Thread thread;
public void start() {
thread = new Thread(this);
thread.start();
}
public void stop() {
thread.stop(); // UNSAFE!
}
public void run() {
while (true) {
somethingWhichTakesAWhileToDo();
}
}
}
public class DoThis implements Runnable {
private Thread thread;
private boolean keepGoing = true;
public void start() {
thread = new Thread(this);
thread.start();
}
public void stop() {
keepGoing = false;
}
public void run() {
while (this.keepGoing) {
somethingWhichTakesAWhileToDo();
}
}
}
Using a format string from an external source (such a request parameter or user-generated content) can expose an application to denial of service attacks. There are circumstances where a format string may be externally controlled, but is only allowed from trusted sources.
protected void doPost(SlingHttpServletRequest request, SlingHttpServletResponse response) {
String messageFormat = request.getParameter("messageFormat");
request.getResource().getValueMap().put("some property", String.format(messageFormat, "some text"));
response.sendStatus(HttpServletResponse.SC_OK);
}
When executing HTTP requests from inside an AEM application, it is critical to ensure that proper timeouts are configured in order to avoid unnecessary thread consumption. Unfortunately, the default behavior of both Java™’s default HTTP Client, java.net.HttpUrlConnection
, and the commonly used Apache HTTP Components client is to never timeout, so timeouts must be explicitly set. As a best practice, these timeouts should be no more than 60 seconds.
@Reference
private HttpClientBuilderFactory httpClientBuilderFactory;
public void dontDoThis() {
HttpClientBuilder builder = httpClientBuilderFactory.newBuilder();
HttpClient httpClient = builder.build();
// do something with the client
}
public void dontDoThisEither() {
URL url = new URL("http://www.google.com");
URLConnection urlConnection = url.openConnection();
BufferedReader in = new BufferedReader(new InputStreamReader(
urlConnection.getInputStream()));
String inputLine;
while ((inputLine = in.readLine()) != null) {
logger.info(inputLine);
}
in.close();
}
@Reference
private HttpClientBuilderFactory httpClientBuilderFactory;
public void doThis() {
HttpClientBuilder builder = httpClientBuilderFactory.newBuilder();
RequestConfig requestConfig = RequestConfig.custom()
.setConnectTimeout(5000)
.setSocketTimeout(5000)
.build();
builder.setDefaultRequestConfig(requestConfig);
HttpClient httpClient = builder.build();
// do something with the client
}
public void orDoThis() {
URL url = new URL("http://www.google.com");
URLConnection urlConnection = url.openConnection();
urlConnection.setConnectTimeout(5000);
urlConnection.setReadTimeout(5000);
BufferedReader in = new BufferedReader(new InputStreamReader(
urlConnection.getInputStream()));
String inputLine;
while ((inputLine = in.readLine()) != null) {
logger.info(inputLine);
}
in.close();
}
ResourceResolver
objects obtained from the ResourceResolverFactory
consume system resources. Although there are measures in place to reclaim these resources when a ResourceResolver
is no longer in use, it is more efficient to explicitly close any opened ResourceResolver
objects by calling the close()
method.
One common misconception is that ResourceResolver
objects created using an existing JCR Session should not be explicitly closed or that doing so closed the underlying JCR Session. This is not the case. Regardless of how a ResourceResolver
is opened, it should be closed when no longer used. Since ResourceResolver
implements the Closeable
interface, it is also possible to use the try-with-resources
syntax instead of explicitly invoking close()
.
public void dontDoThis(Session session) throws Exception {
ResourceResolver resolver = factory.getResourceResolver(Collections.singletonMap("user.jcr.session", (Object)session));
// do some stuff with the resolver
}
public void doThis(Session session) throws Exception {
ResourceResolver resolver = null;
try {
resolver = factory.getResourceResolver(Collections.singletonMap("user.jcr.session", (Object)session));
// do something with the resolver
} finally {
if (resolver != null) {
resolver.close();
}
}
}
public void orDoThis(Session session) throws Exception {
try (ResourceResolver resolver = factory.getResourceResolver(Collections.singletonMap("user.jcr.session", (Object) session))){
// do something with the resolver
}
}
As described in the Sling documentation, bindings servlets by paths is discouraged. Path-bound servlets cannot use standard JCR access controls and, as a result, require additional security rigor. Rather than using path-bound servlets, it is recommended to create nodes in the repository and register servlets by resource type.
@Component(property = {
"sling.servlet.paths=/apps/myco/endpoint"
})
public class DontDoThis extends SlingAllMethodsServlet {
// implementation
}
In general, an exception should be logged exactly one time. Logging exceptions multiple times can cause confusion as it is unclear how many times an exception occurred. The most common pattern which leads to this is logging and throwing a caught exception.
public void dontDoThis() throws Exception {
try {
someOperation();
} catch (Exception e) {
logger.error("something went wrong", e);
throw e;
}
}
public void doThis() {
try {
someOperation();
} catch (Exception e) {
logger.error("something went wrong", e);
}
}
public void orDoThis() throws MyCustomException {
try {
someOperation();
} catch (Exception e) {
throw new MyCustomException(e);
}
}
Another common pattern to avoid is to log a message and then immediately throw an exception. This generally indicates that the exception message ends up duplicated in log files.
public void dontDoThis() throws Exception {
logger.error("something went wrong");
throw new RuntimeException("something went wrong");
}
public void doThis() throws Exception {
throw new RuntimeException("something went wrong");
}
In general, the INFO log level should be used to demarcate important actions and, by default, AEM is configured to log at the INFO level or above. GET and HEAD methods should only ever be read-only operations and thus do not constitute important actions. Logging at the INFO level in response to GET or HEAD requests is likely to create significant log noise, making it harder to identify useful information in log files. Logging when handling GET or HEAD requests should be either at the WARN or ERROR levels when something has gone wrong or at the DEBUG or TRACE levels if deeper troubleshooting information would be helpful.
This does not apply to access.log-type logging for each request.
public void doGet() throws Exception {
logger.info("handling a request from the user");
}
public void doGet() throws Exception {
logger.debug("handling a request from the user.");
}
As a best practice, log messages should provide contextual information about where in the application an exception has occurred. While context can also be determined by using stack traces, in general the log message is going to be easier to read and understand. As a result, when logging an exception, it is a bad practice to use the exception’s message as the log message. The exception message contains what went wrong whereas the log message should be used to tell a log reader what the application was doing when the exception happened. The exception message is still logged. By specifying your own message, the logs are easier to understand.
public void dontDoThis() {
try {
someMethodThrowingAnException();
} catch (Exception e) {
logger.error(e.getMessage(), e);
}
}
public void doThis() {
try {
someMethodThrowingAnException();
} catch (Exception e) {
logger.error("Unable to do something", e);
}
}
As the name suggests, Java™ exceptions should always be used in exceptional circumstances. As a result, when an exception is caught, it is important to ensure that log messages are logged at the appropriate level: either WARN or ERROR. This ensures that those messages appear correctly in the logs.
public void dontDoThis() {
try {
someMethodThrowingAnException();
} catch (Exception e) {
logger.debug(e.getMessage(), e);
}
}
public void doThis() {
try {
someMethodThrowingAnException();
} catch (Exception e) {
logger.error("Unable to do something", e);
}
}
Context is critical when understanding log messages. Using Exception.printStackTrace()
causes only the stack trace to be output to the standard error stream, losing all context. Further, in a multi-threaded application like AEM if multiple exceptions are printed using this method in parallel, their stack traces may overlap which produces significant confusion. Exceptions should be logged through the logging framework only.
public void dontDoThis() {
try {
someMethodThrowingAnException();
} catch (Exception e) {
e.printStackTrace();
}
}
public void doThis() {
try {
someMethodThrowingAnException();
} catch (Exception e) {
logger.error("Unable to do something", e);
}
}
Logging in AEM should always be done through the logging framework, SLF4J. Outputting directly to the standard output or standard error streams loses the structural and contextual information provided by the logging framework and may, sometimes, cause performance issues.
public void dontDoThis() {
try {
someMethodThrowingAnException();
} catch (Exception e) {
System.err.println("Unable to do something");
}
}
public void doThis() {
try {
someMethodThrowingAnException();
} catch (Exception e) {
logger.error("Unable to do something", e);
}
}
In general, paths which start with /libs
and /apps
should not be hardcoded as the paths they refer to are most commonly stored as paths relative to the Sling search path (which is set to /libs,/apps
by default). Using the absolute path may introduce subtle defects which would only appear later in the project lifecycle.
public boolean dontDoThis(Resource resource) {
return resource.isResourceType("/libs/foundation/components/text");
}
public void doThis(Resource resource) {
return resource.isResourceType("foundation/components/text");
}
Do not use the Sling Scheduler for tasks that require a guaranteed execution. Sling Scheduled Jobs guarantee execution and better suited for both clustered and non-clustered environments.
Refer to Apache Sling Eventing and Job Handling documentation to learn more about how Sling Jobs are handled in clustered environments.
The AEM API surface is under constant revision to identify APIs for which usage is discouraged and thus considered deprecated.
Often, these APIs are deprecated using the standard Java™ @Deprecated annotation and, as such, as identified by squid:CallToDeprecatedMethod
.
However, there are cases where an API is deprecated in the context of AEM but may not be deprecated in other contexts. This rule identifies this second class.
The following section details the OakPAL checks executed by Cloud Manager.
OakPAL is a framework, which validates content packages using a standalone Oak repository. It was developed by an AEM Partner and winner of the 2019 AEM Rockstar North America award.
The AEM API contains Java™ interfaces and classes which are only meant to be used, but not implemented, by custom code. For example, the interface com.day.cq.wcm.api.Page
is implemented by AEM only.
When new methods are added to these interfaces, those additional methods do not impact existing code which uses these interfaces and, as a result, the addition of new methods to these interfaces are considered to be backwards-compatible. However, if custom code implements one of these interfaces, that custom code has introduced a backwards-compatibility risk for the customer.
Interfaces and classes which are only intended to be implemented by AEM are annotated with org.osgi.annotation.versioning.ProviderType
or, sometimes, a similar legacy annotation aQute.bnd.annotation.ProviderType
. This rule identifies the cases where such an interface is implemented or a class is extended by custom code.
import com.day.cq.wcm.api.Page;
public class DontDoThis implements Page {
// implementation here
}
It has been a long-standing best practice that the /libs
content tree in the AEM content repository should be considered read-only by customers. Modifying nodes and properties under /libs
creates significant risk for major and minor updates. Modifications to /libs
is only made by Adobe through official channels.
A common problem that occurs on complex projects is where the same OSGi component is configured multiple times. This creates an ambiguity about which configuration is operable. This rule is “runmode-aware” in that it will only identify issues where the same component is configured multiple times in the same run mode or combination of run modes.
+ apps
+ projectA
+ config
+ com.day.cq.commons.impl.ExternalizerImpl
+ projectB
+ config
+ com.day.cq.commons.impl.ExternalizerImpl
+ apps
+ shared-config
+ config
+ com.day.cq.commons.impl.ExternalizerImpl
For security reasons, paths containing /config/
and /install/
are only readable by administrative users in AEM and should be used only for OSGi configuration and OSGi bundles. Placing other types of content under paths which contain these segments results in application behavior which unintentionally varies between administrative and non-administrative users.
A common problem is use of nodes named config
within component dialogs or when specifying the rich text editor configuration for inline editing. To resolve this, the offending node should be renamed to a compliant name. For the rich text editor configuration, use the configPath
property on the cq:inplaceEditing
node to specify the new location.
+ cq:editConfig [cq:EditConfig]
+ cq:inplaceEditing [cq:InplaceEditConfig]
+ config [nt:unstructured]
+ rtePlugins [nt:unstructured]
+ cq:editConfig [cq:EditConfig]
+ cq:inplaceEditing [cq:InplaceEditConfig]
./configPath = inplaceEditingConfig (String)
+ inplaceEditingConfig [nt:unstructured]
+ rtePlugins [nt:unstructured]
Similar to the Packages Should Not Contain Duplicate OSGi Configurations rule, this is a common problem on complex projects where the same node path is written to by multiple separate content packages. While using content package dependencies can be used to ensure a consistent result, it is better to avoid overlaps entirely.
The OSGi configuration com.day.cq.wcm.core.impl.AuthoringUIModeServiceImpl
defines the default authoring mode within AEM. Because the Classic UI has been deprecated since AEM 6.4, an issue is now raised when the default authoring mode is configured to Classic UI.
AEM Components which have a Classic UI dialog should always have a corresponding Touch UI dialog both to provide an optimal authoring experience and to be compatible with the Cloud Service deployment model, where Classic UI is not supported. This rule verifies the following scenarios:
dialog
child node) must have a corresponding Touch UI dialog (that is, a cq:dialog
child node).design_dialog
node) must have a corresponding Touch UI design dialog (that is, a cq:design_dialog
child node).The AEM Modernization Tools documentation provides details and tooling for how to convert components from Classic UI to Touch UI. Refer to The AEM Modernization Tools documentation for more details.
In order to be compatible with the Cloud Service deployment model, individual content packages must contain either content for the immutable areas of the repository (that is, /apps
and /libs
) or the mutable area (that is, everything not in /apps
or /libs
), but not both. For example, a package which includes both /apps/myco/components/text and /etc/clientlibs/myco
is not compatible with Cloud Service and causes an issue to be reported.
Refer to AEM Project Structure documentation for more details.
The rule Customer Packages Should Not Create or Modify Nodes Under /libs always applies.
Support for reverse replication is not available in Cloud Service deployments, as described in Release Notes: Removal of Replication Agents.
Customers using reverse replication should contact Adobe for alternative solutions.
AEM client libraries may contain static resources like images and fonts. As described in the Using Client-Side Libraries documentation, when using proxied client libraries these static resources must be contained in a child folder named resources
in order to be effectively referenced on the publish instances.
+ apps
+ projectA
+ clientlib
- allowProxy=true
+ images
+ myimage.jpg
+ apps
+ projectA
+ clientlib
- allowProxy=true
+ resources
+ myimage.jpg
With the move to Asset micro-services for asset processing on AEM Cloud Service, several workflow processes which were used in on-premise and AMS versions of AEM have become either unsupported or unnecessary.
The migration tool in the AEM Assets as a Cloud Service GitHub repository can be used to update workflow models during migration to AEM as a Cloud Service.
While the use of static templates has historically been common in AEM Projects, editable templates are highly recommended as they provide the most flexibility and support additional features not present in static templates. More information can be found in the Page Templates - Editable documentation.
Migration from static to editable templates can be largely automated using the AEM Modernization Tools.
The legacy Foundation Components (i.e. components under /libs/foundation
) have been deprecated for several AEM releases in favor of the Core Components. Usage of the legacy Foundation Components as the basis for custom components, whether by overlay or inheritance, is discouraged and should be converted to the corresponding core component.
This conversion can be facilitated by the AEM Modernization Tools.
AEM Cloud Service enforces a strict naming policy for run mode names and a strict ordering for those run modes. The list of supported run modes can be found in the Deploying to AEM as a Cloud Service documentation and any deviation from this is identified as an issue.
AEM Cloud Service requires that custom search index definitions (i.e. nodes of type oak:QueryIndexDefinition
) be direct child nodes of /oak:index
. Indexes in other locations must be moved to be compatible with AEM Cloud Service. More information on search indexes can be found in the Content Search and Indexing documentation.
AEM Cloud Service requires that custom search index definitions (i.e. nodes of type oak:QueryIndexDefinition
) must have the compatVersion
property set to 2
. Any other value is not supported by AEM Cloud Service. More information on search indexes can be found in the Content Search and Indexing documentation.
Hard-to-troubleshoot issues can occur when a custom search index definition node has unordered child nodes. To avoid these, it is recommended that all descendent nodes of an oak:QueryIndexDefinition
node be of type nt:unstructured
.
A properly defined custom search index definition node must contain a child node named indexRules
which, in turn must have at least one child. More information can be found in the Oak documentation.
AEM Cloud Service requires that custom search index definitions (that is, nodes of type oak:QueryIndexDefinition
) must be named following a specific pattern described on Content Search and Indexing.
AEM Cloud Service requires that custom search index definitions (i.e. nodes of type oak:QueryIndexDefinition
) have a type
property with the value set to lucene
. Indexing using legacy index types must be updated before migration to AEM Cloud Service. See the Content Search and Indexing documentation for more information.
AEM Cloud Service prohibits custom search index definitions (that is, nodes of type oak:QueryIndexDefinition
) from containing a property named seed
. Indexing using this property must be updated before migration to AEM Cloud Service. See the Content Search and Indexing documentation for more information.
AEM Cloud Service prohibits custom search index definitions (that is, nodes of type oak:QueryIndexDefinition
) from containing a property named reindex
. Indexing using this property must be updated before migration to AEM Cloud Service. See the Content Search and Indexing documentation for more information.
The following section lists the Dispatcher Optimization Tool (DOT) checks executed by Cloud Manager. Follow the links for each check for its GitHub definition and details.