Adobe Experience Manager Sites & More

P_V_Nair · 4/8/22

I have a event listener class to call a service method as below. This piece of code works well and calls the service method when the event is triggered. But,when this code went through the automatic code scanning tool sonar cube in Adobe cloud, we got a critical finding on the resolver object declaration as highlighted in bold red below. Below was the critical finding -

Usage of org.apache.sling.api.resource.ResourceResolver as a field is not thread safe.

So I removed the global declaration of resolver object and tried to create the object inside the try method as below

try (ResourceResolver resourceResolver = resolverFactory.getServiceResourceResolver(ServiceUser.REVIEW_TASK_SERVICE_USER.getAuthMap())) {

This will automatically handle the resolver.close() as well. But the issue is, in this approach, we cannot declare the resolver object globally, which will be available both in activate and onevent methods. This declaration of resolver object inside try method will not trigger the listener and my listener is no longer getting triggered.

package com.inc.aeminc.core.listeners;

import javax.jcr.RepositoryException;
import javax.jcr.Session;
import javax.jcr.observation.Event;
import javax.jcr.observation.EventIterator;
import javax.jcr.observation.EventListener;
import javax.jcr.observation.ObservationManager;

import org.apache.sling.api.resource.LoginException;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.api.resource.ResourceResolverFactory;
import org.apache.sling.jcr.api.SlingRepository;
import org.osgi.service.component.ComponentContext;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Deactivate;
import org.osgi.service.component.annotations.Reference;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.inc.aeminc.core.services.CodeGeneratorService;
import com.inc.aeminc.core.sling.ServiceUser;

@component(service = EventListener.class, immediate = true)
public class ModelFragmentListener implements EventListener {

/**
* Logger
*/
private static final Logger log = LoggerFactory.getLogger(ModelFragmentListener.class);

/**
* Resource Resolver Factory
*/
@reference
private ResourceResolverFactory resolverFactory;

@reference
private SlingRepository repository;

@reference
private CodeGeneratorService CodeGenerator;

private ObservationManager observationManager;

private ResourceResolver resourceResolver;

@activate
protected void activate(ComponentContext componentContext) {

log.info("Activating the observation");

try {
resourceResolver = resolverFactory.getServiceResourceResolver(ServiceUser.REVIEW_TASK_SERVICE_USER.getAuthMap());

/**
* Adapting the resource resolver to session object
*/
Session session = resourceResolver.adaptTo(Session.class);
if (null != session) {
observationManager = session.getWorkspace().getObservationManager();
}

log.info("Session created for ModelFragmentListener");

/**
* Adding the event listener
*/
observationManager.addEventListener(this,
Event.PROPERTY_ADDED | Event.PROPERTY_CHANGED, "/content/dam/testpath",
true, null, null, false);
} catch (RepositoryException | LoginException e) {
log.error("An exception Occured in code Generator ", e);
}
}

@deactivate
protected void deactivate() throws RepositoryException {

if (observationManager != null) {
observationManager.removeEventListener(this);
}
}

@Override
public void onEvent(EventIterator events) {
try {

while (events.hasNext()) {

final Event event = events.nextEvent();
String path = "";
String property = "";
try {
switch (event.getType()) {
case Event.PROPERTY_ADDED:
path = event.getPath();
property = path.substring(path.lastIndexOf("/") + 1);
if ((property.equals("lastFragmentSave")) && ((path.startsWith("/content/dam/testpath/creative-assets")) ||
(path.startsWith("/content/dam/testpath/team-assets")))) {
log.info("Property added: {} ", event.getPath());
path = path.substring(0, path.lastIndexOf("/"));
CodeGenerator.createCode(resourceResolver, path);
}

break;
case Event.PROPERTY_CHANGED:
path = event.getPath();
property = path.substring(path.lastIndexOf("/") + 1);
if ((property.equals("lastFragmentSave")) && ((path.startsWith("/content/dam/testpath/creative-assets")) ||
(path.startsWith("/content/dam/testpath/team-assets")))) {
log.info("Property added: {} ", event.getPath());
path = path.substring(0, path.lastIndexOf("/"));
CodeGenerator.createCode(resourceResolver, path);

}

break;
default:
log.info("Property noaction: {} ", event.getPath());
}

} catch (RepositoryException e) {
log.error("RepositoryException during template change listener ", e);
}

}

} catch (Exception e) {

log.error("Exception occurred", e);
}
}

}

This should be a common issue for any listener class in AEM. How do we declare the resolver object globally and make sure the same resolver object is available inside activate() and onevent() inside a listener class, also making sure it is to be thread safe?

Any leads would be much appreciated.

lukasz-m · 4/11/22

@P_V_Nair, I do not think this is possible. Nevertheless you can try below options:

Use SuppressWarnings annotation against session field on class level
```
@SuppressWarnings("AEM Rules:AEM-3")
private Session session;
```
What is interesting, this approach is applied on one of the class from ACS Commons - here is direct link. As you can see this is exactly the same scenario/case that you have. So this looks to be proper way to go.
You can create and use session object only on activate method level, but in that case you will need to close the session in the same method. I am afraid in that case your event listener will not be registered properly. You could of course not close the session - but this is nasty option I would not even consider.

View solution in original post

lukasz-m · 4/10/22

Hi @P_V_Nair,

In general in this specific case you do not need ResourceResolver at all. Looking into implementation, you are using ResourceResolver for 2 things:

To get jcr session.
To pass ResourceResolver object to other OSGi service.

You are not using any specific method from ResourceResolver on ModelFragmentListener class level. By specific method I mean, e.g. getResource etc.

I would suggest below changes to improve your implementation:

Use SlingRepository to get jcr session, this can be done like that:

// additional field on class level
private Session session;

@Activate
protected void activate() {
    log.info("Activating the observation");
    try {
        session = createSession();
        if (session != null) {
            observationManager = session.getWorkspace().getObservationManager();
            log.info("Session created for ModelFragmentListener");

            /**
            * Adding the event listener
            */
            observationManager.addEventListener(this,
                Event.PROPERTY_ADDED | Event.PROPERTY_CHANGED, "/content/dam/testpath",
                true, null, null, false);
        }
    } catch (RepositoryException | LoginException e) {
        log.error("An exception Occured in code Generator ", e);
    }
}

@Deactivate
protected void deactivate() throws RepositoryException {
    if (observationManager != null) {
        observationManager.removeEventListener(this);
    }
    if (session != null) {
        session.logout();
    }
 }

// additional method dedicated to create jcr session
private Session createSession() {
    Session result = null;
    try {
	String workspace = repository.getDefaultWorkspace();
        /**
        * probably you can get subServiceName easier - but I do not know how ServiceUser 
        * class is implemented in your code
        */
        String subServiceName = ServiceUser
            .REVIEW_TASK_SERVICE_USER
            .getAuthMap()
            .get("sling.service.subservice").toString();

	result = repository.loginService(subServiceName, workspace);
    } catch (RepositoryException e) {
        log.error("Unable to create session", e);
    }
    return result;
}

As you can see, above changes will give you full control on jcr session in activate and deactivate method. You do not need ResourceResolver to achieve that.

You should avoid implementation where you are passing ResourceResolver object into other class. While doing it, you can easily loose control related to ResourceResolver lifecycle. For instance, you can close it in one of the class you are passing it to. In given scenario I would recommend to refactor method createCode from your CodeGeneratorService. Proposed change:
```
// current version
createCode(ResourceResolver resourceResolver, String path);

// alternative options
createCode(ResourceResolverFactory resourceResolverFacrory, String path);

// proper way
createCode(String path);
```
Managing dedicated ResourceResolver on CodeGeneratorService class level is the right way. This will be safe and will guarantee you have full control when you are creating and closing your ResourceResolver object.
Alternatively you can pass ResourceResolverFactory instead of ResourceResolver, but the idea is still the same.

To sum up. It is a good practice to create ResourceResolver in class when it is really needed, so when you have to use method from API its exposing. In all other cases use different options. Design your code in the way that it is not needed to pass ResourceResolver object outside specific class.

P_V_Nair · 4/10/22

@lukasz-m Thanks a lot for the above suggestions and explanation.

private Session session

But in my code I was already using session object as a private variable as the same in your above suggestion and Adobe Sonar rules flagged this as below critical finding

Usage of javax.jcr.Session as a field is not thread safe.,Bug,Critical

Hence, I have modified my code as in my initial description to fix that issue so that session object is not created as a variable and I created it inside the method i need it.

As you see in my code, session object finding was fixed by that approach and the only one critical left was resolver object.

Do you think we can fix this by not declaring both session object and resolver object as variables?

P_V_Nair · 4/11/22

@lukasz-m Do you think there is a way here not to use session object as well as a field?

lukasz-m · 4/11/22

@P_V_Nair, I do not think this is possible. Nevertheless you can try below options:

Use SuppressWarnings annotation against session field on class level
```
@SuppressWarnings("AEM Rules:AEM-3")
private Session session;
```
What is interesting, this approach is applied on one of the class from ACS Commons - here is direct link. As you can see this is exactly the same scenario/case that you have. So this looks to be proper way to go.
You can create and use session object only on activate method level, but in that case you will need to close the session in the same method. I am afraid in that case your event listener will not be registered properly. You could of course not close the session - but this is nasty option I would not even consider.

P_V_Nair · 4/11/22

@lukasz-m Thanks a lot for your explanation. That makes sense.

P_V_Nair · 4/14/22

@lukasz-m I followed exactly your improvements in code and did the session creation as in the approach you said. Added suppress warnings as well. But now Sonar rules are flagging as below and that too critical.

Session should be logged out in finally block.,Code Smell,Critical,10min,AEM Rules:AEM-7

From my understanding we cannot do this , since we need this session to be active for the listener to be active and trigger the event , correct? It is not picking the suppress warnings as well. Any thoughts?

lukasz-m · 4/14/22

@P_V_Nair, I do not know how your code looks currently, but you could try to modify deactivate method like below:

@SuppressWarnings("AEM Rules:AEM-3")
private Session session;

@Deactivate
protected void deactivate(){
    try {
        if (observationManager != null) {
            observationManager.removeEventListener(this);
        }
    } catch (RepositoryException re) {
            log.error("Error deactivating JCR event listener", re);
    } finally {
        if (session != null) {
            session.logout();
            session = null;
        }
    }
 }

I assume you have Session filed on class level, and you are creating session in activate method and closing it in deactivate. This will keep session active until you stop your OSGi component/service. During this operation deactivate method will be run and will close the session.