Skip to content

Commit

Permalink
Execute HandlerInterceptors in registration order
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bclozel committed May 19, 2015
1 parent cf391f5 commit c36435c
Show file tree
Hide file tree
Showing 4 changed files with 190 additions and 28 deletions.
Expand Up @@ -19,7 +19,6 @@
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;
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -246,7 +245,7 @@ public Map<String, CorsConfiguration> getCorsConfiguration() {
@Override
protected void initApplicationContext() throws BeansException {
extendInterceptors(this.interceptors);
detectMappedInterceptors(this.mappedInterceptors);
detectMappedInterceptors(this.detectedMappedInterceptors);
initInterceptors();
}

Expand Down Expand Up @@ -288,14 +287,11 @@ protected void initInterceptors() {
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();
}

/**
Expand Down Expand Up @@ -327,17 +323,29 @@ else if (interceptor instanceof WebRequestInterceptor) {
* @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);
}

/**
* Return all configured {@link MappedInterceptor}s as an array.
* @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);
}

/**
Expand Down Expand Up @@ -397,8 +405,9 @@ public final HandlerExecutionChain getHandler(HttpServletRequest request) throws
* 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}
Expand All @@ -414,15 +423,19 @@ public final HandlerExecutionChain getHandler(HttpServletRequest request) throws
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;
}

Expand Down
@@ -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.
Expand All @@ -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
Expand All @@ -34,9 +38,10 @@
*
* @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;

Expand Down Expand Up @@ -122,6 +127,21 @@ public HandlerInterceptor getInterceptor() {
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
Expand Down
@@ -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());
}

}

}
Expand Up @@ -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.
*
Expand Down Expand Up @@ -89,6 +97,35 @@ public void customPathMatcher() {
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 {
Expand Down

0 comments on commit c36435c

Please sign in to comment.