Execute HandlerInterceptors in registration order

Prior to this commit, registering `HandlerInterceptor`s using the
`InterceptorRegistry` would not guarantee their order of execution. In
fact, `HandlerInterceptor`s would always be executed before
`MappedInterceptor`s.

This change makes `MappedInterceptor` implement the `HandlerInterceptor`
interface, in order to register all interceptors in a single ordered
list. The order of execution of interceptors is now guaranteed in the
`HandlerExecutionChain` built by `AbstractHandlerMapping`.

Issue: SPR-12673

spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMapping.java

@@ -19,7 +19,6 @@ package org.springframework.web.servlet.handler;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -78,9 +77,9 @@ public abstract class AbstractHandlerMapping extends WebApplicationObjectSupport
 
 	private final List<Object> interceptors = new ArrayList<Object>();
 
-	private final List<HandlerInterceptor> adaptedInterceptors = new ArrayList<HandlerInterceptor>();
+	private final List<HandlerInterceptor> handlerInterceptors = new ArrayList<HandlerInterceptor>();
 
-	private final List<MappedInterceptor> mappedInterceptors = new ArrayList<MappedInterceptor>();
+	private final List<MappedInterceptor> detectedMappedInterceptors = new ArrayList<MappedInterceptor>();
 
 	private CorsProcessor corsProcessor = new DefaultCorsProcessor();
 	
@@ -246,7 +245,7 @@ public abstract class AbstractHandlerMapping extends WebApplicationObjectSupport
 	@Override
 	protected void initApplicationContext() throws BeansException {
 		extendInterceptors(this.interceptors);
-		detectMappedInterceptors(this.mappedInterceptors);
+		detectMappedInterceptors(this.detectedMappedInterceptors);
 		initInterceptors();
 	}
 
@@ -288,14 +287,11 @@ public abstract class AbstractHandlerMapping extends WebApplicationObjectSupport
 				if (interceptor == null) {
 					throw new IllegalArgumentException("Entry number " + i + " in interceptors array is null");
 				}
-				if (interceptor instanceof MappedInterceptor) {
-					this.mappedInterceptors.add((MappedInterceptor) interceptor);
-				}
-				else {
-					this.adaptedInterceptors.add(adaptInterceptor(interceptor));
-				}
+				this.handlerInterceptors.add(adaptInterceptor(interceptor));
 			}
 		}
+		this.handlerInterceptors.addAll(this.detectedMappedInterceptors);
+		this.interceptors.clear();
 	}
 
 	/**
@@ -327,8 +323,14 @@ public abstract class AbstractHandlerMapping extends WebApplicationObjectSupport
 	 * @return the array of {@link HandlerInterceptor}s, or {@code null} if none
 	 */
 	protected final HandlerInterceptor[] getAdaptedInterceptors() {
-		int count = this.adaptedInterceptors.size();
-		return (count > 0 ? this.adaptedInterceptors.toArray(new HandlerInterceptor[count]) : null);
+		List<HandlerInterceptor> adaptedInterceptors = new ArrayList<HandlerInterceptor>();
+		for (HandlerInterceptor interceptor : this.handlerInterceptors) {
+			if (!(interceptor instanceof MappedInterceptor)) {
+				adaptedInterceptors.add(interceptor);
+			}
+		}
+		int count = adaptedInterceptors.size();
+		return (count > 0 ? adaptedInterceptors.toArray(new HandlerInterceptor[count]) : null);
 	}
 
 	/**
@@ -336,8 +338,14 @@ public abstract class AbstractHandlerMapping extends WebApplicationObjectSupport
 	 * @return the array of {@link MappedInterceptor}s, or {@code null} if none
 	 */
 	protected final MappedInterceptor[] getMappedInterceptors() {
-		int count = this.mappedInterceptors.size();
-		return (count > 0 ? this.mappedInterceptors.toArray(new MappedInterceptor[count]) : null);
+		List<MappedInterceptor> mappedInterceptors = new ArrayList<MappedInterceptor>();
+		for (HandlerInterceptor interceptor : this.handlerInterceptors) {
+			if (interceptor instanceof MappedInterceptor) {
+				mappedInterceptors.add((MappedInterceptor) interceptor);
+			}
+		}
+		int count = mappedInterceptors.size();
+		return (count > 0 ? mappedInterceptors.toArray(new MappedInterceptor[count]) : null);
 	}
 
 	/**
@@ -397,8 +405,9 @@ public abstract class AbstractHandlerMapping extends WebApplicationObjectSupport
 	 * applicable interceptors.
 	 * <p>The default implementation builds a standard {@link HandlerExecutionChain}
 	 * with the given handler, the handler mapping's common interceptors, and any
-	 * {@link MappedInterceptor}s matching to the current request URL. Subclasses
-	 * may override this in order to extend/rearrange the list of interceptors.
+	 * {@link MappedInterceptor}s matching to the current request URL. Interceptors
+	 * are added in the order they were registered. Subclasses may override this
+	 * in order to extend/rearrange the list of interceptors.
 	 * <p><b>NOTE:</b> The passed-in handler object may be a raw handler or a
 	 * pre-built {@link HandlerExecutionChain}. This method should handle those
 	 * two cases explicitly, either building a new {@link HandlerExecutionChain}
@@ -414,15 +423,19 @@ public abstract class AbstractHandlerMapping extends WebApplicationObjectSupport
 	protected HandlerExecutionChain getHandlerExecutionChain(Object handler, HttpServletRequest request) {
 		HandlerExecutionChain chain = (handler instanceof HandlerExecutionChain ?
 				(HandlerExecutionChain) handler : new HandlerExecutionChain(handler));
-		chain.addInterceptors(getAdaptedInterceptors());
 
 		String lookupPath = this.urlPathHelper.getLookupPathForRequest(request);
-		for (MappedInterceptor mappedInterceptor : this.mappedInterceptors) {
-			if (mappedInterceptor.matches(lookupPath, this.pathMatcher)) {
-				chain.addInterceptor(mappedInterceptor.getInterceptor());
+		for (HandlerInterceptor interceptor : this.handlerInterceptors) {
+			if (interceptor instanceof MappedInterceptor) {
+				MappedInterceptor mappedInterceptor = (MappedInterceptor) interceptor;
+				if (mappedInterceptor.matches(lookupPath, this.pathMatcher)) {
+					chain.addInterceptor(mappedInterceptor.getInterceptor());
+				}
+			}
+			else {
+				chain.addInterceptor(interceptor);
 			}
 		}
-
 		return chain;
 	}
 

spring-webmvc/src/main/java/org/springframework/web/servlet/handler/MappedInterceptor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,14 +16,18 @@
 
 package org.springframework.web.servlet.handler;
 
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
 import org.springframework.util.PathMatcher;
 import org.springframework.web.context.request.WebRequestInterceptor;
 import org.springframework.web.servlet.HandlerInterceptor;
+import org.springframework.web.servlet.ModelAndView;
 
 /**
- * Contains a {@link HandlerInterceptor} along with include (and optionally
- * exclude) path patterns to which the interceptor should apply. Also provides
- * matching logic to test if the interceptor applies to a given request path.
+ * Contains and delegates calls to a {@link HandlerInterceptor} along with
+ * include (and optionally exclude) path patterns to which the interceptor should apply.
+ * Also provides matching logic to test if the interceptor applies to a given request path.
  *
  * <p>A MappedInterceptor can be registered directly with any
  * {@link org.springframework.web.servlet.handler.AbstractHandlerMethodMapping
@@ -34,9 +38,10 @@ import org.springframework.web.servlet.HandlerInterceptor;
  *
  * @author Keith Donald
  * @author Rossen Stoyanchev
+ * @author Brian Clozel
  * @since 3.0
  */
-public final class MappedInterceptor {
+public final class MappedInterceptor implements HandlerInterceptor {
 
 	private final String[] includePatterns;
 
@@ -122,6 +127,21 @@ public final class MappedInterceptor {
 		return this.interceptor;
 	}
 
+	@Override
+	public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {
+		return this.interceptor.preHandle(request, response, handler);
+	}
+
+	@Override
+	public void postHandle(HttpServletRequest request, HttpServletResponse response, Object handler, ModelAndView modelAndView) throws Exception {
+		this.interceptor.postHandle(request, response, handler, modelAndView);
+	}
+
+	@Override
+	public void afterCompletion(HttpServletRequest request, HttpServletResponse response, Object handler, Exception ex) throws Exception {
+		this.interceptor.afterCompletion(request, response, handler, ex);
+	}
+
 	/**
 	 * Returns {@code true} if the interceptor applies to the given request path.
 	 * @param lookupPath the current request path

spring-webmvc/src/test/java/org/springframework/web/servlet/handler/AbstractHandlerMappingTests.java

@@ -0,0 +1,92 @@
+/*
+ * Copyright 2002-2015 the original author or authors.
+ *
+ * Licensed 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.springframework.web.servlet.handler;
+
+import java.io.IOException;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.hamcrest.Matchers;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import org.springframework.http.HttpStatus;
+import org.springframework.mock.web.test.MockHttpServletRequest;
+import org.springframework.mock.web.test.MockHttpServletResponse;
+import org.springframework.web.HttpRequestHandler;
+import org.springframework.web.context.support.StaticWebApplicationContext;
+import org.springframework.web.servlet.HandlerExecutionChain;
+import org.springframework.web.servlet.HandlerInterceptor;
+import org.springframework.web.servlet.support.WebContentGenerator;
+
+/**
+ * @author Brian Clozel
+ */
+public class AbstractHandlerMappingTests {
+
+	private MockHttpServletRequest request;
+	private AbstractHandlerMapping handlerMapping;
+	private StaticWebApplicationContext context;
+
+	@Before
+	public void setup() {
+		this.context = new StaticWebApplicationContext();
+		this.handlerMapping = new TestHandlerMapping();
+		this.request = new MockHttpServletRequest();
+	}
+
+	@Test
+	public void orderedInterceptors() throws Exception {
+		MappedInterceptor firstMappedInterceptor = new MappedInterceptor(new String[]{"/**"}, Mockito.mock(HandlerInterceptor.class));
+		HandlerInterceptor secondHandlerInterceptor = Mockito.mock(HandlerInterceptor.class);
+		MappedInterceptor thirdMappedInterceptor = new MappedInterceptor(new String[]{"/**"}, Mockito.mock(HandlerInterceptor.class));
+		HandlerInterceptor fourthHandlerInterceptor = Mockito.mock(HandlerInterceptor.class);
+
+		this.handlerMapping.setInterceptors(new Object[]{firstMappedInterceptor, secondHandlerInterceptor,
+				thirdMappedInterceptor, fourthHandlerInterceptor});
+		this.handlerMapping.setApplicationContext(this.context);
+		HandlerExecutionChain chain = this.handlerMapping.getHandlerExecutionChain(new SimpleHandler(), this.request);
+		Assert.assertThat(chain.getInterceptors(),
+				Matchers.arrayContaining(firstMappedInterceptor, secondHandlerInterceptor, thirdMappedInterceptor, fourthHandlerInterceptor));
+	}
+
+	class TestHandlerMapping extends AbstractHandlerMapping {
+
+		@Override
+		protected Object getHandlerInternal(HttpServletRequest request) throws Exception {
+			return new SimpleHandler();
+		}
+	}
+
+	class SimpleHandler extends WebContentGenerator implements HttpRequestHandler {
+
+		public SimpleHandler() {
+			super(METHOD_GET);
+		}
+
+		@Override
+		public void handleRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
+			response.setStatus(HttpStatus.OK.value());
+		}
+
+	}
+
+}

spring-webmvc/src/test/java/org/springframework/web/servlet/handler/MappedInterceptorTests.java

@@ -15,18 +15,26 @@
  */
 package org.springframework.web.servlet.handler;
 
+import static org.junit.Assert.*;
+import static org.mockito.BDDMockito.any;
+import static org.mockito.BDDMockito.*;
+import static org.mockito.Mockito.mock;
+
 import java.util.Comparator;
 import java.util.Map;
 
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
 import org.junit.Before;
 import org.junit.Test;
 
 import org.springframework.util.AntPathMatcher;
 import org.springframework.util.PathMatcher;
+import org.springframework.web.servlet.HandlerInterceptor;
+import org.springframework.web.servlet.ModelAndView;
 import org.springframework.web.servlet.i18n.LocaleChangeInterceptor;
 
-import static org.junit.Assert.*;
-
 /**
  * Test fixture for {@link MappedInterceptor} tests.
  *
@@ -89,6 +97,35 @@ public class MappedInterceptorTests {
 		assertFalse(mappedInterceptor.matches("/foo/bar", pathMatcher));
 	}
 
+	@Test
+	public void preHandle() throws Exception {
+		HandlerInterceptor interceptor = mock(HandlerInterceptor.class);
+		MappedInterceptor mappedInterceptor = new MappedInterceptor(new String[] { "/**" }, interceptor);
+		mappedInterceptor.preHandle(mock(HttpServletRequest.class), mock(HttpServletResponse.class), null);
+
+		then(interceptor).should().preHandle(any(HttpServletRequest.class), any(HttpServletResponse.class), any());
+	}
+
+	@Test
+	public void postHandle() throws Exception {
+		HandlerInterceptor interceptor = mock(HandlerInterceptor.class);
+		MappedInterceptor mappedInterceptor = new MappedInterceptor(new String[] { "/**" }, interceptor);
+		mappedInterceptor.postHandle(mock(HttpServletRequest.class), mock(HttpServletResponse.class),
+				null, mock(ModelAndView.class));
+
+		then(interceptor).should().postHandle(any(), any(), any(), any());
+	}
+
+	@Test
+	public void afterCompletion() throws Exception {
+		HandlerInterceptor interceptor = mock(HandlerInterceptor.class);
+		MappedInterceptor mappedInterceptor = new MappedInterceptor(new String[] { "/**" }, interceptor);
+		mappedInterceptor.afterCompletion(mock(HttpServletRequest.class), mock(HttpServletResponse.class),
+				null, mock(Exception.class));
+
+		then(interceptor).should().afterCompletion(any(), any(), any(), any());
+	}
+
 
 
 	public static class TestPathMatcher implements PathMatcher {