SLING-8999 : Improve Sling filter handling
diff --git a/src/main/java/org/apache/sling/engine/impl/filter/FilterHandle.java b/src/main/java/org/apache/sling/engine/impl/filter/FilterHandle.java
index 7e6db26..4e281b1 100644
--- a/src/main/java/org/apache/sling/engine/impl/filter/FilterHandle.java
+++ b/src/main/java/org/apache/sling/engine/impl/filter/FilterHandle.java
@@ -28,7 +28,7 @@
private final Filter filter;
- private final Long filterId;
+ private final long filterId;
private final int order;
@@ -39,10 +39,11 @@
private AtomicLong time;
private FilterPredicate predicate;
-
+
FilterProcessorMBeanImpl mbean;
- FilterHandle(Filter filter, FilterPredicate predicate, Long filterId, int order, final String orderSource, FilterProcessorMBeanImpl mbean) {
+ FilterHandle(Filter filter, FilterPredicate predicate, long filterId, int order, final String orderSource,
+ FilterProcessorMBeanImpl mbean) {
this.filter = filter;
this.predicate = predicate;
this.filterId = filterId;
@@ -57,7 +58,7 @@
return filter;
}
- public Long getFilterId() {
+ public long getFilterId() {
return filterId;
}
@@ -103,6 +104,7 @@
* Note: this class has a natural ordering that is inconsistent with
* equals.
*/
+ @Override
public int compareTo(FilterHandle other) {
if (this == other || equals(other)) {
return 0;
@@ -113,17 +115,7 @@
} else if (order < other.order) {
return 1;
}
-
- // if the filterId is comparable and the other is of the same class
- if (filterId != null && other.filterId != null) {
- int comp = filterId.compareTo(other.filterId);
- if (comp != 0) {
- return comp;
- }
- }
-
- // consider equal ranking
- return 0;
+ return (this.filterId < other.filterId) ? -1 : ((this.filterId == other.filterId) ? 0 : 1);
}
@Override
@@ -134,6 +126,7 @@
return filter.hashCode();
}
+ @Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
diff --git a/src/main/java/org/apache/sling/engine/impl/filter/ServletFilterManager.java b/src/main/java/org/apache/sling/engine/impl/filter/ServletFilterManager.java
index 82b847c..53ef9f2 100644
--- a/src/main/java/org/apache/sling/engine/impl/filter/ServletFilterManager.java
+++ b/src/main/java/org/apache/sling/engine/impl/filter/ServletFilterManager.java
@@ -19,9 +19,9 @@
package org.apache.sling.engine.impl.filter;
import java.util.Dictionary;
-import java.util.HashMap;
import java.util.Hashtable;
import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
import javax.servlet.Filter;
import javax.servlet.FilterConfig;
@@ -34,6 +34,8 @@
import org.apache.sling.engine.jmx.FilterProcessorMBean;
import org.osgi.framework.BundleContext;
import org.osgi.framework.Constants;
+import org.osgi.framework.FrameworkUtil;
+import org.osgi.framework.InvalidSyntaxException;
import org.osgi.framework.ServiceReference;
import org.osgi.framework.ServiceRegistration;
import org.osgi.util.tracker.ServiceTracker;
@@ -42,6 +44,8 @@
public class ServletFilterManager extends ServiceTracker<Filter, Filter> {
+ private static final String JMX_OBJECTNAME = "jmx.objectname";
+
public static enum FilterChainType {
/**
* Indicates request level filters.
@@ -97,19 +101,28 @@
private final SlingFilterChainHelper[] filterChains;
- private Map <Long, ServiceRegistration<FilterProcessorMBean>> mbeanMap;
+ private final Map<Long, MBeanReg> mbeanMap = new ConcurrentHashMap<>();
+
+ private static final org.osgi.framework.Filter SERVICE_FILTER;
+ static {
+ org.osgi.framework.Filter f = null;
+ try {
+ f = FrameworkUtil.createFilter("(&(" + Constants.OBJECTCLASS + "=" + Filter.class.getName()
+ + ")(|(" + EngineConstants.SLING_FILTER_SCOPE + "=*)(" + EngineConstants.FILTER_SCOPE + "=*)))");
+ } catch (InvalidSyntaxException e) {
+ // we ignore this here as the above is a constant expression
+ }
+ SERVICE_FILTER = f;
+ }
public ServletFilterManager(final BundleContext context,
final SlingServletContext servletContext) {
- super(context, Filter.class, null);
+ super(context, SERVICE_FILTER, null);
this.servletContext = servletContext;
this.filterChains = new SlingFilterChainHelper[FilterChainType.values().length];
- this.filterChains[FilterChainType.REQUEST.ordinal()] = new SlingFilterChainHelper();
- this.filterChains[FilterChainType.ERROR.ordinal()] = new SlingFilterChainHelper();
- this.filterChains[FilterChainType.INCLUDE.ordinal()] = new SlingFilterChainHelper();
- this.filterChains[FilterChainType.FORWARD.ordinal()] = new SlingFilterChainHelper();
- this.filterChains[FilterChainType.COMPONENT.ordinal()] = new SlingFilterChainHelper();
- this.mbeanMap = new HashMap<Long, ServiceRegistration<FilterProcessorMBean>>();
+ for (final FilterChainType type : FilterChainType.values()) {
+ this.filterChains[type.ordinal()] = new SlingFilterChainHelper();
+ }
}
public SlingFilterChainHelper getFilterChain(final FilterChainType chain) {
@@ -122,9 +135,6 @@
@Override
public Filter addingService(ServiceReference<Filter> reference) {
- if ( this.excludeFilter(reference) ) {
- return null;
- }
Filter service = super.addingService(reference);
if (service != null) {
initFilter(reference, service);
@@ -134,8 +144,13 @@
@Override
public void modifiedService(ServiceReference<Filter> reference, Filter service) {
- destroyFilter(reference, service);
- if ( !this.excludeFilter(reference) ) {
+ // only if the filter name has changed, we need to do a service re-registration
+ final String newFilterName = SlingFilterConfig.getName(reference);
+ if (newFilterName.equals(getUsedFilterName(reference))) {
+ removeFilterFromChains((Long) reference.getProperty(Constants.SERVICE_ID));
+ addFilterToChains(service, reference);
+ } else {
+ destroyFilter(reference, service);
initFilter(reference, service);
}
}
@@ -148,140 +163,68 @@
}
}
- /**
- * Check if the filter should be excluded.
- */
- private boolean excludeFilter(final ServiceReference<Filter> reference) {
- boolean exclude = true;
- // if the service has a filter scope property, we include it
- if ( reference.getProperty(EngineConstants.SLING_FILTER_SCOPE) != null
- || reference.getProperty(EngineConstants.FILTER_SCOPE) != null ) {
- exclude = false;
- }
- if ( !exclude ) {
- final String filterName = SlingFilterConfig.getName(reference);
- if (filterName == null) {
- log.error("initFilter: Missing name for filter {}", reference);
- exclude = true;
- }
- }
- return exclude;
- }
-
private void initFilter(final ServiceReference<Filter> reference,
final Filter filter) {
- // we already checked name in excludeFilter()
final String filterName = SlingFilterConfig.getName(reference);
+ final Long serviceId = (Long) reference.getProperty(Constants.SERVICE_ID);
- // initialize the filter first
try {
- // service id
- final Long serviceId = (Long) reference.getProperty(Constants.SERVICE_ID);
- FilterProcessorMBeanImpl mbean;
+ MBeanReg reg;
try {
final Dictionary<String, String> mbeanProps = new Hashtable<String, String>();
- mbeanProps.put("jmx.objectname", "org.apache.sling:type=engine-filter,service="+filterName);
- mbean = new FilterProcessorMBeanImpl();
- ServiceRegistration<FilterProcessorMBean> filterProcessorMBeanRegistration = context.registerService(FilterProcessorMBean.class, mbean, mbeanProps);
- mbeanMap.put(serviceId,filterProcessorMBeanRegistration);
+ mbeanProps.put(JMX_OBJECTNAME, "org.apache.sling:type=engine-filter,service=" + filterName);
+ reg = new MBeanReg();
+ reg.mbean = new FilterProcessorMBeanImpl();
+
+ reg.registration = reference.getBundle().getBundleContext().registerService(FilterProcessorMBean.class,
+ reg.mbean, mbeanProps);
+
+ mbeanMap.put(serviceId, reg);
} catch (Throwable t) {
log.debug("Unable to register mbean", t);
- mbean = null;
+ reg = null;
}
- final FilterConfig config = new SlingFilterConfig(
- servletContext, reference, filterName);
+ // initialize the filter first
+ final FilterConfig config = new SlingFilterConfig(servletContext, reference, filterName);
filter.init(config);
- // get the order, Integer.MAX_VALUE by default
- final String orderSource;
- Object orderObj = reference.getProperty(Constants.SERVICE_RANKING);
- if (orderObj == null) {
- // filter order is defined as lower value has higher
- // priority while service ranking is the opposite In
- // addition we allow different types than Integer
- orderObj = reference.getProperty(EngineConstants.FILTER_ORDER);
- if (orderObj != null) {
- log.warn("Filter service {} is using deprecated property {}. Use {} instead.",
- new Object[] {reference, EngineConstants.FILTER_ORDER, Constants.SERVICE_RANKING});
- // we can use 0 as the default as this will be applied
- // in the next step anyway if this props contains an
- // invalid value
- orderSource = EngineConstants.FILTER_ORDER + "=" + orderObj;
- orderObj = Integer.valueOf(-1 * OsgiUtil.toInteger(orderObj, 0));
- } else {
- orderSource = "none";
- }
- } else {
- orderSource = Constants.SERVICE_RANKING + "=" + orderObj;
- }
- final int order = (orderObj instanceof Integer) ? ((Integer) orderObj).intValue() : 0;
-
- // register by scope
- String[] scopes = OsgiUtil.toStringArray(
- reference.getProperty(EngineConstants.SLING_FILTER_SCOPE), null);
-
- FilterPredicate predicate = new FilterPredicate(reference);
-
- if ( scopes == null ) {
- scopes = OsgiUtil.toStringArray(
- reference.getProperty(EngineConstants.FILTER_SCOPE), null);
- log.warn("Filter service {} is using deprecated property {}. Use {} instead.",
- new Object[] {reference, EngineConstants.FILTER_SCOPE, EngineConstants.SLING_FILTER_SCOPE});
- }
- if (scopes != null && scopes.length > 0) {
- for (String scope : scopes) {
- scope = scope.toUpperCase();
- try {
- FilterChainType type = FilterChainType.valueOf(scope.toString());
- getFilterChain(type).addFilter(filter, predicate, serviceId,
- order, orderSource, mbean);
-
- if (type == FilterChainType.COMPONENT) {
- getFilterChain(FilterChainType.INCLUDE).addFilter(
- filter, predicate, serviceId, order, orderSource, mbean);
- getFilterChain(FilterChainType.FORWARD).addFilter(
- filter, predicate, serviceId, order, orderSource, mbean);
- }
-
- } catch (IllegalArgumentException iae) {
- // TODO: log ...
- }
- }
- } else {
- log.warn(String.format(
- "A Filter (Service ID %s) has been registered without a filter.scope property.",
- reference.getProperty(Constants.SERVICE_ID)));
- getFilterChain(FilterChainType.REQUEST).addFilter(filter, predicate,
- serviceId, order, orderSource,mbean);
- }
-
+ // add to chains
+ addFilterToChains(filter, reference);
} catch (ServletException ce) {
log.error("Filter " + filterName + " failed to initialize", ce);
} catch (Throwable t) {
- log.error("Unexpected Problem initializing ComponentFilter "
- + "", t);
+ log.error("Unexpected problem initializing filter " + filterName, t);
}
}
+ private String getUsedFilterName(final ServiceReference<Filter> reference) {
+ final MBeanReg reg = mbeanMap.get(reference.getProperty(Constants.SERVICE_ID));
+ if (reg != null) {
+ final String objectName = (String) reg.registration.getReference().getProperty(JMX_OBJECTNAME);
+ if (objectName != null) {
+ final int pos = objectName.indexOf(",service=");
+ if (pos != -1) {
+ return objectName.substring(pos + 9);
+ }
+ }
+ }
+ return null;
+ }
+
private void destroyFilter(final ServiceReference<Filter> reference,
final Filter filter) {
// service id
- Object serviceId = reference.getProperty(Constants.SERVICE_ID);
+ Long serviceId = (Long) reference.getProperty(Constants.SERVICE_ID);
- ServiceRegistration<FilterProcessorMBean> mbean = mbeanMap.remove(serviceId);
- if (mbean != null) {
- mbean.unregister();
- }
-
- boolean removed = false;
- for (SlingFilterChainHelper filterChain : filterChains) {
- removed |= filterChain.removeFilterById(serviceId);
+ final MBeanReg reg = mbeanMap.remove(serviceId);
+ if (reg != null) {
+ reg.registration.unregister();
}
// destroy it
- if (removed) {
+ if (removeFilterFromChains(serviceId)) {
try {
filter.destroy();
} catch (Throwable t) {
@@ -289,4 +232,83 @@
}
}
}
+
+ @SuppressWarnings("deprecation")
+ private void addFilterToChains(final Filter filter, final ServiceReference<Filter> reference) {
+ final Long serviceId = (Long) reference.getProperty(Constants.SERVICE_ID);
+ final MBeanReg mbeanReg = mbeanMap.get(serviceId);
+ final FilterProcessorMBeanImpl mbean = mbeanReg == null ? null : mbeanReg.mbean;
+
+ // get the order, Integer.MAX_VALUE by default
+ final String orderSource;
+ Object orderObj = reference.getProperty(Constants.SERVICE_RANKING);
+ if (orderObj == null) {
+ // filter order is defined as lower value has higher
+ // priority while service ranking is the opposite In
+ // addition we allow different types than Integer
+ orderObj = reference.getProperty(EngineConstants.FILTER_ORDER);
+ if (orderObj != null) {
+ log.warn("Filter service {} is using deprecated property {}. Use {} instead.",
+ new Object[] { reference, EngineConstants.FILTER_ORDER, Constants.SERVICE_RANKING });
+ // we can use 0 as the default as this will be applied
+ // in the next step anyway if this props contains an
+ // invalid value
+ orderSource = EngineConstants.FILTER_ORDER + "=" + orderObj;
+ orderObj = Integer.valueOf(-1 * OsgiUtil.toInteger(orderObj, 0));
+ } else {
+ orderSource = "none";
+ }
+ } else {
+ orderSource = Constants.SERVICE_RANKING + "=" + orderObj;
+ }
+ final int order = (orderObj instanceof Integer) ? ((Integer) orderObj).intValue() : 0;
+
+ // register by scope
+ String[] scopes = OsgiUtil.toStringArray(reference.getProperty(EngineConstants.SLING_FILTER_SCOPE), null);
+
+ FilterPredicate predicate = new FilterPredicate(reference);
+
+ if (scopes == null) {
+ scopes = OsgiUtil.toStringArray(reference.getProperty(EngineConstants.FILTER_SCOPE), null);
+ log.warn("Filter service {} is using deprecated property {}. Use {} instead.",
+ new Object[] { reference, EngineConstants.FILTER_SCOPE, EngineConstants.SLING_FILTER_SCOPE });
+ }
+ if (scopes != null && scopes.length > 0) {
+ for (String scope : scopes) {
+ scope = scope.toUpperCase();
+ try {
+ FilterChainType type = FilterChainType.valueOf(scope.toString());
+ getFilterChain(type).addFilter(filter, predicate, serviceId, order, orderSource, mbean);
+
+ if (type == FilterChainType.COMPONENT) {
+ getFilterChain(FilterChainType.INCLUDE).addFilter(filter, predicate, serviceId, order,
+ orderSource, mbean);
+ getFilterChain(FilterChainType.FORWARD).addFilter(filter, predicate, serviceId, order,
+ orderSource, mbean);
+ }
+
+ } catch (IllegalArgumentException iae) {
+ // TODO: log ...
+ }
+ }
+ } else {
+ log.warn(String.format("A Filter (Service ID %s) has been registered without a %s property.", serviceId,
+ EngineConstants.SLING_FILTER_SCOPE));
+ getFilterChain(FilterChainType.REQUEST).addFilter(filter, predicate, serviceId, order, orderSource, mbean);
+ }
+
+ }
+
+ private boolean removeFilterFromChains(final Long serviceId) {
+ boolean removed = false;
+ for (SlingFilterChainHelper filterChain : filterChains) {
+ removed |= filterChain.removeFilterById(serviceId);
+ }
+ return removed;
+ }
+
+ private static final class MBeanReg {
+ FilterProcessorMBeanImpl mbean;
+ ServiceRegistration<FilterProcessorMBean> registration;
+ }
}
diff --git a/src/main/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelper.java b/src/main/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelper.java
index 01bc9ed..9402b38 100644
--- a/src/main/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelper.java
+++ b/src/main/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelper.java
@@ -43,7 +43,7 @@
}
public synchronized Filter addFilter(final Filter filter, FilterPredicate pattern,
- final Long filterId, final int order, final String orderSource, FilterProcessorMBeanImpl mbean) {
+ final long filterId, final int order, final String orderSource, FilterProcessorMBeanImpl mbean) {
if (filterList == null) {
filterList = new TreeSet<FilterHandle>();
}
@@ -52,13 +52,11 @@
return filter;
}
- public synchronized boolean removeFilterById(final Object filterId) {
+ public synchronized boolean removeFilterById(final long filterId) {
if (filterList != null) {
for (Iterator<FilterHandle> fi = filterList.iterator(); fi.hasNext();) {
FilterHandle test = fi.next();
- if (test.getFilterId() == filterId
- || (test.getFilterId() != null && test.getFilterId().equals(
- filterId))) {
+ if (test.getFilterId() == filterId) {
fi.remove();
filters = getFiltersInternal();
return true;
diff --git a/src/main/java/org/apache/sling/engine/impl/helper/SlingFilterConfig.java b/src/main/java/org/apache/sling/engine/impl/helper/SlingFilterConfig.java
index 2dccbed..66e41e7 100644
--- a/src/main/java/org/apache/sling/engine/impl/helper/SlingFilterConfig.java
+++ b/src/main/java/org/apache/sling/engine/impl/helper/SlingFilterConfig.java
@@ -27,6 +27,7 @@
import java.util.Enumeration;
import java.util.List;
+import javax.servlet.Filter;
import javax.servlet.FilterConfig;
import javax.servlet.ServletContext;
@@ -42,7 +43,7 @@
private ServletContext servletContext;
/** The <code>ServiceReference</code> providing the properties */
- private ServiceReference reference;
+ private ServiceReference<Filter> reference;
/** The name of this configuration object */
private String name;
@@ -58,7 +59,7 @@
* @param filterName The name of this configuration.
*/
public SlingFilterConfig(final ServletContext servletContext,
- final ServiceReference reference,
+ final ServiceReference<Filter> reference,
final String filterName) {
this.servletContext = servletContext;
this.reference = reference;
@@ -68,6 +69,7 @@
/**
* @see javax.servlet.FilterConfig#getInitParameter(java.lang.String)
*/
+ @Override
public String getInitParameter(String name) {
Object prop = reference.getProperty(name);
return (prop == null) ? null : String.valueOf(prop);
@@ -76,6 +78,7 @@
/**
* @see javax.servlet.FilterConfig#getInitParameterNames()
*/
+ @Override
public Enumeration<String> getInitParameterNames() {
List<String> keys = Arrays.asList(reference.getPropertyKeys());
return Collections.enumeration(keys);
@@ -84,16 +87,18 @@
/**
* @see javax.servlet.FilterConfig#getServletContext()
*/
+ @Override
public ServletContext getServletContext() {
return servletContext;
}
/**
- * Looks for a name value in the service reference properties. See the
- * class comment at the top for the list of properties checked by this
- * method.
+ * Looks for a name value in the service reference properties. See the class
+ * comment at the top for the list of properties checked by this method. As the
+ * service id is part of the checked property list this method always returns a
+ * name.
*/
- public static String getName(ServiceReference reference) {
+ public static String getName(ServiceReference<Filter> reference) {
String servletName = null;
for (int i = 0; i < NAME_PROPERTIES.length
&& (servletName == null || servletName.length() == 0); i++) {
@@ -108,6 +113,7 @@
/**
* @see javax.servlet.FilterConfig#getFilterName()
*/
+ @Override
public String getFilterName() {
return name;
}
