From c36435c0427c75d4dcee382a88a466ad96241faa Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Tue, 19 May 2015 12:21:35 +0200 Subject: [PATCH] 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 --- .../handler/AbstractHandlerMapping.java | 55 ++++++----- .../servlet/handler/MappedInterceptor.java | 30 +++++- .../handler/AbstractHandlerMappingTests.java | 92 +++++++++++++++++++ .../handler/MappedInterceptorTests.java | 41 ++++++++- 4 files changed, 190 insertions(+), 28 deletions(-) create mode 100644 spring-webmvc/src/test/java/org/springframework/web/servlet/handler/AbstractHandlerMappingTests.java diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMapping.java index a68eb3903c9..6b07cd9438a 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMapping.java +++ b/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 interceptors = new ArrayList(); - private final List adaptedInterceptors = new ArrayList(); + private final List handlerInterceptors = new ArrayList(); - private final List mappedInterceptors = new ArrayList(); + private final List detectedMappedInterceptors = new ArrayList(); 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 adaptedInterceptors = new ArrayList(); + 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 mappedInterceptors = new ArrayList(); + 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. *

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. *

NOTE: 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; } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/MappedInterceptor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/MappedInterceptor.java index 88b1b86d9f2..a238d537ea7 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/MappedInterceptor.java +++ b/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. * *

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 diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/AbstractHandlerMappingTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/AbstractHandlerMappingTests.java new file mode 100644 index 00000000000..cc9fa838158 --- /dev/null +++ b/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()); + } + + } + +} diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/MappedInterceptorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/MappedInterceptorTests.java index 480b984e7c1..6c25c27cda9 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/handler/MappedInterceptorTests.java +++ b/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 {