FELIX-4060 : Correct ordering of info objects and per context handler registries. Added some test cases. Patch provided by Thomas Baier
git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1661790 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/http/base/src/main/java/org/apache/felix/http/base/internal/handler/PerContextHandlerRegistry.java b/http/base/src/main/java/org/apache/felix/http/base/internal/handler/PerContextHandlerRegistry.java
index c0ade02..f45479c 100644
--- a/http/base/src/main/java/org/apache/felix/http/base/internal/handler/PerContextHandlerRegistry.java
+++ b/http/base/src/main/java/org/apache/felix/http/base/internal/handler/PerContextHandlerRegistry.java
@@ -34,7 +34,6 @@
import org.apache.felix.http.base.internal.runtime.HandlerRuntime.ErrorPage;
import org.apache.felix.http.base.internal.runtime.ServletContextHelperInfo;
import org.apache.felix.http.base.internal.runtime.ServletInfo;
-import org.apache.felix.http.base.internal.util.InternalIdFactory;
public final class PerContextHandlerRegistry implements Comparable<PerContextHandlerRegistry>
{
@@ -91,31 +90,16 @@
@Override
public int compareTo(final PerContextHandlerRegistry other)
{
- final int result = ((Integer)other.path.length()).compareTo(this.path.length());
+ final int result = Integer.compare(other.path.length(), this.path.length());
if ( result == 0 ) {
- if (other.ranking == this.ranking)
+ if (this.ranking == other.ranking)
{
- if (other.serviceId == this.serviceId)
- {
- return 0;
- }
- // service id might be negative, we have to change the behavior in that case
- if ( this.serviceId < 0 )
- {
- if ( other.serviceId > 0 )
- {
- return -1;
- }
- return other.serviceId < this.serviceId ? -1 : 1;
- }
- if ( other.serviceId < 0 )
- {
- return -1;
- }
- return other.serviceId > this.serviceId ? -1 : 1;
+ // Service id's can be negative. Negative id's follow the reverse natural ordering of integers.
+ int reverseOrder = ( this.serviceId >= 0 && other.serviceId >= 0 ) ? 1 : -1;
+ return reverseOrder * Long.compare(this.serviceId, other.serviceId);
}
- return (other.ranking > this.ranking) ? 1 : -1;
+ return Integer.compare(other.ranking, this.ranking);
}
return result;
}
diff --git a/http/base/src/main/java/org/apache/felix/http/base/internal/runtime/AbstractInfo.java b/http/base/src/main/java/org/apache/felix/http/base/internal/runtime/AbstractInfo.java
index d7e43db..d454ce2 100644
--- a/http/base/src/main/java/org/apache/felix/http/base/internal/runtime/AbstractInfo.java
+++ b/http/base/src/main/java/org/apache/felix/http/base/internal/runtime/AbstractInfo.java
@@ -76,34 +76,19 @@
}
/**
- * Compare two info objects based on their ranking (aka ServiceReference ordering)
+ * Compare two info objects based on their ranking (aka reverse ServiceReference ordering)
*/
@Override
public int compareTo(final AbstractInfo<T> other)
{
- if (other.ranking == this.ranking)
+ if (this.ranking == other.ranking)
{
- if (other.serviceId == this.serviceId)
- {
- return 0;
- }
- // service id might be negative, we have to change the behavior in that case
- if ( this.serviceId < 0 )
- {
- if ( other.serviceId > 0 )
- {
- return -1;
- }
- return other.serviceId < this.serviceId ? -1 : 1;
- }
- if ( other.serviceId < 0 )
- {
- return -1;
- }
- return other.serviceId > this.serviceId ? -1 : 1;
+ // Service id's can be negative. Negative id's follow the reverse natural ordering of integers.
+ int reverseOrder = ( this.serviceId >= 0 && other.serviceId >= 0 ) ? 1 : -1;
+ return reverseOrder * Long.compare(this.serviceId, other.serviceId);
}
- return (other.ranking > this.ranking) ? 1 : -1;
+ return Integer.compare(other.ranking, this.ranking);
}
protected boolean isEmpty(final String value)
diff --git a/http/base/src/test/java/org/apache/felix/http/base/internal/handler/PerContextHandlerRegistryTest.java b/http/base/src/test/java/org/apache/felix/http/base/internal/handler/PerContextHandlerRegistryTest.java
index 2cad3ba..9d89769 100644
--- a/http/base/src/test/java/org/apache/felix/http/base/internal/handler/PerContextHandlerRegistryTest.java
+++ b/http/base/src/test/java/org/apache/felix/http/base/internal/handler/PerContextHandlerRegistryTest.java
@@ -17,19 +17,14 @@
package org.apache.felix.http.base.internal.handler;
import static org.junit.Assert.assertEquals;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import org.apache.felix.http.base.internal.runtime.ServletContextHelperInfo;
+import org.apache.felix.http.base.internal.runtime.WhiteboardServiceHelper;
import org.junit.Test;
-import org.osgi.framework.Constants;
-import org.osgi.framework.ServiceReference;
-import org.osgi.service.http.context.ServletContextHelper;
-import org.osgi.service.http.whiteboard.HttpWhiteboardConstants;
/**
* Test for the ordering of servlet contexts
@@ -69,15 +64,51 @@
assertEquals(3L, list.get(3).getContextServiceId());
}
+ @Test public void testOrderingSymetry()
+ {
+ testSymetry("/", "/foo", 1L, 2L, 0, 0);
+ testSymetry("/", "/", 1L, 2L, 0, 10);
+ testSymetry("/", "/", 1L, 2L, 0, 0);
+ testSymetry("/", "/", 1L, -2L, 0, 0);
+ testSymetry("/", "/", -1L, -2L, 0, 0);
+ testSymetry("/", "/", 0L, -1L, 0, 0);
+ testSymetry("/", "/", 0L, 1L, 0, 0);
+ }
+
+ private void testSymetry(String path, String otherPath, long id, long otherId, int ranking, int otherRanking)
+ {
+ PerContextHandlerRegistry handlerRegistry = new PerContextHandlerRegistry(createServletContextHelperInfo(path, id, ranking));
+ PerContextHandlerRegistry other = new PerContextHandlerRegistry(createServletContextHelperInfo(otherPath, otherId, otherRanking));
+
+ assertEquals(handlerRegistry.compareTo(other), -other.compareTo(handlerRegistry));
+ }
+
+ @Test public void testOrderingTransitivity()
+ {
+ testTransitivity("/", "/foo", "/barrr", 1L, 2L, 3L, 0, 0, 0);
+ testTransitivity("/", "/", "/", 0L, 1L, 2L, 1, 2, 3);
+ testTransitivity("/", "/", "/", 2L, 1L, 0L, 0, 0, 0);
+ testTransitivity("/", "/", "/", -1L, 1L, 0L, 0, 0, 0);
+ testTransitivity("/", "/", "/", -2L, -1L, 0L, 0, 0, 0);
+ }
+
+ private void testTransitivity(String highPath, String midPath, String lowPath,
+ long highId, long midId, long lowId,
+ int highRanking, int midRanking, int lowRanking)
+ {
+ PerContextHandlerRegistry high = new PerContextHandlerRegistry(createServletContextHelperInfo(highPath, highId, highRanking));
+ PerContextHandlerRegistry mid = new PerContextHandlerRegistry(createServletContextHelperInfo(midPath, midId, midRanking));
+ PerContextHandlerRegistry low = new PerContextHandlerRegistry(createServletContextHelperInfo(lowPath, lowId, lowRanking));
+
+ assertEquals(1, high.compareTo(mid));
+ assertEquals(1, mid.compareTo(low));
+ assertEquals(1, high.compareTo(low));
+ }
+
private ServletContextHelperInfo createServletContextHelperInfo(final String path,
final long serviceId,
final int ranking)
{
- final ServiceReference<ServletContextHelper> ref = mock(ServiceReference.class);
- when(ref.getProperty(Constants.SERVICE_ID)).thenReturn(serviceId);
- when(ref.getProperty(Constants.SERVICE_RANKING)).thenReturn(ranking);
- when(ref.getProperty(HttpWhiteboardConstants.HTTP_WHITEBOARD_CONTEXT_PATH)).thenReturn(path);
- when(ref.getPropertyKeys()).thenReturn(new String[0]);
- return new ServletContextHelperInfo(ref);
+ return WhiteboardServiceHelper.createContextInfo(ranking, serviceId, "", path, null);
}
}
diff --git a/http/base/src/test/java/org/apache/felix/http/base/internal/runtime/AbstractInfoOrderingTest.java b/http/base/src/test/java/org/apache/felix/http/base/internal/runtime/AbstractInfoOrderingTest.java
new file mode 100644
index 0000000..1b91578
--- /dev/null
+++ b/http/base/src/test/java/org/apache/felix/http/base/internal/runtime/AbstractInfoOrderingTest.java
@@ -0,0 +1,149 @@
+/*
+ * 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.felix.http.base.internal.runtime;
+
+import static java.lang.Integer.signum;
+import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.fail;
+import static org.junit.Assert.assertTrue;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class AbstractInfoOrderingTest
+{
+ @Parameters
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ // Expected value must be non-negative
+ // negative values are tested by symmetry
+
+ // same service id (note: rank must be identical)
+ { 0, 0, 0, 1, 1 },
+ { 0, 1, 1, 0, 0 },
+ { 0, -1, -1, -1, -1 },
+ // rank has priority
+ { 1, 0, 1, 1, 0 },
+ { 1, 0, 1, -1, 0 },
+ { 1, -1, 0, 1, 0 },
+ { 1, -1, 0, -1, 0 },
+ // same rank
+ { 1, 1, 1, 2, 1 },
+ { 1, -1, -1, -1, 1 },
+ { 1, 0, 0, 2, 1 },
+ { 1, 0, 0, 1, 0 },
+ { 1, 0, 0, -1, 1 },
+ { 1, 0, 0, -1, 1 },
+ { 1, 0, 0, -2, 1 },
+ { 1, 0, 0, -1, 2 },
+ { 1, 0, 0, -1, 1 },
+ { 1, 0, 0, -2, -1 }
+ });
+ }
+
+ private final int expected;
+ private final TestInfo testInfo;
+ private final TestInfo other;
+
+ public AbstractInfoOrderingTest(int expected,
+ int testRank, int otherRank, long testId, long otherId)
+ {
+ if (expected < 0)
+ {
+ throw new IllegalArgumentException("Expected values must be non-negative.");
+ }
+ this.expected = expected;
+ testInfo = new TestInfo(testRank, testId);
+ other = new TestInfo(otherRank, otherId);
+ }
+
+ @Test
+ public void ordering()
+ {
+ assertEquals(expected, signum(testInfo.compareTo(other)));
+ if ( expected != 0 )
+ {
+ final List<AbstractInfo> list = new ArrayList<AbstractInfo>();
+ list.add(testInfo);
+ list.add(other);
+ Collections.sort(list);
+ if ( expected == -1 )
+ {
+ assertEquals(testInfo, list.get(0));
+ assertEquals(other, list.get(1));
+ }
+ else
+ {
+ assertEquals(testInfo, list.get(1));
+ assertEquals(other, list.get(0));
+ }
+ }
+ }
+
+ @Test
+ public void orderingSymetry()
+ {
+ assertTrue(signum(testInfo.compareTo(other)) == -signum(other.compareTo(testInfo)));
+ }
+
+ @Test
+ public void orderingTransitivity()
+ {
+ assertTrue(testInfo.compareTo(other) >= 0);
+
+ TestInfo three = new TestInfo(0, 0);
+
+ // three falls in between the two other points
+ if (testInfo.compareTo(three) >= 0 && three.compareTo(other) >= 0)
+ {
+ assertTrue(testInfo.compareTo(other) >= 0);
+ }
+ // three falls below the two other points
+ else if (testInfo.compareTo(other) >= 0 && other.compareTo(three) >= 0)
+ {
+ assertTrue(testInfo.compareTo(three) >= 0);
+ }
+ // three falls above the two other points
+ else if (three.compareTo(testInfo) >= 0 && testInfo.compareTo(other) >= 0)
+ {
+ assertTrue(three.compareTo(other) >= 0);
+ }
+ else
+ {
+ fail("Since testInfo >= other, one of the above cases must match");
+ }
+ }
+
+ private static class TestInfo extends AbstractInfo<TestInfo>
+ {
+ public TestInfo(int ranking, long serviceId)
+ {
+ super(ranking, serviceId);
+ }
+ }
+}