You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by fm...@apache.org on 2015/04/24 10:58:12 UTC

svn commit: r1675805 - in /sling/trunk/bundles/resourceresolver/src: main/java/org/apache/sling/resourceresolver/impl/tree/ProviderHandler.java test/java/org/apache/sling/resourceresolver/impl/tree/ResourceProviderEntryTest.java

Author: fmeschbe
Date: Fri Apr 24 08:58:11 2015
New Revision: 1675805

URL: http://svn.apache.org/r1675805
Log:
SLING-4656 Fix ProviderHandler.compareTo

* Document natural order of ProviderHandler to be the 
  same as service.ranking order. which is the revers of
  the natural ServiceReference order previously
  implemented
* Added test case by Carsten Ziegeler (thanks)

Modified:
    sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/impl/tree/ProviderHandler.java
    sling/trunk/bundles/resourceresolver/src/test/java/org/apache/sling/resourceresolver/impl/tree/ResourceProviderEntryTest.java

Modified: sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/impl/tree/ProviderHandler.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/impl/tree/ProviderHandler.java?rev=1675805&r1=1675804&r2=1675805&view=diff
==============================================================================
--- sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/impl/tree/ProviderHandler.java (original)
+++ sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/impl/tree/ProviderHandler.java Fri Apr 24 08:58:11 2015
@@ -43,12 +43,22 @@ import org.slf4j.LoggerFactory;
  * The provider handler is the common base class for the
  * {@link ResourceProviderHandler} and the
  * {@link ResourceProviderFactoryHandler}.
+ * <p>
+ * The natural ordering for instances of this class is according to the
+ * OSGi Service ranking order:
+ * <ol>
+ * <li>An instance with a higher service.ranking property is compared less
+ *    than an instance with a lower service.ranking property.</li>
+ * <li>If service.ranking properties are equal, the service with the
+ *    lower service.id is compared less than the service with the higher
+ *    service.id</li>
+ * </ol>
  */
 public abstract class ProviderHandler implements Comparable<ProviderHandler> {
 
     /** Default logger */
     private final Logger logger = LoggerFactory.getLogger(getClass());
-    
+
     /** Service properties. */
     private final Map<String, Object> properties;
 
@@ -314,28 +324,21 @@ public abstract class ProviderHandler im
             return 0; // same service
         }
 
-        Object rankObj = this.getProperties().get(Constants.SERVICE_RANKING);
+        Object thisRankObj = this.getProperties().get(Constants.SERVICE_RANKING);
         Object otherRankObj = other.getProperties().get(Constants.SERVICE_RANKING);
 
-        // If no rank, then spec says it defaults to zero.
-        rankObj = (rankObj == null) ? new Integer(0) : rankObj;
-        otherRankObj = (otherRankObj == null) ? new Integer(0) : otherRankObj;
-
-        // If rank is not Integer, then spec says it defaults to zero.
-        Integer rank = (rankObj instanceof Integer)
-            ? (Integer) rankObj : new Integer(0);
-        Integer otherRank = (otherRankObj instanceof Integer)
-            ? (Integer) otherRankObj : new Integer(0);
+        // If rank is not specified or not an Integer, then spec says it defaults to zero.
+        Integer thisRank = (thisRankObj instanceof Integer) ? (Integer) thisRankObj : Integer.valueOf(0);
+        Integer otherRank = (otherRankObj instanceof Integer) ? (Integer) otherRankObj : Integer.valueOf(0);
 
         // Sort by rank in ascending order.
-        if (rank.compareTo(otherRank) < 0) {
-            return -1; // lower rank
-        } else if (rank.compareTo(otherRank) > 0) {
-            return 1; // higher rank
+        int rankOrder = thisRank.compareTo(otherRank);
+        if (rankOrder != 0) {
+            return (rankOrder > 0) ? -1 : 1;
         }
 
         // If ranks are equal, then sort by service id in descending order.
-        return (this.serviceId.compareTo(other.serviceId) < 0) ? 1 : -1;
+        return (this.serviceId.compareTo(other.serviceId) > 0) ? 1 : -1;
     }
 
     /**

Modified: sling/trunk/bundles/resourceresolver/src/test/java/org/apache/sling/resourceresolver/impl/tree/ResourceProviderEntryTest.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/resourceresolver/src/test/java/org/apache/sling/resourceresolver/impl/tree/ResourceProviderEntryTest.java?rev=1675805&r1=1675804&r2=1675805&view=diff
==============================================================================
--- sling/trunk/bundles/resourceresolver/src/test/java/org/apache/sling/resourceresolver/impl/tree/ResourceProviderEntryTest.java (original)
+++ sling/trunk/bundles/resourceresolver/src/test/java/org/apache/sling/resourceresolver/impl/tree/ResourceProviderEntryTest.java Fri Apr 24 08:58:11 2015
@@ -25,6 +25,7 @@ import static org.junit.Assert.fail;
 
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.Map;
 
 import org.apache.sling.api.resource.AbstractResource;
@@ -43,7 +44,7 @@ import org.osgi.framework.Constants;
 public class ResourceProviderEntryTest {
 
     private static final Map<String, String> EMPTY_PARAMS = Collections.emptyMap();
-    
+
     private ResourceResolver rootResolver;
 
     private ResourceProviderEntry root;
@@ -249,6 +250,36 @@ public class ResourceProviderEntryTest {
         }
     }
 
+    @Test public void testOrderingWithoutRanking() {
+        final Map<String, Object> props1 = new HashMap<String, Object>();
+        props1.put(Constants.SERVICE_ID, 1L);
+        final ProviderHandler ph1 = new MyProviderHandler(props1);
+
+        assertEquals(0, ph1.compareTo(ph1));
+
+        final Map<String, Object> props2 = new HashMap<String, Object>();
+        props2.put(Constants.SERVICE_ID, 2L);
+        final ProviderHandler ph2 = new MyProviderHandler(props2);
+
+        assertEquals(-1, ph1.compareTo(ph2));
+        assertEquals(1, ph2.compareTo(ph1));
+    }
+
+    @Test public void testOrderingWithRanking() {
+        final Map<String, Object> props1 = new HashMap<String, Object>();
+        props1.put(Constants.SERVICE_ID, 1L);
+        props1.put(Constants.SERVICE_RANKING, 50);
+        final ProviderHandler ph1 = new MyProviderHandler(props1);
+
+        final Map<String, Object> props2 = new HashMap<String, Object>();
+        props2.put(Constants.SERVICE_ID, 2L);
+        props2.put(Constants.SERVICE_RANKING, 150);
+        final ProviderHandler ph2 = new MyProviderHandler(props2);
+
+        assertEquals(1, ph1.compareTo(ph2));
+        assertEquals(-1, ph2.compareTo(ph1));
+    }
+
     private void assertEqualsResolver(final ResourceResolver resolver, final Resource res) {
         assertEquals(resolver, res.getResourceResolver());
     }
@@ -297,4 +328,27 @@ public class ResourceProviderEntryTest {
 			return false;
 		}
     }
+
+    private static class MyProviderHandler extends ProviderHandler {
+
+        public MyProviderHandler(Map<String, Object> properties) {
+            super(properties);
+        }
+
+        @Override
+        public Resource getResource(ResourceResolverContext ctx, ResourceResolver resourceResolver, String path,
+                Map<String, String> parameters) {
+            return null;
+        }
+
+        @Override
+        public Iterator<Resource> listChildren(ResourceResolverContext ctx, Resource parent) {
+            return null;
+        }
+
+        @Override
+        public ResourceProvider getResourceProvider(ResourceResolverContext ctx) {
+            return null;
+        }
+    }
 }