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.
Solved! Go to Solution.
Views
Replies
Total Likes
@P_V_Nair, I do not think this is possible. Nevertheless you can try below options:
@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.
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:
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:
// 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();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.
result = repository.loginService(subServiceName, workspace); } catch (RepositoryException e) { log.error("Unable to create session", e); } return result; }
// 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.
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.
@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, I do not think this is possible. Nevertheless you can try below options:
@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.
@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?
@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.
Views
Likes
Replies
Views
Likes
Replies
Views
Likes
Replies