SLING-7134 - Script execution order is not deterministic on Java 9
* the SlingServletResolver now uses the ScriptEngineManager to help in
picking the most appropriate script to render a resource, based on the available
scripts extensions and the order of the ScriptEngineFactories from the list returned by
ScriptEngineManager#getEngineFactories
* the AbstractResourceCollector will use the ordered list of the ScriptEngineFactories extensions
for sorting the candidate WeightedResources
* added tests for the new resource collection mechanism
diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java b/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java
index 4e1e6f4..060bfe6 100644
--- a/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java
+++ b/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java
@@ -45,6 +45,8 @@
import javax.management.NotCompliantMBeanException;
import javax.management.StandardMBean;
+import javax.script.ScriptEngineFactory;
+import javax.script.ScriptEngineManager;
import javax.servlet.Servlet;
import javax.servlet.ServletContext;
import javax.servlet.ServletException;
@@ -188,6 +190,11 @@
@Reference(target="("+ServiceUserMapped.SUBSERVICENAME+"=console)")
private ServiceUserMapped consoleServiceUserMapped;
+ @Reference
+ private ScriptEngineManager scriptEngineManager;
+
+ private List<String> scriptEnginesExtensions = new ArrayList<>();
+
private ResourceResolver sharedScriptResolver;
private final Map<ServiceReference<Servlet>, ServletReg> servletsByReference = new HashMap<>();
@@ -654,7 +661,7 @@
return scriptServlet;
}
- final Collection<Resource> candidates = locationUtil.getServlets(resolver);
+ final Collection<Resource> candidates = locationUtil.getServlets(resolver, scriptEnginesExtensions);
if (LOGGER.isDebugEnabled()) {
if (candidates.isEmpty()) {
@@ -880,6 +887,15 @@
LOGGER.debug("Unable to register mbean");
}
}
+ updateScriptEngineExtensions();
+ }
+
+ private void updateScriptEngineExtensions() {
+ List<String> scriptEnginesExtensions = new ArrayList<>();
+ for (ScriptEngineFactory factory : scriptEngineManager.getEngineFactories()) {
+ scriptEnginesExtensions.addAll(factory.getExtensions());
+ }
+ this.scriptEnginesExtensions = Collections.unmodifiableList(scriptEnginesExtensions);
}
/**
@@ -1099,6 +1115,7 @@
@Override
public void handleEvent(final Event event) {
flushCache();
+ updateScriptEngineExtensions();
}
private void flushCache() {
@@ -1291,7 +1308,7 @@
defaultExtensions,
method,
requestPathInfo.getSelectors());
- servlets = locationUtil.getServlets(resourceResolver);
+ servlets = locationUtil.getServlets(resourceResolver, scriptEnginesExtensions);
}
tr(pw);
tdLabel(pw, "Candidates");
diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/helper/AbstractResourceCollector.java b/src/main/java/org/apache/sling/servlets/resolver/internal/helper/AbstractResourceCollector.java
index 224c0bd..7a15896 100644
--- a/src/main/java/org/apache/sling/servlets/resolver/internal/helper/AbstractResourceCollector.java
+++ b/src/main/java/org/apache/sling/servlets/resolver/internal/helper/AbstractResourceCollector.java
@@ -18,14 +18,20 @@
*/
package org.apache.sling.servlets.resolver.internal.helper;
+import java.util.ArrayList;
import java.util.Collection;
+import java.util.Comparator;
+import java.util.HashSet;
import java.util.Iterator;
+import java.util.List;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
+import org.apache.commons.lang3.StringUtils;
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ResourceResolver;
+import org.apache.sling.api.resource.ResourceUtil;
import org.apache.sling.api.resource.SyntheticResource;
import org.apache.sling.servlets.resolver.internal.SlingServletResolver;
@@ -64,9 +70,32 @@
this.executionPaths = executionPaths;
}
- public final Collection<Resource> getServlets(final ResourceResolver resolver) {
+ public final Collection<Resource> getServlets(final ResourceResolver resolver, final List<String> scriptExtensions) {
- final SortedSet<Resource> resources = new TreeSet<Resource>();
+ final SortedSet<WeightedResource> resources = new TreeSet<>(new Comparator<WeightedResource>() {
+ @Override
+ public int compare(WeightedResource o1, WeightedResource o2) {
+ if (o1.equals(o2)) {
+ return 0;
+ }
+ int comparisonResult = o1.compareTo(o2);
+ if (comparisonResult == 0) {
+ String o1Extension = getScriptExtension(o1.getName());
+ String o2Extension = getScriptExtension(o2.getName());
+ if (StringUtils.isNotEmpty(o1Extension) && StringUtils.isNotEmpty(o2Extension)) {
+ int o1ExtensionIndex = scriptExtensions.indexOf(o1Extension);
+ int o2ExtensionIndex = scriptExtensions.indexOf(o2Extension);
+ if (o1ExtensionIndex > o2ExtensionIndex) {
+ return -1;
+ } else if (o1ExtensionIndex == o2ExtensionIndex) {
+ return 0;
+ }
+ return 1;
+ }
+ }
+ return comparisonResult;
+ }
+ });
final Iterator<String> locations = new LocationIterator(resourceType, resourceSuperType,
baseResourceType, resolver);
while (locations.hasNext()) {
@@ -85,10 +114,12 @@
getWeightedResources(resources, locationRes);
}
- return resources;
+ List<Resource> result = new ArrayList<>(resources.size());
+ result.addAll(resources);
+ return result;
}
- abstract protected void getWeightedResources(final Set<Resource> resources,
+ abstract protected void getWeightedResources(final Set<WeightedResource> resources,
final Resource location);
/**
@@ -104,8 +135,9 @@
* the name of the resource.
* @param methodPrefixWeight The method/prefix weight assigned to the
* resource according to the resource name.
+ * @param scriptExtensionPriority The priority of the script engine used to run this script
*/
- protected final void addWeightedResource(final Set<Resource> resources,
+ protected final void addWeightedResource(final Set<WeightedResource> resources,
final Resource resource,
final int numSelectors,
final int methodPrefixWeight) {
@@ -225,4 +257,12 @@
return false;
}
+ private String getScriptExtension(String scriptName) {
+ int lastIndexOf = scriptName.lastIndexOf('.');
+ if (lastIndexOf > -1 && lastIndexOf < scriptName.length() - 1) {
+ return scriptName.substring(lastIndexOf + 1);
+ }
+ return null;
+ }
+
}
diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/helper/NamedScriptResourceCollector.java b/src/main/java/org/apache/sling/servlets/resolver/internal/helper/NamedScriptResourceCollector.java
index bcc163c..10b2c64 100644
--- a/src/main/java/org/apache/sling/servlets/resolver/internal/helper/NamedScriptResourceCollector.java
+++ b/src/main/java/org/apache/sling/servlets/resolver/internal/helper/NamedScriptResourceCollector.java
@@ -84,7 +84,7 @@
}
@Override
- protected void getWeightedResources(final Set<Resource> resources,
+ protected void getWeightedResources(final Set<WeightedResource> resources,
final Resource location) {
final ResourceResolver resolver = location.getResourceResolver();
// if extension is set, we first check for an exact script match
diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/helper/ResourceCollector.java b/src/main/java/org/apache/sling/servlets/resolver/internal/helper/ResourceCollector.java
index 7a8e6a1..20eacc2 100644
--- a/src/main/java/org/apache/sling/servlets/resolver/internal/helper/ResourceCollector.java
+++ b/src/main/java/org/apache/sling/servlets/resolver/internal/helper/ResourceCollector.java
@@ -183,7 +183,7 @@
}
@Override
- protected void getWeightedResources(final Set<Resource> resources,
+ protected void getWeightedResources(final Set<WeightedResource> resources,
final Resource location) {
final ResourceResolver resolver = location.getResourceResolver();
@@ -283,7 +283,7 @@
private boolean checkScriptName(final String scriptName,
final String selector, final String parentName,
final String suffix, final String htmlSuffix,
- final Set<Resource> resources, final Resource child,
+ final Set<WeightedResource> resources, final Resource child,
final int selIdx) {
if (selector != null && matches(scriptName, selector, suffix)) {
addWeightedResource(resources, child, selIdx + 1,
@@ -333,7 +333,7 @@
&& lenScriptName == (lenName + lenSuffix);
}
- private void addLocationServlet(final Set<Resource> resources,
+ private void addLocationServlet(final Set<WeightedResource> resources,
final Resource location) {
final String path = location.getPath()
+ ServletResourceProviderFactory.SERVLET_PATH_EXTENSION;
diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/helper/WeightedResource.java b/src/main/java/org/apache/sling/servlets/resolver/internal/helper/WeightedResource.java
index 470593f..ad6244e 100644
--- a/src/main/java/org/apache/sling/servlets/resolver/internal/helper/WeightedResource.java
+++ b/src/main/java/org/apache/sling/servlets/resolver/internal/helper/WeightedResource.java
@@ -131,7 +131,6 @@
return 1;
}
- // extensions are equal, compare ordinal (lower ordinal wins)
- return (ordinal < o.ordinal) ? -1 : 1;
+ return 0;
}
}
diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/SlingServletResolverTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/SlingServletResolverTest.java
index 25ba33c..2bf82d4 100644
--- a/src/test/java/org/apache/sling/servlets/resolver/internal/SlingServletResolverTest.java
+++ b/src/test/java/org/apache/sling/servlets/resolver/internal/SlingServletResolverTest.java
@@ -30,6 +30,7 @@
import java.util.List;
import java.util.Map;
+import javax.script.ScriptEngineManager;
import javax.servlet.Servlet;
import javax.servlet.http.HttpServlet;
@@ -134,6 +135,11 @@
resolverField.setAccessible(true);
resolverField.set(servletResolver, factory);
+ // set script engine manager
+ final Field scriptEngineManagerField = resolverClass.getDeclaredField("scriptEngineManager");
+ scriptEngineManagerField.setAccessible(true);
+ scriptEngineManagerField.set(servletResolver, Mockito.mock(ScriptEngineManager.class));
+
final Bundle bundle = Mockito.mock(Bundle.class);
Mockito.when(bundle.getBundleId()).thenReturn(1L);
diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/helper/ResourceCollectorTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/helper/ResourceCollectorTest.java
index f53500e..d0cf55d 100644
--- a/src/test/java/org/apache/sling/servlets/resolver/internal/helper/ResourceCollectorTest.java
+++ b/src/test/java/org/apache/sling/servlets/resolver/internal/helper/ResourceCollectorTest.java
@@ -18,9 +18,12 @@
*/
package org.apache.sling.servlets.resolver.internal.helper;
+import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
+import java.util.List;
import java.util.Map;
import org.apache.sling.api.resource.Resource;
@@ -139,6 +142,36 @@
effectiveTest(names, baseIdxs, indices);
}
+ public void testScriptExtensionsPriority() {
+ String[] names = {".servlet", // 0
+ "/" + label + ".esp", // 1
+ "/GET.esp", // 2
+ "/" + label + ".html.esp", // 3
+ "/html.esp", // 4
+ ".esp", // 5
+ "/image.esp", // 6
+ "/print/other.esp", // 7
+ "/print.other.esp", // 8
+ "/print.html.esp", // 9
+ "/print/a4.html.esp", // 10
+ "/print/a4.html.js", // 11
+ "/print/a4.html.html", // 12
+ "/print/a4.html.jsp", // 13
+ "/print", // resource to enable walking the tree
+ "/print", // resource to enable walking the tree
+ };
+
+ int[] baseIdxs = { 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 0, 1 , 0 , 1, 0};
+ int[] indices = {12, 13, 11, 10, 9, 3, 4, 1, 2, 0};
+
+ effectiveTest(names, baseIdxs, indices, new ArrayList<String>(){{
+ add("esp");
+ add("js");
+ add("jsp");
+ add("html");
+ }});
+ }
+
public void testAnyServlets0() {
// use a request with another request method "ANY"
request.setMethod("ANY");
@@ -256,6 +289,10 @@
}
protected void effectiveTest(String[] names, int[] baseIdxs, int[] indices) {
+ effectiveTest(names, baseIdxs, indices, null);
+ }
+
+ protected void effectiveTest(String[] names, int[] baseIdxs, int[] indices, List<String> scriptEngineExtensions) {
String[] base = { "/apps/" + resourceTypePath,
"/libs/" + resourceTypePath };
@@ -271,7 +308,12 @@
}
ResourceCollector lu = ResourceCollector.create(request, null, new String[] {"html"});
- Collection<Resource> res = lu.getServlets(request.getResourceResolver());
+ Collection<Resource> res;
+ if (scriptEngineExtensions != null) {
+ res = lu.getServlets(request.getResourceResolver(), scriptEngineExtensions);
+ } else {
+ res = lu.getServlets(request.getResourceResolver(), Collections.EMPTY_LIST);
+ }
Iterator<Resource> rIter = res.iterator();
for (int index : indices) {
diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/helper/ScriptSelectionTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/helper/ScriptSelectionTest.java
index 6e662c7..d084127 100644
--- a/src/test/java/org/apache/sling/servlets/resolver/internal/helper/ScriptSelectionTest.java
+++ b/src/test/java/org/apache/sling/servlets/resolver/internal/helper/ScriptSelectionTest.java
@@ -19,6 +19,7 @@
package org.apache.sling.servlets.resolver.internal.helper;
import java.util.Collection;
+import java.util.Collections;
import org.apache.sling.api.resource.Resource;
import org.apache.sling.commons.testing.sling.MockResource;
@@ -69,7 +70,7 @@
// Create mock request and get scripts from ResourceCollector
final MockSlingHttpServletRequest req = makeRequest(method, selectors, extension);
final ResourceCollector u = ResourceCollector.create(req, null, new String[] {"html"});
- final Collection<Resource> s = u.getServlets(req.getResourceResolver());
+ final Collection<Resource> s = u.getServlets(req.getResourceResolver(), Collections.EMPTY_LIST);
if(expectedScript == null) {
assertFalse("No script must be found", s.iterator().hasNext());
diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/helper/WeightedResourceTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/helper/WeightedResourceTest.java
index 78278e5..85ae4e7 100644
--- a/src/test/java/org/apache/sling/servlets/resolver/internal/helper/WeightedResourceTest.java
+++ b/src/test/java/org/apache/sling/servlets/resolver/internal/helper/WeightedResourceTest.java
@@ -72,15 +72,4 @@
assertTrue(lr2.compareTo(lr1) > 0);
}
- public void testCompareToOrdinal() {
- WeightedResource lr1 = new WeightedResource(0, null, 0, WeightedResource.WEIGHT_NONE);
- WeightedResource lr2 = new WeightedResource(1, null, 0, WeightedResource.WEIGHT_NONE);
-
- // expect the same objects to compare equal
- assertEquals(0, lr1.compareTo(lr1));
- assertEquals(0, lr2.compareTo(lr2));
-
- assertTrue(lr1.compareTo(lr2) < 0);
- assertTrue(lr2.compareTo(lr1) > 0);
- }
}
