MockMvc.perform with the TestContext framework causes additional perform invocations to be "polluted" [SPR-13260]

[spring-projects-issues]

Rob Winch opened SPR-13260 and commented

The following has worked in prior versions of Spring Framework and the 4.2.0.BUILD-SNAPSHOT up until today. To be clear I believe this bug is a regression that is in 4.2.0.BUILD-SNAPSHOT.

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration
@WebAppConfiguration
public class MockMvcPerformTwiceTests {
	@Autowired
	WebApplicationContext context;

	@Test
	public void mockMvcPerformTwice() throws Exception {
		final String SESSION_ATTR = "mockMvcPerformTwice";
		MockMvc mvc = MockMvcBuilders
				.webAppContextSetup(context)
				.build();

		mvc
			.perform(get("/").sessionAttr(SESSION_ATTR, "value"))
			.andExpect(request().sessionAttribute(SESSION_ATTR, not(nullValue())));

		mvc
			.perform(get("/"))
			.andExpect(request().sessionAttribute(SESSION_ATTR, nullValue()));
	}

	@Configuration
	@EnableWebMvc
	static class Config {
		@Bean
		public MyController myController() {
			return new MyController();
		}
	}

	@RestController
	static class MyController {
		@RequestMapping("/")
		public String hello() {
			return "hello";
		}
	}
}

I believe this is related to the changes in #17803 but do not have time investigate further. I wanted to log this issue to ensure we figure out what is going on before GA.

---

Affects: 4.2 GA

Issue Links:

• Reuse MockHttpServletRequest from ServletTestExecutionListener in Spring MVC Test framework [SPR-13211] #17803 Reuse MockHttpServletRequest from ServletTestExecutionListener in Spring MVC Test framework

Referenced from: commits 3c799e6

[spring-projects-issues]

Sam Brannen commented

Rob Winch,

This is in fact a regression that was introduced in #17803.

I will therefore have to back out those changes.

Thanks for bringing this to my attention!

Sam

[spring-projects-issues]

Sam Brannen commented

If you think about, the presence of a session attribute from a previous invocation of MockMvc.perform() actually makes sense. So I would argue that is actually desirable since it allows for test cases that span multiple requests but retain the same session, analogous to a production environment.

However... what is definitely undesirable is the fact that everything else related to the mocked request (including request parameters) is retained across invocations of MockMvc.perform() which would never be the case in a production environment.

In light of that, I will revert the changes committed for #17803.

[spring-projects-issues]

Rob Winch commented

Sam Brannen Thanks for the fast response and turnaround! Reverting the changes will indeed resolve my issue.

It seems it does not matter at the moment (and perhaps never will), but to steer possible future direction...

I do not think that sharing the session attributes makes sense.

• To me reusing the session is very misleading as the RequestBuilder implementations do not indicate that previous sessions will be used.
• Reusing the session within a test method is unintuitive since the methods are static and no value produced from the first request is passed into the second request.
• It is also confusing since this behavior only happen when using TestContext framework.
• MockMvcBuilders already has a concept of defaultRequest which is pretty explicit that it is merged with additional requests, this makes it quite unintuitive that the session is also defaulted from previous invocations
• This breaks existing use of MockMvc which relies on the fact that each request is independent

[spring-projects-issues]

Sam Brannen commented

Fixed in GitHub commit 3c799e6:

Do not reuse mock requests in Spring MVC Test

#17803 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 #17803 and introduces
a new MockMvcReuseTests class to serve as regression tests within
Spring's test suite.

[spring-projects-issues]

Sam Brannen commented

Thanks for the feedback, Rob.

Yes, I don't think it will matter too much... unless someone someday requests support for shared session state across invocations of MockMvc.perform(), but if we did go that route... it would have to be via an opt-in configuration flag for a given MockMvc instance.

Thanks to you, we now have tests in place to make sure the previous behavior is retained by default. ;)

Cheers,

Sam

[spring-projects-issues]

Rossen Stoyanchev commented

The expectation is that the mock request is fully set up each time. If you want to re-use session attributes, then you can access the request from the MvcResult with .andReturn() and then manually set up the session attributes on the next request being performed. Something similar happens with MockMvcRequestBuilders#asyncDispatch where the MvcResult from the async request is used to set up the async dispatch (see the Javadoc for an example).