SLING-10179 better handling of doFilter misusage (#13)
- in case doFilter is called more than once by a custom filter, an explicit IllegalStateException will be thrown, instead of an ArrayIndexOutOfBoundsException,
- added unit test case (that bring some coverage to AbstractFilterChain)
diff --git a/src/main/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChain.java b/src/main/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChain.java
index 0841015..245f8b5 100644
--- a/src/main/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChain.java
+++ b/src/main/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChain.java
@@ -53,6 +53,12 @@
final int filterIdx = ++this.current;
final long start = System.currentTimeMillis();
+ if (filterIdx > this.filters.length) {
+ //this happens when the whole filter chain has been executed, and a filter in that chain,
+ //for some (bad) reason, calls doFilter yet another time.
+ throw new IllegalStateException("doFilter should not be called more than once");
+ }
+
// the previous filter may have wrapped non-Sling request and response
// wrappers (e.g. WebCastellum does this), so we have to make
// sure the request and response are Sling types again
@@ -67,11 +73,11 @@
FilterHandle filter = this.filters[this.current];
if (filter.select(slingRequest)) {
- LOG.debug("{} got selected for this request", filter);
+ LOG.debug("{} got selected for this request", filter);
trackFilter(slingRequest, filter);
filter.getFilter().doFilter(slingRequest, slingResponse, this);
} else {
- LOG.debug("{} was not selected for this request", filter);
+ LOG.debug("{} was not selected for this request", filter);
if (this.current == this.filters.length-1) {
this.render(slingRequest, slingResponse);
} else {
@@ -83,9 +89,11 @@
}
} finally {
- times[filterIdx] = System.currentTimeMillis() - start;
- if (filterIdx == 0) {
- consolidateFilterTimings(slingRequest);
+ if (filterIdx < times.length) {
+ times[filterIdx] = System.currentTimeMillis() - start;
+ if (filterIdx == 0) {
+ consolidateFilterTimings(slingRequest);
+ }
}
}
}
diff --git a/src/test/java/org/apache/sling/engine/impl/filter/AbstractFilterTest.java b/src/test/java/org/apache/sling/engine/impl/filter/AbstractFilterTest.java
index 1cf2cb8..2c7a7cd 100644
--- a/src/test/java/org/apache/sling/engine/impl/filter/AbstractFilterTest.java
+++ b/src/test/java/org/apache/sling/engine/impl/filter/AbstractFilterTest.java
@@ -23,6 +23,7 @@
import javax.servlet.Filter;
+import junit.framework.TestCase;
import org.apache.sling.api.SlingHttpServletRequest;
import org.apache.sling.api.request.RequestPathInfo;
import org.jmock.Expectations;
@@ -34,7 +35,7 @@
import org.osgi.framework.ServiceReference;
@RunWith(JMock.class)
-public abstract class AbstractFilterTest {
+public abstract class AbstractFilterTest extends TestCase {
protected final Mockery context = new JUnit4Mockery();
protected ServiceReference<Filter> mockService(Object... map){
@@ -103,6 +104,10 @@
will(returnValue(method));
allowing(req).getPathInfo();
will(returnValue(path));
+ allowing(req).getServletPath();
+ will(returnValue(path));
+ allowing(req).getAttribute(with(any(String.class)));
+ will(returnValue(new Object()));
}});
return req;
}
diff --git a/src/test/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChainTest.java b/src/test/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChainTest.java
new file mode 100644
index 0000000..3d07c3a
--- /dev/null
+++ b/src/test/java/org/apache/sling/engine/impl/filter/AbstractSlingFilterChainTest.java
@@ -0,0 +1,77 @@
+/*
+ * 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 org.apache.sling.api.SlingHttpServletRequest;
+import org.apache.sling.api.SlingHttpServletResponse;
+import org.apache.sling.engine.impl.SlingHttpServletRequestImpl;
+import org.apache.sling.engine.impl.SlingRequestProcessorImpl;
+import org.apache.sling.engine.impl.request.RequestData;
+
+import org.junit.Test;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+public class AbstractSlingFilterChainTest extends AbstractFilterTest {
+
+ @Test
+ public void testDoubleCall() throws Exception {
+ Filter badFilter = new Filter() {
+ @Override
+ public void init(FilterConfig filterConfig) throws ServletException {}
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
+ chain.doFilter(request, response);
+ chain.doFilter(request, response);
+ }
+
+ @Override
+ public void destroy() { }
+ };
+
+ FilterHandle handle = new FilterHandle(badFilter, null, 1, 1, null, null);
+
+ AbstractSlingFilterChain chain = new AbstractSlingFilterChain(new FilterHandle[]{ handle }) {
+ @Override
+ protected void render(SlingHttpServletRequest request, SlingHttpServletResponse response) throws IOException, ServletException {
+ }
+ };
+ HttpServletRequest httpReq = whateverRequest();
+ final RequestData requestData = new RequestData(new SlingRequestProcessorImpl(), httpReq,
+ context.mock(HttpServletResponse.class));
+ final SlingHttpServletRequestImpl req = new SlingHttpServletRequestImpl(requestData, httpReq);
+ boolean illegalStateCaught = false;
+ try {
+ chain.doFilter(req, context.mock(SlingHttpServletResponse.class));
+ } catch (IllegalStateException e) {
+ illegalStateCaught = true;
+ }
+ assertTrue("an illegal state exception should have been caught", illegalStateCaught);
+ }
+
+}
