SLING-5664 consider service ranking when injecting OSGi services

git-svn-id: https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/models/impl@1739788 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/pom.xml b/pom.xml
index 342620d..7465660 100644 (file)
--- a/pom.xml
+++ b/pom.xml
@@ -99,17 +99,17 @@
             <scope>provided</scope>\r
         </dependency>\r
         <dependency>\r
-            <groupId>junit</groupId>\r
-            <artifactId>junit</artifactId>\r
-            <scope>test</scope>\r
-        </dependency>\r
-        <dependency>\r
             <groupId>javax.inject</groupId>\r
             <artifactId>javax.inject</artifactId>\r
             <version>1</version>\r
             <scope>provided</scope>\r
         </dependency>\r
         <dependency>\r
+            <groupId>junit</groupId>\r
+            <artifactId>junit</artifactId>\r
+            <scope>test</scope>\r
+        </dependency>\r
+        <dependency>\r
             <groupId>org.apache.sling</groupId>\r
             <artifactId>org.apache.sling.testing.osgi-mock</artifactId>\r
             <version>1.5.0</version>\r
@@ -122,6 +122,12 @@
             <scope>test</scope>\r
         </dependency>\r
         <dependency>\r
+            <groupId>org.hamcrest</groupId>\r
+            <artifactId>hamcrest-junit</artifactId>\r
+            <version>2.0.0.0</version>\r
+            <scope>test</scope>\r
+        </dependency>\r
+        <dependency>\r
             <groupId>org.slf4j</groupId>\r
             <artifactId>slf4j-simple</artifactId>\r
             <scope>test</scope>\r

diff --git a/src/main/java/org/apache/sling/models/impl/injectors/OSGiServiceInjector.java b/src/main/java/org/apache/sling/models/impl/injectors/OSGiServiceInjector.java
index 4ae6e0b..af6c340 100644 (file)
--- a/src/main/java/org/apache/sling/models/impl/injectors/OSGiServiceInjector.java
+++ b/src/main/java/org/apache/sling/models/impl/injectors/OSGiServiceInjector.java
@@ -23,18 +23,16 @@ import java.lang.reflect.Type;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 
 import javax.annotation.Nonnull;
-import javax.servlet.ServletRequest;
 
 import org.apache.commons.lang.StringUtils;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Property;
 import org.apache.felix.scr.annotations.Service;
-import org.apache.sling.api.scripting.SlingBindings;
-import org.apache.sling.api.scripting.SlingScriptHelper;
 import org.apache.sling.models.annotations.Filter;
 import org.apache.sling.models.annotations.injectorspecific.InjectionStrategy;
 import org.apache.sling.models.annotations.injectorspecific.OSGiService;
@@ -91,71 +89,49 @@ public class OSGiServiceInjector implements Injector, StaticInjectAnnotationProc
 
     private <T> Object getService(Object adaptable, Class<T> type, String filter,
             DisposalCallbackRegistry callbackRegistry) {
-        SlingScriptHelper helper = getScriptHelper(adaptable);
-
-        if (helper != null) {
-            T[] services = helper.getServices(type, filter);
-            if (services == null || services.length == 0) {
+        // cannot use SlingScriptHelper since it does not support ordering by service ranking due to https://issues.apache.org/jira/browse/SLING-5665
+        try {
+            ServiceReference[] refs = bundleContext.getServiceReferences(type.getName(), filter);
+            if (refs == null || refs.length == 0) {
                 return null;
             } else {
-                return services[0];
-            }
-        } else {
-            try {
-                ServiceReference[] refs = bundleContext.getServiceReferences(type.getName(), filter);
-                if (refs == null || refs.length == 0) {
-                    return null;
-                } else {
-                    callbackRegistry.addDisposalCallback(new Callback(refs, bundleContext));
-                    return bundleContext.getService(refs[0]);
-                }
-            } catch (InvalidSyntaxException e) {
-                log.error("invalid filter expression", e);
-                return null;
+                // sort by service ranking (lowest first) (see ServiceReference.compareTo)
+                List<ServiceReference> references = Arrays.asList(refs);
+                Collections.sort(references);
+                callbackRegistry.addDisposalCallback(new Callback(refs, bundleContext));
+                return bundleContext.getService(references.get(references.size() - 1));
             }
+        } catch (InvalidSyntaxException e) {
+            log.error("invalid filter expression", e);
+            return null;
         }
     }
 
     private <T> Object[] getServices(Object adaptable, Class<T> type, String filter,
             DisposalCallbackRegistry callbackRegistry) {
-        SlingScriptHelper helper = getScriptHelper(adaptable);
-
-        if (helper != null) {
-            T[] services = helper.getServices(type, filter);
-            return services;
-        } else {
-            try {
-                ServiceReference[] refs = bundleContext.getServiceReferences(type.getName(), filter);
-                if (refs == null || refs.length == 0) {
-                    return null;
-                } else {
-                    callbackRegistry.addDisposalCallback(new Callback(refs, bundleContext));
-                    List<Object> services = new ArrayList<Object>();
-                    for (ServiceReference ref : refs) {
-                        Object service = bundleContext.getService(ref);
-                        if (service != null) {
-                            services.add(service);
-                        }
-                    }
-                    return services.toArray();
-                }
-            } catch (InvalidSyntaxException e) {
-                log.error("invalid filter expression", e);
+        // cannot use SlingScriptHelper since it does not support ordering by service ranking due to https://issues.apache.org/jira/browse/SLING-5665
+        try {
+            ServiceReference[] refs = bundleContext.getServiceReferences(type.getName(), filter);
+            if (refs == null || refs.length == 0) {
                 return null;
-            }
-        }
-    }
-
-    private SlingScriptHelper getScriptHelper(Object adaptable) {
-        if (adaptable instanceof ServletRequest) {
-            ServletRequest request = (ServletRequest) adaptable;
-            SlingBindings bindings = (SlingBindings) request.getAttribute(SlingBindings.class.getName());
-            if (bindings != null) {
-                return bindings.getSling();
             } else {
-                return null;
+                // sort by service ranking (lowest first) (see ServiceReference.compareTo)
+                List<ServiceReference> references = Arrays.asList(refs);
+                Collections.sort(references);
+                // make highest service ranking being returned first
+                Collections.reverse(references);
+                callbackRegistry.addDisposalCallback(new Callback(refs, bundleContext));
+                List<Object> services = new ArrayList<Object>();
+                for (ServiceReference ref : references) {
+                    Object service = bundleContext.getService(ref);
+                    if (service != null) {
+                        services.add(service);
+                    }
+                }
+                return services.toArray();
             }
-        } else {
+        } catch (InvalidSyntaxException e) {
+            log.error("invalid filter expression", e);
             return null;
         }
     }

diff --git a/src/test/java/org/apache/sling/models/impl/OSGiInjectionTest.java b/src/test/java/org/apache/sling/models/impl/OSGiInjectionTest.java
index c0c8da4..21b3d09 100644 (file)
--- a/src/test/java/org/apache/sling/models/impl/OSGiInjectionTest.java
+++ b/src/test/java/org/apache/sling/models/impl/OSGiInjectionTest.java
@@ -16,10 +16,18 @@
  */
 package org.apache.sling.models.impl;
 
-import static org.junit.Assert.*;
-import static org.mockito.Matchers.*;
-import static org.mockito.Mockito.*;
-
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
 import java.util.Dictionary;
 import java.util.Hashtable;
 
@@ -37,6 +45,7 @@ import org.apache.sling.models.testmodels.classes.RequestOSGiModel;
 import org.apache.sling.models.testmodels.classes.SetOSGiModel;
 import org.apache.sling.models.testmodels.classes.SimpleOSGiModel;
 import org.apache.sling.models.testmodels.interfaces.ServiceInterface;
+import org.hamcrest.Matchers;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -97,27 +106,6 @@ public class OSGiInjectionTest {
     }
 
     @Test
-    public void testRequestOSGiModelField() throws Exception {
-        ServiceInterface service = mock(ServiceInterface.class);
-
-        SlingHttpServletRequest request = mock(SlingHttpServletRequest.class);
-        when(request.getAttribute(SlingBindings.class.getName())).thenReturn(bindings);
-
-        when(helper.getServices(ServiceInterface.class, null)).thenReturn(new ServiceInterface[] { service });
-
-        RequestOSGiModel model = factory.getAdapter(request, RequestOSGiModel.class);
-        assertNotNull(model);
-        assertNotNull(model.getService());
-        assertEquals(service, model.getService());
-
-        verify(bundleContext).registerService(eq(Runnable.class.getName()), eq(factory), any(Dictionary.class));
-        verify(bundleContext).addBundleListener(any(BundleListener.class));
-        verify(bundleContext).registerService(eq(Object.class.getName()), any(Object.class), any(Dictionary.class));
-        verify(bundleContext).getBundles();
-        verifyNoMoreInteractions(bundleContext);
-    }
-
-    @Test
     public void testListOSGiModelField() throws Exception {
         ServiceReference ref1 = mock(ServiceReference.class);
         ServiceInterface service1 = mock(ServiceInterface.class);
@@ -134,9 +122,9 @@ public class OSGiInjectionTest {
         ListOSGiModel model = factory.getAdapter(res, ListOSGiModel.class);
         assertNotNull(model);
         assertNotNull(model.getServices());
-        assertEquals(2, model.getServices().size());
-        assertEquals(service1, model.getServices().get(0));
-        assertEquals(service2, model.getServices().get(1));
+        // the order on those is non deterministic as the ServiceReference.compareTo() is always returning 0
+        // the real order is tested in the IT
+        assertThat(model.getServices(), Matchers.containsInAnyOrder(service1, service2));
 
         verifyNoMoreInteractions(res);
     }
@@ -158,9 +146,9 @@ public class OSGiInjectionTest {
         ArrayOSGiModel model = factory.getAdapter(res, ArrayOSGiModel.class);
         assertNotNull(model);
         assertNotNull(model.getServices());
-        assertEquals(2, model.getServices().length);
-        assertEquals(service1, model.getServices()[0]);
-        assertEquals(service2, model.getServices()[1]);
+        // the order on those is non deterministic as the ServiceReference.compareTo() is always returning 0
+        // the real order is tested in the IT
+        assertThat(Arrays.asList(model.getServices()), Matchers.containsInAnyOrder(service1, service2));
 
         verifyNoMoreInteractions(res);
     }
@@ -205,10 +193,9 @@ public class OSGiInjectionTest {
         CollectionOSGiModel model = factory.getAdapter(res, CollectionOSGiModel.class);
         assertNotNull(model);
         assertNotNull(model.getServices());
-        assertEquals(2, model.getServices().size());
-
-        assertTrue(model.getServices().contains(service1));
-        assertTrue(model.getServices().contains(service2));
+        // the order on those is non deterministic as the ServiceReference.compareTo() is always returning 0
+        // the real order is tested in the IT
+        assertThat(model.getServices(), Matchers.containsInAnyOrder(service1, service2));
 
         verifyNoMoreInteractions(res);
     }
@@ -274,9 +261,9 @@ public class OSGiInjectionTest {
                 = factory.getAdapter(res, org.apache.sling.models.testmodels.classes.constructorinjection.ListOSGiModel.class);
         assertNotNull(model);
         assertNotNull(model.getServices());
-        assertEquals(2, model.getServices().size());
-        assertEquals(service1, model.getServices().get(0));
-        assertEquals(service2, model.getServices().get(1));
+        // the order on those is non deterministic as the ServiceReference.compareTo() is always returning 0
+        // the real order is tested in the IT
+        assertThat(model.getServices(), Matchers.containsInAnyOrder(service1, service2));
 
         verifyNoMoreInteractions(res);
     }