SLING-11738 : Add option to prevent request override
diff --git a/.gitignore b/.gitignore
index 5b783ed..1ef01fc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -13,5 +13,6 @@
*.bak
.vlt
.DS_Store
+.vscode
jcr.log
atlassian-ide-plugin.xml
diff --git a/bnd.bnd b/bnd.bnd
deleted file mode 100644
index ddf92e6..0000000
--- a/bnd.bnd
+++ /dev/null
@@ -1 +0,0 @@
-Require-Capability: osgi.implementation;filter:="(&(osgi.implementation=osgi.http)(version>=1.0)(!(version>=2.0)))"
\ No newline at end of file
diff --git a/pom.xml b/pom.xml
index c793a29..19bd764 100644
--- a/pom.xml
+++ b/pom.xml
@@ -22,7 +22,7 @@
<parent>
<groupId>org.apache.sling</groupId>
<artifactId>sling-bundle-parent</artifactId>
- <version>47</version>
+ <version>49</version>
<relativePath />
</parent>
@@ -55,10 +55,19 @@
<dependency>
<groupId>org.osgi</groupId>
<artifactId>osgi.core</artifactId>
+ <version>8.0.0</version>
+ <scope>provided</scope>
</dependency>
<dependency>
<groupId>org.osgi</groupId>
<artifactId>org.osgi.service.component.annotations</artifactId>
+ <version>1.5.0</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.osgi</groupId>
+ <artifactId>org.osgi.service.component</artifactId>
+ <version>1.5.0</version>
<scope>provided</scope>
</dependency>
<dependency>
@@ -74,22 +83,37 @@
<dependency>
<groupId>org.osgi</groupId>
<artifactId>org.osgi.service.http.whiteboard</artifactId>
- <version>1.0.0</version>
+ <version>1.1.0</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.api</artifactId>
- <version>2.1.0</version>
+ <version>2.25.4</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>javax.servlet</groupId>
<artifactId>javax.servlet-api</artifactId>
+ <version>3.1.0</version>
+ <scope>provided</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <version>4.13.2</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <version>4.10.0</version>
+ <scope>test</scope>
</dependency>
</dependencies>
</project>
diff --git a/src/main/java/org/apache/sling/featureflags/Features.java b/src/main/java/org/apache/sling/featureflags/Features.java
index e9b7f7d..93b67ec 100644
--- a/src/main/java/org/apache/sling/featureflags/Features.java
+++ b/src/main/java/org/apache/sling/featureflags/Features.java
@@ -18,6 +18,8 @@
*/
package org.apache.sling.featureflags;
+import java.util.Collection;
+
import org.osgi.annotation.versioning.ProviderType;
/**
@@ -35,11 +37,24 @@
* are configured with OSGi configuration whose factory PID is
* {@code org.apache.sling.featureflags.Feature}.
*
- * @return The known features
+ * @return The known features, the array might be empty
+ * @deprecated Use {@link #getAllFeatures()} instead
*/
Feature[] getFeatures();
/**
+ * Get the list of all (known) features.
+ * <p>
+ * Features are known if they are registered as {@link Feature} services or
+ * are configured with OSGi configuration whose factory PID is
+ * {@code org.apache.sling.featureflags.Feature}.
+ *
+ * @return The known features, the collection might be empty
+ * @since 1.2.0
+ */
+ Collection<Feature> getAllFeatures();
+
+ /**
* Returns the feature with the given name.
* <p>
* Features are known if they are registered as {@link Feature} services or
diff --git a/src/main/java/org/apache/sling/featureflags/impl/ConfiguredFeature.java b/src/main/java/org/apache/sling/featureflags/impl/ConfiguredFeature.java
index 7280f64..7b3565c 100644
--- a/src/main/java/org/apache/sling/featureflags/impl/ConfiguredFeature.java
+++ b/src/main/java/org/apache/sling/featureflags/impl/ConfiguredFeature.java
@@ -28,16 +28,14 @@
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.ConfigurationPolicy;
+import org.osgi.service.component.annotations.Modified;
import org.osgi.service.metatype.annotations.AttributeDefinition;
import org.osgi.service.metatype.annotations.Designate;
import org.osgi.service.metatype.annotations.ObjectClassDefinition;
@Designate(ocd = ConfiguredFeature.Config.class, factory = true)
@Component(service = Feature.class,
- configurationPolicy = ConfigurationPolicy.REQUIRE,
- property = {
- Constants.SERVICE_VENDOR + "=The Apache Software Foundation"
- })
+ configurationPolicy = ConfigurationPolicy.REQUIRE)
public class ConfiguredFeature implements Feature {
@ObjectClassDefinition(name = "Apache Sling Configured Feature",
@@ -59,18 +57,23 @@
@AttributeDefinition(name = "Enabled", description = "Boolean flag indicating whether the feature is "
+ "enabled or not by this configuration")
boolean enabled() default false;
+
+ @AttributeDefinition(name = "Allow Override", description = "If disabled, the enabled state can't be overriden for a request")
+ boolean allowOverride() default true;
}
private static final String PROP_FEATURE = "feature";
+ private volatile String name;
- private String name;
+ private volatile String description;
- private String description;
+ private volatile boolean enabled;
- private boolean enabled;
+ private volatile boolean allowOverride;
@Activate
+ @Modified
private void activate(final Config config, final Map<String, Object> properties) {
this.name = config.name();
if ( this.name == null ) {
@@ -87,24 +90,25 @@
this.description = this.name;
}
this.enabled = config.enabled();
+ this.allowOverride = config.allowOverride();
}
@Override
- public boolean isEnabled(ExecutionContext context) {
-
- // Request Parameter Override
- ServletRequest request = context.getRequest();
- if (request != null) {
- String[] features = request.getParameterValues(PROP_FEATURE);
- if (features != null) {
- for (String feature : features) {
- if (this.name.equals(feature)) {
- return true;
+ public boolean isEnabled(final ExecutionContext context) {
+ if ( this.allowOverride ) {
+ // Request Parameter Override
+ final ServletRequest request = context.getRequest();
+ if (request != null) {
+ final String[] features = request.getParameterValues(PROP_FEATURE);
+ if (features != null) {
+ for (String feature : features) {
+ if (this.name.equals(feature)) {
+ return true;
+ }
}
- }
+ }
}
}
-
return this.enabled;
}
diff --git a/src/main/java/org/apache/sling/featureflags/impl/ExecutionContextImpl.java b/src/main/java/org/apache/sling/featureflags/impl/ExecutionContextImpl.java
index 8a95302..877140d 100644
--- a/src/main/java/org/apache/sling/featureflags/impl/ExecutionContextImpl.java
+++ b/src/main/java/org/apache/sling/featureflags/impl/ExecutionContextImpl.java
@@ -36,30 +36,14 @@
private static final String REQUEST_ATTRIBUTE_RESOLVER = "org.apache.sling.auth.core.ResourceResolver";
- private final ResourceResolver resourceResolver;
-
private final HttpServletRequest request;
- private final Map<String, Boolean> featureCache;
+ private volatile Map<String, Boolean> featureCache;
private final Features features;
public ExecutionContextImpl(final Features features, final HttpServletRequest request) {
- ResourceResolver resourceResolver = null;
- if (request != null) {
- if ( request instanceof SlingHttpServletRequest ) {
- resourceResolver = ((SlingHttpServletRequest)request).getResourceResolver();
- } else {
- Object resolverObject = request.getAttribute(REQUEST_ATTRIBUTE_RESOLVER);
- if (resolverObject instanceof ResourceResolver) {
- resourceResolver = (ResourceResolver) resolverObject;
- }
- }
- }
-
this.request = request;
- this.resourceResolver = resourceResolver;
- this.featureCache = new HashMap<String, Boolean>();
this.features = features;
}
@@ -70,7 +54,18 @@
@Override
public ResourceResolver getResourceResolver() {
- return this.resourceResolver;
+ ResourceResolver resourceResolver = null;
+ if (request != null) {
+ if ( request instanceof SlingHttpServletRequest ) {
+ resourceResolver = ((SlingHttpServletRequest)request).getResourceResolver();
+ } else {
+ final Object resolverObject = request.getAttribute(REQUEST_ATTRIBUTE_RESOLVER);
+ if (resolverObject instanceof ResourceResolver) {
+ resourceResolver = (ResourceResolver) resolverObject;
+ }
+ }
+ }
+ return resourceResolver;
}
@Override
@@ -79,14 +74,17 @@
}
boolean isEnabled(final Feature feature) {
+ if ( this.featureCache == null ) {
+ this.featureCache = new HashMap<>();
+ }
final String name = feature.getName();
- Boolean entry = this.featureCache.get(name);
- if (entry == null) {
+ Boolean result = this.featureCache.get(name);
+ if (result == null) {
// put false in the cache to stop on circular calls
this.featureCache.put(name, Boolean.FALSE);
- entry = feature.isEnabled(this);
- this.featureCache.put(name, entry);
+ result = feature.isEnabled(this);
+ this.featureCache.put(name, result);
}
- return entry;
+ return result;
}
}
diff --git a/src/main/java/org/apache/sling/featureflags/impl/FeatureManager.java b/src/main/java/org/apache/sling/featureflags/impl/FeatureManager.java
index 17464fd..b225341 100644
--- a/src/main/java/org/apache/sling/featureflags/impl/FeatureManager.java
+++ b/src/main/java/org/apache/sling/featureflags/impl/FeatureManager.java
@@ -21,16 +21,18 @@
import java.io.IOException;
import java.io.PrintWriter;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.Collections;
-import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
+import javax.servlet.GenericServlet;
import javax.servlet.Servlet;
-import javax.servlet.ServletConfig;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
@@ -46,9 +48,10 @@
import org.osgi.service.component.annotations.Reference;
import org.osgi.service.component.annotations.ReferenceCardinality;
import org.osgi.service.component.annotations.ReferencePolicy;
+import org.osgi.service.component.propertytypes.ServiceRanking;
import org.osgi.service.http.whiteboard.HttpWhiteboardConstants;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import org.osgi.service.http.whiteboard.propertytypes.HttpWhiteboardContextSelect;
+import org.osgi.service.http.whiteboard.propertytypes.HttpWhiteboardFilterPattern;
/**
* This service implements the feature handling. It keeps track of all
@@ -57,32 +60,35 @@
@Component(service = {Features.class, Filter.class, Servlet.class},
configurationPolicy = ConfigurationPolicy.IGNORE,
property = {
- HttpWhiteboardConstants.HTTP_WHITEBOARD_CONTEXT_SELECT + "=(" + HttpWhiteboardConstants.HTTP_WHITEBOARD_CONTEXT_NAME + "=org.apache.sling)",
- HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_PATTERN + "=/",
"felix.webconsole.label=features",
"felix.webconsole.title=Features",
- "felix.webconsole.category=Sling",
- Constants.SERVICE_RANKING + ":Integer=16384",
- Constants.SERVICE_VENDOR + "=The Apache Software Foundation"
+ "felix.webconsole.category=Sling"
})
-public class FeatureManager implements Features, Filter, Servlet {
+@HttpWhiteboardContextSelect("(" + HttpWhiteboardConstants.HTTP_WHITEBOARD_CONTEXT_NAME + "=org.apache.sling)")
+@HttpWhiteboardFilterPattern("/")
+@ServiceRanking(16384)
+public class FeatureManager
+ extends GenericServlet
+ implements Features, Filter {
- private final Logger logger = LoggerFactory.getLogger(this.getClass());
+ private final ThreadLocal<ExecutionContextImpl> perThreadClientContext = new ThreadLocal<>();
- private final ThreadLocal<ExecutionContextImpl> perThreadClientContext = new ThreadLocal<ExecutionContextImpl>();
+ private final ConcurrentMap<Long, String> idToNameMap = new ConcurrentHashMap<>();
- private final Map<String, List<FeatureDescription>> allFeatures = new HashMap<String, List<FeatureDescription>>();
+ private final ConcurrentMap<String, List<FeatureDescription>> registeredFeatures = new ConcurrentHashMap<>();
- private Map<String, Feature> activeFeatures = Collections.emptyMap();
-
- private ServletConfig servletConfig;
+ private final ConcurrentMap<String, Feature> activeFeatures = new ConcurrentHashMap<>();
//--- Features
@Override
public Feature[] getFeatures() {
- final Map<String, Feature> activeFeatures = this.activeFeatures;
- return activeFeatures.values().toArray(new Feature[activeFeatures.size()]);
+ return this.activeFeatures.values().toArray(new Feature[activeFeatures.size()]);
+ }
+
+ @Override
+ public Collection<Feature> getAllFeatures() {
+ return Collections.unmodifiableCollection(this.activeFeatures.values());
}
@Override
@@ -122,40 +128,23 @@
@Override
public void destroy() {
// method shared by Servlet and Filter interface
- this.servletConfig = null;
}
//--- Servlet
@Override
- public void init(final ServletConfig config) {
- this.servletConfig = config;
- }
-
- @Override
- public ServletConfig getServletConfig() {
- return this.servletConfig;
- }
-
- @Override
- public String getServletInfo() {
- return "Features";
- }
-
- @Override
public void service(ServletRequest req, ServletResponse res) throws IOException {
if ("GET".equals(((HttpServletRequest) req).getMethod())) {
final PrintWriter pw = res.getWriter();
- final Feature[] features = getFeatures();
- if (features == null || features.length == 0) {
+ if (this.activeFeatures.isEmpty()) {
pw.println("<p class='statline ui-state-highlight'>No Features currently defined</p>");
} else {
pw.printf("<p class='statline ui-state-highlight'>%d Feature(s) currently defined</p>%n",
- features.length);
+ this.activeFeatures.size());
pw.println("<table class='nicetable'>");
pw.println("<tr><th>Name</th><th>Description</th><th>Enabled</th></tr>");
final ExecutionContextImpl ctx = getCurrentExecutionContext();
- for (final Feature feature : features) {
+ for (final Feature feature : this.activeFeatures.values()) {
pw.printf("<tr><td>%s</td><td>%s</td><td>%s</td></tr>%n", ResponseUtil.escapeXml(feature.getName()),
ResponseUtil.escapeXml(feature.getDescription()), ctx.isEnabled(feature));
}
@@ -171,56 +160,47 @@
// bind method for Feature services
@Reference(cardinality = ReferenceCardinality.MULTIPLE,
- policy = ReferencePolicy.DYNAMIC)
- private void bindFeature(final Feature f, final Map<String, Object> props) {
- synchronized (this.allFeatures) {
- final String name = f.getName();
+ policy = ReferencePolicy.DYNAMIC,
+ updated = "updatedFeature")
+ void bindFeature(final Feature f, final Map<String, Object> props) {
+ final String name = f.getName();
+ if ( name != null && !name.isEmpty() ) {
final FeatureDescription info = new FeatureDescription(f, props);
-
- List<FeatureDescription> candidates = this.allFeatures.get(name);
- if (candidates == null) {
- candidates = new ArrayList<FeatureDescription>();
- this.allFeatures.put(name, candidates);
+ this.idToNameMap.put(info.serviceId, name);
+ final List<FeatureDescription> candidates = this.registeredFeatures.computeIfAbsent(name, key -> new ArrayList<>());
+ synchronized (candidates) {
+ candidates.add(info);
+ Collections.sort(candidates);
+ this.activeFeatures.put(name, candidates.get(0).feature);
}
- candidates.add(info);
- Collections.sort(candidates);
-
- this.calculateActiveProviders();
}
}
+ // updated method for Feature services
+ void updatedFeature(final Feature f, final Map<String, Object> props) {
+ this.unbindFeature(f, props);
+ this.bindFeature(f, props);
+ }
+
// unbind method for Feature services
- @SuppressWarnings("unused")
- private void unbindFeature(final Feature f, final Map<String, Object> props) {
- synchronized (this.allFeatures) {
- final String name = f.getName();
- final FeatureDescription info = new FeatureDescription(f, props);
-
- final List<FeatureDescription> candidates = this.allFeatures.get(name);
+ void unbindFeature(final Feature f, final Map<String, Object> props) {
+ final FeatureDescription info = new FeatureDescription(f, props);
+ final String name = this.idToNameMap.remove(info.serviceId);
+ if ( name != null ) {
+ final List<FeatureDescription> candidates = this.registeredFeatures.get(name);
if (candidates != null) { // sanity check
- candidates.remove(info);
- if (candidates.size() == 0) {
- this.allFeatures.remove(name);
+ synchronized ( candidates ) {
+ candidates.remove(info);
+ if ( candidates.isEmpty() ) {
+ this.activeFeatures.remove(name);
+ } else {
+ this.activeFeatures.put(name, candidates.get(0).feature);
+ }
}
}
- this.calculateActiveProviders();
}
}
- // calculates map of active features (eliminating Feature name
- // collisions). Must be called while synchronized on this.allFeatures
- private void calculateActiveProviders() {
- final Map<String, Feature> activeMap = new HashMap<String, Feature>();
- for (final Map.Entry<String, List<FeatureDescription>> entry : this.allFeatures.entrySet()) {
- final FeatureDescription desc = entry.getValue().get(0);
- activeMap.put(entry.getKey(), desc.feature);
- if (entry.getValue().size() > 1) {
- logger.warn("More than one feature service for feature {}", entry.getKey());
- }
- }
- this.activeFeatures = activeMap;
- }
-
//--- Client Context management and access
void pushContext(final HttpServletRequest request) {
@@ -242,7 +222,7 @@
*/
private final static class FeatureDescription implements Comparable<FeatureDescription> {
- public final int ranking;
+ private final int ranking;
public final long serviceId;
diff --git a/src/main/java/org/apache/sling/featureflags/package-info.java b/src/main/java/org/apache/sling/featureflags/package-info.java
index 4df4dc6..10dfe08 100644
--- a/src/main/java/org/apache/sling/featureflags/package-info.java
+++ b/src/main/java/org/apache/sling/featureflags/package-info.java
@@ -63,5 +63,5 @@
* @version 1.1
* @see <a href="http://sling.apache.org/documentation/the-sling-engine/featureflags.html">Feature Flags</a>
*/
-@org.osgi.annotation.versioning.Version("1.1.0")
+@org.osgi.annotation.versioning.Version("1.2.0")
package org.apache.sling.featureflags;
diff --git a/src/test/java/org/apache/sling/featureflags/impl/FeatureManagerTest.java b/src/test/java/org/apache/sling/featureflags/impl/FeatureManagerTest.java
new file mode 100644
index 0000000..823cab8
--- /dev/null
+++ b/src/test/java/org/apache/sling/featureflags/impl/FeatureManagerTest.java
@@ -0,0 +1,80 @@
+/*
+ * 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.featureflags.impl;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertSame;
+
+import java.util.Collections;
+
+import org.apache.sling.featureflags.Feature;
+import org.apache.sling.featureflags.Features;
+import org.junit.Test;
+import org.mockito.Mockito;
+import org.osgi.framework.Constants;
+
+public class FeatureManagerTest {
+
+ private Feature createFeature(final String name) {
+ final Feature f = Mockito.mock(Feature.class);
+ Mockito.when(f.getName()).thenReturn(name);
+ return f;
+ }
+
+ @Test
+ public void testFeatures() {
+ final Features features = new FeatureManager();
+ assertEquals(0, features.getAllFeatures().size());
+ }
+
+ @Test
+ public void testDuplicateFeatureName() {
+ final FeatureManager features = new FeatureManager();
+ final Feature f1 = createFeature("a");
+ final Feature f2 = createFeature("b");
+ final Feature f3 = createFeature("b");
+ features.bindFeature(f1, Collections.singletonMap(Constants.SERVICE_ID, 1L));
+ features.bindFeature(f2, Collections.singletonMap(Constants.SERVICE_ID, 2L));
+ features.bindFeature(f3, Collections.singletonMap(Constants.SERVICE_ID, 3L));
+
+ // a and b
+ assertEquals(2, features.getAllFeatures().size());
+ assertNotNull(features.getFeature("a"));
+ assertSame(f1, features.getFeature("a"));
+ assertNotNull(features.getFeature("b"));
+ assertSame(f2, features.getFeature("b"));
+
+ // remove a - b remains
+ features.unbindFeature(f1, Collections.singletonMap(Constants.SERVICE_ID, 1L));
+ assertEquals(1, features.getAllFeatures().size());
+ assertNotNull(features.getFeature("b"));
+ assertSame(f2, features.getFeature("b"));
+
+ // remove higher b - lower b remains
+ features.unbindFeature(f2, Collections.singletonMap(Constants.SERVICE_ID, 2L));
+ assertEquals(1, features.getAllFeatures().size());
+ assertNotNull(features.getFeature("b"));
+ assertSame(f3, features.getFeature("b"));
+
+ // remove lower b - no features
+ features.unbindFeature(f3, Collections.singletonMap(Constants.SERVICE_ID, 3L));
+ assertEquals(0, features.getAllFeatures().size());
+ }
+}
