Author: cziegeler
Date: Thu May 22 12:47:10 2014
New Revision: 1596846
URL: http://svn.apache.org/r1596846
Log:
SLING-2920 : Wrong handling of Sling Filter ordering. Apply patch from Felix
Meschberger, add test case and remove synced method calls for request handling
Added:
sling/trunk/bundles/engine/src/test/java/org/apache/sling/engine/impl/filter/
sling/trunk/bundles/engine/src/test/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelperTest.java
(with props)
Modified:
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/WebConsoleConfigPrinter.java
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/ServletFilterManager.java
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelper.java
Modified:
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/WebConsoleConfigPrinter.java
URL:
http://svn.apache.org/viewvc/sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/WebConsoleConfigPrinter.java?rev=1596846&r1=1596845&r2=1596846&view=diff
==============================================================================
---
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/WebConsoleConfigPrinter.java
(original)
+++
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/WebConsoleConfigPrinter.java
Thu May 22 12:47:10 2014
@@ -87,8 +87,10 @@ public class WebConsoleConfigPrinter {
pw.print(entry.getOrder());
pw.print(" : ");
pw.print(entry.getFilter().getClass());
- pw.print(" (");
- pw.print(entry.getFitlerId());
+ pw.print(" (id: ");
+ pw.print(entry.getFilterId());
+ pw.print(", property: ");
+ pw.print(entry.getOrderSource());
pw.println(")");
}
}
Modified:
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/ServletFilterManager.java
URL:
http://svn.apache.org/viewvc/sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/ServletFilterManager.java?rev=1596846&r1=1596845&r2=1596846&view=diff
==============================================================================
---
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/ServletFilterManager.java
(original)
+++
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/ServletFilterManager.java
Thu May 22 12:47:10 2014
@@ -187,26 +187,29 @@ public class ServletFilterManager extend
filter.init(config);
// service id
- Long serviceId = (Long)
reference.getProperty(Constants.SERVICE_ID);
+ final Long serviceId = (Long)
reference.getProperty(Constants.SERVICE_ID);
// get the order, Integer.MAX_VALUE by default
+ final String orderSource;
Object orderObj =
reference.getProperty(Constants.SERVICE_RANKING);
if (orderObj == null) {
- // filter order is defined as lower value has higher
priority
- // while service ranking is the opposite
- // In addition we allow different types than Integer
+ // filter order is defined as lower value has higher
+ // priority while service ranking is the opposite In
+ // addition we allow different types than Integer
orderObj =
reference.getProperty(EngineConstants.FILTER_ORDER);
- if ( orderObj != null ) {
- // we can use 0 as the default as this will be applied
in
- // the next step anyway if this props contains an
invalid
- // value
- Integer order = OsgiUtil.toInteger(orderObj, 0);
- order = order * -1;
+ if (orderObj != null) {
+ // we can use 0 as the default as this will be applied
+ // in the next step anyway if this props contains an
+ // invalid value
+ orderSource = EngineConstants.FILTER_ORDER + "=" +
orderObj;
+ orderObj = Integer.valueOf(-1 *
OsgiUtil.toInteger(orderObj, 0));
+ } else {
+ orderSource = "none";
}
+ } else {
+ orderSource = Constants.SERVICE_RANKING + "=" + orderObj;
}
- final int order = (orderObj instanceof Integer)
- ? ((Integer) orderObj).intValue()
- : 0;
+ final int order = (orderObj instanceof Integer) ? ((Integer)
orderObj).intValue() : 0;
// register by scope
String[] scopes = OsgiUtil.toStringArray(
@@ -221,13 +224,13 @@ public class ServletFilterManager extend
try {
FilterChainType type =
FilterChainType.valueOf(scope.toString());
getFilterChain(type).addFilter(filter, serviceId,
- order);
+ order, orderSource);
if (type == FilterChainType.COMPONENT) {
getFilterChain(FilterChainType.INCLUDE).addFilter(
- filter, serviceId, order);
+ filter, serviceId, order, orderSource);
getFilterChain(FilterChainType.FORWARD).addFilter(
- filter, serviceId, order);
+ filter, serviceId, order, orderSource);
}
} catch (IllegalArgumentException iae) {
@@ -239,7 +242,7 @@ public class ServletFilterManager extend
"A Filter (Service ID %s) has been registered without
a filter.scope property.",
reference.getProperty(Constants.SERVICE_ID)));
getFilterChain(FilterChainType.REQUEST).addFilter(filter,
- serviceId, order);
+ serviceId, order, orderSource);
}
} catch (ServletException ce) {
Modified:
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelper.java
URL:
http://svn.apache.org/viewvc/sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelper.java?rev=1596846&r1=1596845&r2=1596846&view=diff
==============================================================================
---
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelper.java
(original)
+++
sling/trunk/bundles/engine/src/main/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelper.java
Thu May 22 12:47:10 2014
@@ -33,33 +33,34 @@ import javax.servlet.Filter;
*/
public class SlingFilterChainHelper {
- SortedSet<FilterListEntry> filterList;
+ private static final Filter[] EMPTY_FILTER_ARRAY = new Filter[0];
- Filter[] filters;
+ private SortedSet<FilterListEntry> filterList;
+
+ private Filter[] filters = EMPTY_FILTER_ARRAY;
SlingFilterChainHelper() {
}
- public synchronized Filter addFilter(Filter filter,
- Long filterId, int order) {
- filters = null;
+ public synchronized Filter addFilter(final Filter filter,
+ final Long filterId, final int order, final String orderSource) {
if (filterList == null) {
filterList = new TreeSet<FilterListEntry>();
}
- filterList.add(new FilterListEntry(filter, filterId, order));
+ filterList.add(new FilterListEntry(filter, filterId, order,
orderSource));
+ filters = this.getFilterArray();
return filter;
}
-
+/*
public synchronized Filter[] removeAllFilters() {
// will be returned after cleaning the lists
Filter[] removedFilters = getFilters();
- filters = null;
+ filters = EMPTY_FILTER_ARRAY;
filterList = null;
return removedFilters;
}
-
public synchronized Filter removeFilter(Filter filter) {
if (filterList != null) {
filters = null;
@@ -67,50 +68,61 @@ public class SlingFilterChainHelper {
FilterListEntry test = fi.next();
if (test.getFilter().equals(filter)) {
fi.remove();
+ filters = this.getFilterArray();
return test.getFilter();
}
}
}
- // no removed ComponentFilter
+ // no removed filter
return null;
}
+*/
- public synchronized boolean removeFilterById(Object filterId) {
+ public synchronized boolean removeFilterById(final Object filterId) {
if (filterList != null) {
- filters = null;
for (Iterator<FilterListEntry> fi = filterList.iterator();
fi.hasNext();) {
FilterListEntry test = fi.next();
- if (test.getFitlerId() == filterId
- || (test.getFitlerId() != null &&
test.getFitlerId().equals(
+ if (test.getFilterId() == filterId
+ || (test.getFilterId() != null &&
test.getFilterId().equals(
filterId))) {
fi.remove();
+ filters = this.getFilterArray();
return true;
}
}
}
- // no removed ComponentFilter
+ // no removed filter
return false;
}
/**
* Returns the list of <code>Filter</code>s added to this instance
* or <code>null</code> if no filters have been added.
+ * This method doesn't need to be synced as it is called from synced
methods.
*/
- public synchronized Filter[] getFilters() {
- if (filters == null) {
- if (filterList != null && !filterList.isEmpty()) {
- Filter[] tmp = new Filter[filterList.size()];
- int i = 0;
- for (FilterListEntry entry : filterList) {
- tmp[i] = entry.getFilter();
- i++;
- }
- filters = tmp;
+ private Filter[] getFilterArray() {
+ if (filterList != null && !filterList.isEmpty()) {
+ final Filter[] tmp = new Filter[filterList.size()];
+ int i = 0;
+ for (FilterListEntry entry : filterList) {
+ tmp[i] = entry.getFilter();
+ i++;
}
+ return tmp;
}
- return filters;
+ return EMPTY_FILTER_ARRAY;
+ }
+
+ /**
+ * Returns the list of <code>Filter</code>s added to this instance
+ * or <code>null</code> if no filters have been added.
+ * This method doesn't need to be synced as it is only
+ * returned the current cached filter array.
+ */
+ public Filter[] getFilters() {
+ return this.filters;
}
/**
@@ -134,17 +146,20 @@ public class SlingFilterChainHelper {
private final int order;
- FilterListEntry(Filter filter, Long filterId, int order) {
+ private final String orderSource;
+
+ FilterListEntry(final Filter filter, final Long filterId, final int
order, final String orderSource) {
this.filter = filter;
this.filterId = filterId;
this.order = order;
+ this.orderSource = orderSource;
}
public Filter getFilter() {
return filter;
}
- public Long getFitlerId() {
+ public Long getFilterId() {
return filterId;
}
@@ -152,22 +167,27 @@ public class SlingFilterChainHelper {
return order;
}
+ public String getOrderSource() {
+ return orderSource;
+ }
+
/**
* Note: this class has a natural ordering that is inconsistent with
* equals.
*/
- public int compareTo(FilterListEntry other) {
+ public int compareTo(final FilterListEntry other) {
if (this == other || equals(other)) {
return 0;
}
- if (order < other.order) {
+ // new service.ranking order (correct)
+ if (order > other.order) {
return -1;
- } else if (order > other.order) {
+ } else if (order < other.order) {
return 1;
}
- // if the filterId is comparable and the other is of the same class
+ // compare filter id (service id)
if (filterId != null && other.filterId != null) {
int comp = filterId.compareTo(other.filterId);
if (comp != 0) {
@@ -175,8 +195,8 @@ public class SlingFilterChainHelper {
}
}
- // this is inserted, obj is existing key
- return 1; // insert after current key
+ // consider equal ranking
+ return 0;
}
@Override
@@ -187,13 +207,14 @@ public class SlingFilterChainHelper {
return filter.hashCode();
}
- public boolean equals(Object obj) {
+ @Override
+ public boolean equals(final Object obj) {
if (this == obj) {
return true;
}
if (obj instanceof FilterListEntry) {
- FilterListEntry other = (FilterListEntry) obj;
+ final FilterListEntry other = (FilterListEntry) obj;
return getFilter().equals(other.getFilter());
}
Added:
sling/trunk/bundles/engine/src/test/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelperTest.java
URL:
http://svn.apache.org/viewvc/sling/trunk/bundles/engine/src/test/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelperTest.java?rev=1596846&view=auto
==============================================================================
---
sling/trunk/bundles/engine/src/test/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelperTest.java
(added)
+++
sling/trunk/bundles/engine/src/test/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelperTest.java
Thu May 22 12:47:10 2014
@@ -0,0 +1,53 @@
+/*
+ * 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.engine.impl.filter;
+
+import static org.junit.Assert.assertEquals;
+
+import javax.servlet.Filter;
+
+import org.jmock.Mockery;
+import org.jmock.integration.junit4.JMock;
+import org.jmock.integration.junit4.JUnit4Mockery;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+/**
+ * Basic tests for the filter chain.
+ */
+@RunWith(JMock.class)
+public class SlingFilterChainHelperTest {
+
+ private final Mockery context = new JUnit4Mockery();
+ @Test public void testOrdering() {
+ final SlingFilterChainHelper chain = new SlingFilterChainHelper();
+
+ chain.addFilter(context.mock(Filter.class, "A"), 1L, 100, "1:100");
+ chain.addFilter(context.mock(Filter.class, "B"), 2L, 100, "2:100");
+ chain.addFilter(context.mock(Filter.class, "C"), 3L, -100, "3:-100");
+ chain.addFilter(context.mock(Filter.class, "D"), 4L, -1000, "4:-1000");
+ chain.addFilter(context.mock(Filter.class, "E"), 5L, 1000, "5:1000");
+
+ final SlingFilterChainHelper.FilterListEntry[] entries =
chain.getFilterListEntries();
+ assertEquals(5, entries.length);
+ assertEquals("5:1000", entries[0].getOrderSource());
+ assertEquals("1:100", entries[1].getOrderSource());
+ assertEquals("2:100", entries[2].getOrderSource());
+ assertEquals("3:-100", entries[3].getOrderSource());
+ assertEquals("4:-1000", entries[4].getOrderSource());
+ }
+}
Propchange:
sling/trunk/bundles/engine/src/test/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelperTest.java
------------------------------------------------------------------------------
svn:eol-style = native
Propchange:
sling/trunk/bundles/engine/src/test/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelperTest.java
------------------------------------------------------------------------------
svn:keywords = author date id revision rev url
Propchange:
sling/trunk/bundles/engine/src/test/java/org/apache/sling/engine/impl/filter/SlingFilterChainHelperTest.java
------------------------------------------------------------------------------
svn:mime-type = text/plain
