From 3c799e6e05a44989016912b8b80af6d653f28035 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Wed, 22 Jul 2015 17:50:05 +0200 Subject: [PATCH] Do not reuse mock requests in Spring MVC Test SPR-13211 introduced support for reusing mock requests in Spring MVC Test if the request was created by the the Spring TestContext Framework. Unfortunately, that change makes it impossible for MockMvc.perform() to be invoked multiple times within the same test method without side effects. For example, session attributes and request parameters are transparently and unexpectedly retained for subsequent invocations of perform(), causing certain categories of tests to fail. This commit reverts the changes introduced in SPR-13211 and introduces a new MockMvcReuseTests class to serve as regression tests within Spring's test suite. Issue: SPR-13260, SPR-13211 --- .../MockHttpServletRequestBuilder.java | 29 +---- ...ockMultipartHttpServletRequestBuilder.java | 1 - .../test/web/servlet/MockMvcReuseTests.java | 123 ++++++++++++++++++ ...stAttributesRequestContextHolderTests.java | 4 +- .../spr/RequestContextHolderTests.java | 33 +++-- 5 files changed, 151 insertions(+), 39 deletions(-) create mode 100644 spring-test/src/test/java/org/springframework/test/web/servlet/MockMvcReuseTests.java diff --git a/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockHttpServletRequestBuilder.java b/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockHttpServletRequestBuilder.java index 41c315ada0..ba8f678dc2 100644 --- a/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockHttpServletRequestBuilder.java +++ b/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockHttpServletRequestBuilder.java @@ -29,7 +29,6 @@ import java.util.Map.Entry; import javax.servlet.ServletContext; import javax.servlet.ServletRequest; import javax.servlet.http.Cookie; -import javax.servlet.http.HttpServletRequest; import org.springframework.beans.Mergeable; import org.springframework.beans.factory.NoSuchBeanDefinitionException; @@ -39,7 +38,6 @@ import org.springframework.http.MediaType; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockHttpSession; -import org.springframework.test.context.web.ServletTestExecutionListener; import org.springframework.test.web.servlet.MockMvc; import org.springframework.util.Assert; import org.springframework.util.LinkedMultiValueMap; @@ -48,9 +46,6 @@ import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.ValueConstants; import org.springframework.web.context.WebApplicationContext; -import org.springframework.web.context.request.RequestAttributes; -import org.springframework.web.context.request.RequestContextHolder; -import org.springframework.web.context.request.ServletRequestAttributes; import org.springframework.web.context.support.WebApplicationContextUtils; import org.springframework.web.servlet.DispatcherServlet; import org.springframework.web.servlet.FlashMap; @@ -645,31 +640,11 @@ public class MockHttpServletRequestBuilder } /** - * Create a {@link MockHttpServletRequest}. - *

If an instance of {@code MockHttpServletRequest} that was created - * by the Spring TestContext Framework is available via the - * {@link RequestAttributes} bound to the current thread in - * {@link RequestContextHolder}, this method simply returns that instance. - *

Otherwise, this method creates a new {@code MockHttpServletRequest} - * based on the supplied {@link ServletContext}. + * Create a new {@link MockHttpServletRequest} based on the supplied + * {@code ServletContext}. *

Can be overridden in subclasses. - * @see RequestContextHolder#getRequestAttributes() - * @see ServletRequestAttributes - * @see ServletTestExecutionListener#CREATED_BY_THE_TESTCONTEXT_FRAMEWORK */ protected MockHttpServletRequest createServletRequest(ServletContext servletContext) { - RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes(); - if (requestAttributes instanceof ServletRequestAttributes) { - HttpServletRequest request = ((ServletRequestAttributes) requestAttributes).getRequest(); - if (request instanceof MockHttpServletRequest) { - MockHttpServletRequest mockRequest = (MockHttpServletRequest) request; - Object createdByTcf = mockRequest.getAttribute(ServletTestExecutionListener.CREATED_BY_THE_TESTCONTEXT_FRAMEWORK); - if (Boolean.TRUE.equals(createdByTcf)) { - return mockRequest; - } - } - } - return new MockHttpServletRequest(servletContext); } diff --git a/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockMultipartHttpServletRequestBuilder.java b/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockMultipartHttpServletRequestBuilder.java index f9f77d36d6..b56e8a9f15 100644 --- a/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockMultipartHttpServletRequestBuilder.java +++ b/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockMultipartHttpServletRequestBuilder.java @@ -109,7 +109,6 @@ public class MockMultipartHttpServletRequestBuilder extends MockHttpServletReque * Create a new {@link MockMultipartHttpServletRequest} based on the * supplied {@code ServletContext} and the {@code MockMultipartFiles} * added to this builder. - *

Can be overridden in subclasses. */ @Override protected final MockHttpServletRequest createServletRequest(ServletContext servletContext) { diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/MockMvcReuseTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/MockMvcReuseTests.java new file mode 100644 index 0000000000..a54a3b4bb6 --- /dev/null +++ b/spring-test/src/test/java/org/springframework/test/web/servlet/MockMvcReuseTests.java @@ -0,0 +1,123 @@ +/* + * 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.test.web.servlet; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.test.context.web.WebAppConfiguration; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.context.WebApplicationContext; +import org.springframework.web.servlet.config.annotation.EnableWebMvc; + +import static org.hamcrest.CoreMatchers.*; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; +import static org.springframework.test.web.servlet.setup.MockMvcBuilders.*; + +/** + * Integration tests that verify that {@link MockMvc} can be reused multiple + * times within the same test method without side effects between independent + * requests. + *

See SPR-13260. + * + * @author Sam Brannen + * @author Rob Winch + * @since 4.2 + */ +@RunWith(SpringJUnit4ClassRunner.class) +@ContextConfiguration +@WebAppConfiguration +public class MockMvcReuseTests { + + private static final String HELLO = "hello"; + private static final String ENIGMA = "enigma"; + private static final String FOO = "foo"; + private static final String BAR = "bar"; + + @Autowired + private WebApplicationContext wac; + + private MockMvc mvc; + + + @Before + public void setUp() { + this.mvc = webAppContextSetup(this.wac).build(); + } + + @Test + public void sessionAttributesAreClearedBetweenInvocations() throws Exception { + + this.mvc.perform(get("/")) + .andExpect(content().string(HELLO)) + .andExpect(request().sessionAttribute(FOO, nullValue())); + + this.mvc.perform(get("/").sessionAttr(FOO, BAR)) + .andExpect(content().string(HELLO)) + .andExpect(request().sessionAttribute(FOO, BAR)); + + this.mvc.perform(get("/")) + .andExpect(content().string(HELLO)) + .andExpect(request().sessionAttribute(FOO, nullValue())); + } + + @Test + public void requestParametersAreClearedBetweenInvocations() throws Exception { + this.mvc.perform(get("/")) + .andExpect(content().string(HELLO)); + + this.mvc.perform(get("/").param(ENIGMA, "")) + .andExpect(content().string(ENIGMA)); + + this.mvc.perform(get("/")) + .andExpect(content().string(HELLO)); + } + + + @Configuration + @EnableWebMvc + static class Config { + + @Bean + public MyController myController() { + return new MyController(); + } + } + + @RestController + static class MyController { + + @RequestMapping("/") + public String hello() { + return HELLO; + } + + @RequestMapping(path = "/", params = ENIGMA) + public String enigma() { + return ENIGMA; + } + } + +} diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/CustomRequestAttributesRequestContextHolderTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/CustomRequestAttributesRequestContextHolderTests.java index 4df80b9d31..a50125b17b 100644 --- a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/CustomRequestAttributesRequestContextHolderTests.java +++ b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/CustomRequestAttributesRequestContextHolderTests.java @@ -30,7 +30,6 @@ import org.springframework.context.annotation.Configuration; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockServletContext; -import org.springframework.test.context.web.ServletTestExecutionListener; import org.springframework.test.web.servlet.MockMvc; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; @@ -50,8 +49,7 @@ import static org.springframework.test.web.servlet.setup.MockMvcBuilders.*; /** * Integration tests for SPR-13211 which verify that a custom mock request - * (i.e., one not created by {@link ServletTestExecutionListener}) is not - * reused by MockMvc. + * is not reused by MockMvc. * * @author Sam Brannen * @since 4.2 diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/RequestContextHolderTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/RequestContextHolderTests.java index 6afc3f626f..acfb8c6a41 100644 --- a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/RequestContextHolderTests.java +++ b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/RequestContextHolderTests.java @@ -61,7 +61,7 @@ import static org.springframework.test.web.servlet.setup.MockMvcBuilders.*; *

* * @author Rossen Stoyanchev @@ -137,7 +137,7 @@ public class RequestContextHolderTests { @After public void verifyRestoredRequestAttributes() { - assertRequestAttributes(); + assertRequestAttributes(false); } @@ -294,17 +294,34 @@ public class RequestContextHolderTests { private static void assertRequestAttributes() { + assertRequestAttributes(true); + } + + private static void assertRequestAttributes(boolean withinMockMvc) { RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes(); assertThat(requestAttributes, instanceOf(ServletRequestAttributes.class)); - assertRequestAttributes(((ServletRequestAttributes) requestAttributes).getRequest()); + assertRequestAttributes(((ServletRequestAttributes) requestAttributes).getRequest(), withinMockMvc); } private static void assertRequestAttributes(ServletRequest request) { - assertThat(request.getAttribute(FROM_TCF_MOCK), is(FROM_TCF_MOCK)); - assertThat(request.getAttribute(FROM_MVC_TEST_DEFAULT), is(FROM_MVC_TEST_DEFAULT)); - assertThat(request.getAttribute(FROM_MVC_TEST_MOCK), is(FROM_MVC_TEST_MOCK)); - assertThat(request.getAttribute(FROM_REQUEST_FILTER), is(FROM_REQUEST_FILTER)); - assertThat(request.getAttribute(FROM_REQUEST_ATTRIBUTES_FILTER), is(FROM_REQUEST_ATTRIBUTES_FILTER)); + assertRequestAttributes(request, true); + } + + private static void assertRequestAttributes(ServletRequest request, boolean withinMockMvc) { + if (withinMockMvc) { + assertThat(request.getAttribute(FROM_TCF_MOCK), is(nullValue())); + assertThat(request.getAttribute(FROM_MVC_TEST_DEFAULT), is(FROM_MVC_TEST_DEFAULT)); + assertThat(request.getAttribute(FROM_MVC_TEST_MOCK), is(FROM_MVC_TEST_MOCK)); + assertThat(request.getAttribute(FROM_REQUEST_FILTER), is(FROM_REQUEST_FILTER)); + assertThat(request.getAttribute(FROM_REQUEST_ATTRIBUTES_FILTER), is(FROM_REQUEST_ATTRIBUTES_FILTER)); + } + else { + assertThat(request.getAttribute(FROM_TCF_MOCK), is(FROM_TCF_MOCK)); + assertThat(request.getAttribute(FROM_MVC_TEST_DEFAULT), is(nullValue())); + assertThat(request.getAttribute(FROM_MVC_TEST_MOCK), is(nullValue())); + assertThat(request.getAttribute(FROM_REQUEST_FILTER), is(nullValue())); + assertThat(request.getAttribute(FROM_REQUEST_ATTRIBUTES_FILTER), is(nullValue())); + } } }