This is an automated email from the ASF dual-hosted git repository.
jsedding pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
The following commit(s) were added to refs/heads/master by this push:
new 75724dd SLING-13251 - MBean registration in ServletFilterManager
fails for factory components (#80)
75724dd is described below
commit 75724dd4a5b00be1462cda6e4f2d062652d2639a
Author: Julian Sedding <[email protected]>
AuthorDate: Wed Jun 24 16:36:52 2026 +0200
SLING-13251 - MBean registration in ServletFilterManager fails for factory
components (#80)
- prefer service.pid over component.name, as the former
is unique (if present), whereas the latter is not unique
- quote filter name before including it in the JMX ObjectName
- add test coverage for JMX MBean registration
- extract method for DRYer code (and higher coverage)
---
pom.xml | 10 +++
.../engine/impl/filter/ServletFilterManager.java | 53 +++++++-------
.../engine/impl/helper/SlingFilterConfig.java | 16 ++--
.../impl/filter/ServletFilterManagerTest.java | 83 ++++++++++++++++++++-
.../engine/impl/helper/SlingFilterConfigTest.java | 66 +++++++++++++++++
.../engine/impl/testutil/MockServiceReference.java | 85 ++++++++++++++++++++++
6 files changed, 277 insertions(+), 36 deletions(-)
diff --git a/pom.xml b/pom.xml
index ae78edb..7880f30 100644
--- a/pom.xml
+++ b/pom.xml
@@ -174,6 +174,16 @@
<version>2.0.17</version>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-params</artifactId>
+ <scope>test</scope>
+ </dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
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 d3c97be..70b6226 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
@@ -18,6 +18,8 @@
*/
package org.apache.sling.engine.impl.filter;
+import javax.management.ObjectName;
+
import java.util.Dictionary;
import java.util.Hashtable;
import java.util.Map;
@@ -47,7 +49,7 @@ import org.slf4j.LoggerFactory;
@Component(service = ServletFilterManager.class)
public class ServletFilterManager {
- private static final String JMX_OBJECTNAME = "jmx.objectname";
+ static final String JMX_OBJECTNAME = "jmx.objectname";
public static enum FilterChainType {
/**
@@ -137,14 +139,7 @@ public class ServletFilterManager {
public void updatedFilter(final ServiceReference<Filter> reference, final
Filter service) {
// 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, null, reference);
- } else {
- destroyFilter(reference, service);
- initFilter(reference, service, null);
- }
+ reregisterOrUpdateMBean(reference,
reference.getProperty(Constants.SERVICE_ID), service, null);
}
public void unbindFilter(final ServiceReference<Filter> reference, final
Filter service) {
@@ -171,15 +166,7 @@ public class ServletFilterManager {
@SuppressWarnings({"rawtypes", "unchecked"})
final ServiceReference<Filter> ref = (ServiceReference<Filter>)
(ServiceReference) reference;
final Filter s = JavaxToJakartaFilterWrapper.toJakartaFilter(service);
- // only if the filter name has changed, we need to do a service
re-registration
- final String newFilterName = SlingFilterConfig.getName(ref);
- if (newFilterName.equals(getUsedFilterName(ref))) {
- removeFilterFromChains((Long)
reference.getProperty(Constants.SERVICE_ID));
- addFilterToChains(s, service, ref);
- } else {
- destroyFilter(ref, s);
- initFilter(ref, s, service);
- }
+ reregisterOrUpdateMBean(ref,
reference.getProperty(Constants.SERVICE_ID), s, service);
}
public void unbindJavaxFilter(
@@ -202,7 +189,7 @@ public class ServletFilterManager {
MBeanReg reg;
try {
final Dictionary<String, String> mbeanProps = new
Hashtable<>();
- mbeanProps.put(JMX_OBJECTNAME,
"org.apache.sling:type=engine-filter,service=" + filterName);
+ mbeanProps.put(JMX_OBJECTNAME,
createJmxObjectName(filterName));
reg = new MBeanReg();
reg.mbean = new FilterProcessorMBeanImpl();
@@ -230,16 +217,28 @@ public class ServletFilterManager {
}
}
- private String getUsedFilterName(final ServiceReference<Filter> reference)
{
+ private void reregisterOrUpdateMBean(
+ ServiceReference<Filter> ref, Object reference, Filter s,
javax.servlet.Filter service) {
+ // only if the filter name has changed, we need to do a service
re-registration
+ final String newFilterName = SlingFilterConfig.getName(ref);
+ String newJmxObjectName = createJmxObjectName(newFilterName);
+ if (newJmxObjectName.equals(getUsedJmxObjectName(ref))) {
+ removeFilterFromChains((Long) reference);
+ addFilterToChains(s, service, ref);
+ } else {
+ destroyFilter(ref, s);
+ initFilter(ref, s, service);
+ }
+ }
+
+ private static String createJmxObjectName(String filterName) {
+ return "org.apache.sling:type=engine-filter,service=" +
ObjectName.quote(filterName);
+ }
+
+ private String getUsedJmxObjectName(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 (String)
reg.registration.getReference().getProperty(JMX_OBJECTNAME);
}
return null;
}
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 692ec9f..b6da723 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
@@ -22,8 +22,8 @@ import java.util.Arrays;
import java.util.Collections;
import java.util.Enumeration;
import java.util.List;
+import java.util.Optional;
-import jakarta.servlet.Filter;
import jakarta.servlet.FilterConfig;
import jakarta.servlet.ServletContext;
import org.osgi.framework.ServiceReference;
@@ -35,13 +35,13 @@ import static
org.osgi.service.component.ComponentConstants.COMPONENT_NAME;
public class SlingFilterConfig implements FilterConfig {
/** The list of property names checked by {@link
#getName(ServiceReference)} */
- private static final String[] NAME_PROPERTIES = {"sling.core.servletName",
COMPONENT_NAME, SERVICE_PID, SERVICE_ID};
+ private static final String[] NAME_PROPERTIES = {"sling.core.servletName",
SERVICE_PID, COMPONENT_NAME, SERVICE_ID};
/** The <code>ServletContext</code> of this configuration object */
private ServletContext servletContext;
/** The <code>ServiceReference</code> providing the properties */
- private ServiceReference<Filter> reference;
+ private ServiceReference<?> reference;
/** The name of this configuration object */
private String name;
@@ -56,7 +56,7 @@ public class SlingFilterConfig implements FilterConfig {
* @param filterName The name of this configuration.
*/
public SlingFilterConfig(
- final ServletContext servletContext, final
ServiceReference<Filter> reference, final String filterName) {
+ final ServletContext servletContext, final ServiceReference<?>
reference, final String filterName) {
this.servletContext = servletContext;
this.reference = reference;
this.name = filterName;
@@ -97,15 +97,17 @@ public class SlingFilterConfig implements FilterConfig {
* @param reference the filter service
* @return the name
*/
- public static String getName(ServiceReference<Filter> reference) {
+ public static String getName(ServiceReference<?> reference) {
String servletName = null;
- for (int i = 0; i < NAME_PROPERTIES.length && (servletName == null ||
servletName.length() == 0); i++) {
+ for (int i = 0; i < NAME_PROPERTIES.length && (servletName == null ||
servletName.isEmpty()); i++) {
Object prop = reference.getProperty(NAME_PROPERTIES[i]);
if (prop != null) {
servletName = String.valueOf(prop);
}
}
- return servletName;
+ return Optional.ofNullable(servletName)
+ .orElseThrow(() ->
+ new NullPointerException("ServiceReferences without a
service.id property should not exist"));
}
/**
diff --git
a/src/test/java/org/apache/sling/engine/impl/filter/ServletFilterManagerTest.java
b/src/test/java/org/apache/sling/engine/impl/filter/ServletFilterManagerTest.java
index 2508e80..a735846 100644
---
a/src/test/java/org/apache/sling/engine/impl/filter/ServletFilterManagerTest.java
+++
b/src/test/java/org/apache/sling/engine/impl/filter/ServletFilterManagerTest.java
@@ -20,6 +20,7 @@ package org.apache.sling.engine.impl.filter;
import java.io.IOException;
import java.util.Hashtable;
+import java.util.Map;
import jakarta.servlet.Filter;
import jakarta.servlet.FilterChain;
@@ -34,15 +35,26 @@ import org.apache.sling.engine.EngineConstants;
import org.apache.sling.engine.impl.ProductInfoProvider;
import
org.apache.sling.engine.impl.filter.ServletFilterManager.FilterChainType;
import org.apache.sling.engine.impl.helper.SlingServletContext;
+import org.apache.sling.engine.jmx.FilterProcessorMBean;
import org.apache.sling.testing.mock.osgi.junit.OsgiContext;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.mockito.Mockito;
import org.osgi.framework.BundleContext;
+import org.osgi.framework.ServiceReference;
+import org.osgi.framework.ServiceRegistration;
+import static
org.apache.sling.engine.impl.testutil.MockServiceReference.serviceReference;
+import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
+import static org.osgi.framework.Constants.SERVICE_ID;
+import static org.osgi.framework.Constants.SERVICE_PID;
public class ServletFilterManagerTest {
@@ -123,7 +135,75 @@ public class ServletFilterManagerTest {
assertFilterInScopes(servletFilterManager, testFilter,
FilterChainType.ERROR);
}
+ @Test
+ public void majorFilterRegistrationUpdateCausesJmxMBeanReregistration()
throws Exception {
+ TestFilter filter = new TestFilter();
+ BundleContext bundleContext = osgiContext.bundleContext();
+
+ // initially no MBean is registered
+
assertNull(bundleContext.getServiceReference(FilterProcessorMBean.class));
+
+ // MBean is registered
+ servletFilterManager.bindFilter(
+ serviceReference(bundleContext, Map.of(SERVICE_PID, "name",
SERVICE_ID, 42L)), filter);
+ ServiceReference<FilterProcessorMBean> originalMBeanRef =
+ bundleContext.getServiceReference(FilterProcessorMBean.class);
+ assertNotNull(originalMBeanRef);
+ assertEquals("org.apache.sling:type=engine-filter,service=\"name\"",
getJmxBeanObjectName(bundleContext));
+
+ // MBean is reregistered
+ servletFilterManager.updatedFilter(
+ serviceReference(bundleContext, Map.of(SERVICE_PID,
"updated-name", SERVICE_ID, 42L)), filter);
+ assertNotSame(originalMBeanRef,
bundleContext.getServiceReference(FilterProcessorMBean.class));
+ assertEquals(
+
"org.apache.sling:type=engine-filter,service=\"updated-name\"",
getJmxBeanObjectName(bundleContext));
+
+ // MBean is unregistered
+ servletFilterManager.unbindFilter(serviceReference(bundleContext,
Map.of(SERVICE_ID, 42L)), filter);
+
+
assertNull(bundleContext.getServiceReference(FilterProcessorMBean.class));
+ }
+
+ @Test
+ public void minorFilterRegistrationUpdateCausesNoJmxMBeanReregistration()
throws Exception {
+ TestFilter filter = new TestFilter();
+ BundleContext bundleContext = osgiContext.bundleContext();
+
+ // initially no MBean is registered
+
assertNull(bundleContext.getServiceReference(FilterProcessorMBean.class));
+
+ // MBean is registered
+ servletFilterManager.bindFilter(
+ serviceReference(bundleContext, Map.of(SERVICE_PID, "name",
SERVICE_ID, 42L)), filter);
+ ServiceReference<FilterProcessorMBean> originalMBeanRef =
+ bundleContext.getServiceReference(FilterProcessorMBean.class);
+ assertNotNull(originalMBeanRef);
+ assertEquals("org.apache.sling:type=engine-filter,service=\"name\"",
getJmxBeanObjectName(bundleContext));
+
+ // MBean is not reregistered
+ servletFilterManager.updatedFilter(
+ serviceReference(bundleContext, Map.of(SERVICE_PID, "name",
SERVICE_ID, 42L, "dummy", "change")),
+ filter);
+ assertSame(originalMBeanRef,
bundleContext.getServiceReference(FilterProcessorMBean.class));
+
+ // MBean is unregistered
+ servletFilterManager.unbindFilter(
+ serviceReference(bundleContext, Map.of(SERVICE_PID,
"updated-name", SERVICE_ID, 42L)), filter);
+
assertNull(bundleContext.getServiceReference(FilterProcessorMBean.class));
+ }
+
+ private static String getJmxBeanObjectName(BundleContext bundleContext) {
+ ServiceReference<FilterProcessorMBean> mbeanRef =
bundleContext.getServiceReference(FilterProcessorMBean.class);
+ return (String)
mbeanRef.getProperty(ServletFilterManager.JMX_OBJECTNAME);
+ }
+
private static TestFilter registerFilterForScopes(BundleContext
bundleContext, FilterChainType... scopes) {
+ ServiceRegistration<Filter> filterServiceRegistration =
registrationOfFilterForScopes(bundleContext, scopes);
+ return (TestFilter)
bundleContext.getService(filterServiceRegistration.getReference());
+ }
+
+ private static ServiceRegistration<Filter> registrationOfFilterForScopes(
+ BundleContext bundleContext, FilterChainType... scopes) {
Hashtable<String, Object> properties = new Hashtable<>();
if (scopes != null) {
String[] scopeNames = new String[scopes.length];
@@ -134,8 +214,7 @@ public class ServletFilterManagerTest {
}
TestFilter testFilter = new TestFilter();
- bundleContext.registerService(Filter.class, testFilter, properties);
- return testFilter;
+ return bundleContext.registerService(Filter.class, testFilter,
properties);
}
private static TestFilter registerFilterForValues(BundleContext
bundleContext, String... scopes) {
diff --git
a/src/test/java/org/apache/sling/engine/impl/helper/SlingFilterConfigTest.java
b/src/test/java/org/apache/sling/engine/impl/helper/SlingFilterConfigTest.java
new file mode 100644
index 0000000..f658216
--- /dev/null
+++
b/src/test/java/org/apache/sling/engine/impl/helper/SlingFilterConfigTest.java
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.engine.impl.helper;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Stream;
+
+import jakarta.servlet.Filter;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.osgi.framework.ServiceReference;
+
+import static
org.apache.sling.engine.impl.testutil.MockServiceReference.serviceReference;
+import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.params.provider.Arguments.argumentSet;
+import static org.osgi.framework.Constants.SERVICE_ID;
+import static org.osgi.framework.Constants.SERVICE_PID;
+import static org.osgi.service.component.ComponentConstants.COMPONENT_NAME;
+
+public class SlingFilterConfigTest {
+
+ static Stream<Arguments> scenariosForGetName() {
+ List<String> propertyNames = List.of("sling.core.servletName",
SERVICE_PID, COMPONENT_NAME, SERVICE_ID);
+ return propertyNames.stream().map(propertyName -> {
+ int idx = propertyNames.indexOf(propertyName);
+ Map<String, Object> properties = new HashMap<>();
+ String expectedValue = "ServletName from " + propertyName;
+ if (Objects.equals(propertyName, SERVICE_ID)) {
+ properties.put(propertyName, 5);
+ expectedValue = "5";
+ } else {
+ properties.put(propertyName, expectedValue);
+ }
+
+ List<String> ignoredProperties = propertyNames.subList(idx + 1,
propertyNames.size());
+ ignoredProperties.forEach(p -> properties.put(p, "ServletName from
" + p));
+ return argumentSet("MBean name from " + propertyName,
expectedValue, serviceReference(properties));
+ });
+ }
+
+ @ParameterizedTest
+ @MethodSource("scenariosForGetName")
+ void testGetName(String expectedName, ServiceReference<Filter> reference) {
+ assertEquals(expectedName, SlingFilterConfig.getName(reference));
+ }
+}
diff --git
a/src/test/java/org/apache/sling/engine/impl/testutil/MockServiceReference.java
b/src/test/java/org/apache/sling/engine/impl/testutil/MockServiceReference.java
new file mode 100644
index 0000000..e7e8525
--- /dev/null
+++
b/src/test/java/org/apache/sling/engine/impl/testutil/MockServiceReference.java
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.engine.impl.testutil;
+
+import java.util.Dictionary;
+import java.util.Hashtable;
+import java.util.Map;
+import java.util.Objects;
+
+import jakarta.servlet.Filter;
+import org.osgi.framework.Bundle;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.ServiceReference;
+
+public class MockServiceReference implements ServiceReference<Filter> {
+
+ private final BundleContext bundleContext;
+ private final Map<String, Object> properties;
+
+ public MockServiceReference(BundleContext bundleContext, Map<String,
Object> properties) {
+ this.bundleContext = bundleContext;
+ this.properties = properties;
+ }
+
+ public static ServiceReference<Filter> serviceReference(Map<String,
Object> properties) {
+ return new MockServiceReference(null, properties);
+ }
+
+ public static ServiceReference<Filter> serviceReference(
+ BundleContext bundleContext, Map<String, Object> properties) {
+ return new MockServiceReference(bundleContext, properties);
+ }
+
+ @Override
+ public Object getProperty(String key) {
+ return properties.get(key);
+ }
+
+ @Override
+ public String[] getPropertyKeys() {
+ return properties.keySet().toArray(new String[0]);
+ }
+
+ @Override
+ public Dictionary<String, Object> getProperties() {
+ return new Hashtable<>(properties);
+ }
+
+ @Override
+ public Bundle getBundle() {
+ return Objects.requireNonNull(bundleContext, "MockServiceReference was
constructed with a null BundleContext")
+ .getBundle();
+ }
+
+ @Override
+ public Bundle[] getUsingBundles() {
+ throw new UnsupportedOperationException("not implemented");
+ }
+
+ @Override
+ public boolean isAssignableTo(Bundle bundle, String className) {
+ throw new UnsupportedOperationException("not implemented");
+ }
+
+ @Override
+ public int compareTo(Object reference) {
+ throw new UnsupportedOperationException("not implemented");
+ }
+}
