MockMvc HtmlUnit support shares CookieManager

Previously MockMvc builders failed to share the WebConnection used for
managing cookies in the MockMvcWebConnection. This meant that the various
CookieManagers would have different states.

This commit ensures that the WebConnection is set on the
MockMvcWebConnection.

Fixes SPR-14066

Author: Rob Winch

spring-test/src/main/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebClientBuilder.java

@@ -106,7 +106,7 @@ public static MockMvcWebClientBuilder webAppContextSetup(WebApplicationContext c
 	 */
 	public MockMvcWebClientBuilder withDelegate(WebClient webClient) {
 		Assert.notNull(webClient, "WebClient must not be null");
-		webClient.setWebConnection(createConnection(webClient.getWebConnection()));
+		webClient.setWebConnection(createConnection(webClient));
 		this.webClient = webClient;
 		return this;
 	}

spring-test/src/main/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebConnection.java

@@ -63,16 +63,16 @@ public final class MockMvcWebConnection implements WebConnection {
 
 	private WebClient webClient;
 
-
 	/**
 	 * Create a new instance that assumes the context path of the application
 	 * is {@code ""} (i.e., the root context).
 	 * <p>For example, the URL {@code http://localhost/test/this} would use
 	 * {@code ""} as the context path.
 	 * @param mockMvc the {@code MockMvc} instance to use; never {@code null}
+	 * @param webClient the {@link WebClient} to use. never {@code null}
 	 */
-	public MockMvcWebConnection(MockMvc mockMvc) {
-		this(mockMvc, "");
+	public MockMvcWebConnection(MockMvc mockMvc, WebClient webClient) {
+		this(mockMvc, webClient, "");
 	}
 
 	/**
@@ -83,17 +83,47 @@ public MockMvcWebConnection(MockMvc mockMvc) {
 	 * which states that it can be an empty string and otherwise must start
 	 * with a "/" character and not end with a "/" character.
 	 * @param mockMvc the {@code MockMvc} instance to use; never {@code null}
+	 * @param webClient  the {@link WebClient} to use. never {@code null}
 	 * @param contextPath the contextPath to use
 	 */
-	public MockMvcWebConnection(MockMvc mockMvc, String contextPath) {
+	public MockMvcWebConnection(MockMvc mockMvc, WebClient webClient, String contextPath) {
 		Assert.notNull(mockMvc, "MockMvc must not be null");
+		Assert.notNull(webClient, "WebClient must not be null");
 		validateContextPath(contextPath);
 
-		this.webClient = new WebClient();
+		this.webClient = webClient;
 		this.mockMvc = mockMvc;
 		this.contextPath = contextPath;
 	}
 
+	/**
+	 * Create a new instance that assumes the context path of the application
+	 * is {@code ""} (i.e., the root context).
+	 * <p>For example, the URL {@code http://localhost/test/this} would use
+	 * {@code ""} as the context path.
+	 * @param mockMvc the {@code MockMvc} instance to use; never {@code null}
+	 * @deprecated Use {@link #MockMvcWebConnection(MockMvc, WebClient)}
+	 */
+	@Deprecated
+	public MockMvcWebConnection(MockMvc mockMvc) {
+		this(mockMvc, "");
+	}
+
+	/**
+	 * Create a new instance with the specified context path.
+	 * <p>The path may be {@code null} in which case the first path segment
+	 * of the URL is turned into the contextPath. Otherwise it must conform
+	 * to {@link javax.servlet.http.HttpServletRequest#getContextPath()}
+	 * which states that it can be an empty string and otherwise must start
+	 * with a "/" character and not end with a "/" character.
+	 * @param mockMvc the {@code MockMvc} instance to use; never {@code null}
+	 * @param contextPath the contextPath to use
+	 * @deprecated use {@link #MockMvcWebConnection(MockMvc, WebClient, String)}
+	 */
+	@Deprecated
+	public MockMvcWebConnection(MockMvc mockMvc, String contextPath) {
+		this(mockMvc, new WebClient(), contextPath);
+	}
 
 	public void setWebClient(WebClient webClient) {
 		Assert.notNull(webClient, "WebClient must not be null");

spring-test/src/main/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebConnectionBuilderSupport.java

@@ -19,6 +19,7 @@
 import java.util.ArrayList;
 import java.util.List;
 
+import com.gargoylesoftware.htmlunit.WebClient;
 import com.gargoylesoftware.htmlunit.WebConnection;
 
 import org.springframework.test.web.servlet.MockMvc;
@@ -145,10 +146,46 @@ public T useMockMvcForHosts(String... hosts) {
 	 * @see #alwaysUseMockMvc()
 	 * @see #useMockMvc(WebRequestMatcher...)
 	 * @see #useMockMvcForHosts(String...)
+	 * @deprecated Use {@link #createConnection(WebClient)} instead
 	 */
+	@Deprecated
 	protected final WebConnection createConnection(WebConnection defaultConnection) {
 		Assert.notNull(defaultConnection, "Default WebConnection must not be null");
-		MockMvcWebConnection mockMvcWebConnection = new MockMvcWebConnection(this.mockMvc, this.contextPath);
+		return createConnection(new WebClient(), defaultConnection);
+	}
+
+	/**
+	 * Create a new {@link WebConnection} that will use a {@link MockMvc}
+	 * instance if one of the specified {@link WebRequestMatcher} instances
+	 * matches.
+	 * @param webClient the WebClient to use if none of
+	 * the specified {@code WebRequestMatcher} instances matches; never {@code null}
+	 * @return a new {@code WebConnection} that will use a {@code MockMvc}
+	 * instance if one of the specified {@code WebRequestMatcher} matches
+	 * @see #alwaysUseMockMvc()
+	 * @see #useMockMvc(WebRequestMatcher...)
+	 * @see #useMockMvcForHosts(String...)
+	 */
+	protected final WebConnection createConnection(WebClient webClient) {
+		Assert.notNull(webClient, "WebClient must not be null");
+		return createConnection(webClient, webClient.getWebConnection());
+	}
+
+	/**
+	 * Create a new {@link WebConnection} that will use a {@link MockMvc}
+	 * instance if one of the specified {@link WebRequestMatcher} instances
+	 * matches.
+	 * @param webClient the WebClient to use if none of
+	 * the specified {@code WebRequestMatcher} instances matches; never {@code null}
+	 * @param defaultConnection the WebConnection to use
+	 * @return a new {@code WebConnection} that will use a {@code MockMvc}
+	 * instance if one of the specified {@code WebRequestMatcher} matches
+	 * @see #alwaysUseMockMvc()
+	 * @see #useMockMvc(WebRequestMatcher...)
+	 * @see #useMockMvcForHosts(String...)
+	 */
+	private WebConnection createConnection(WebClient webClient, WebConnection defaultConnection) {
+		MockMvcWebConnection mockMvcWebConnection = new MockMvcWebConnection(this.mockMvc, webClient, this.contextPath);
 
 		if (this.alwaysUseMockMvc) {
 			return mockMvcWebConnection;
@@ -162,5 +199,4 @@ protected final WebConnection createConnection(WebConnection defaultConnection)
 
 		return new DelegatingWebConnection(defaultConnection, delegates);
 	}
-
 }

spring-test/src/main/java/org/springframework/test/web/servlet/htmlunit/webdriver/MockMvcHtmlUnitDriverBuilder.java

@@ -129,7 +129,7 @@ public MockMvcHtmlUnitDriverBuilder javascriptEnabled(boolean javascriptEnabled)
 	public MockMvcHtmlUnitDriverBuilder withDelegate(WebConnectionHtmlUnitDriver driver) {
 		Assert.notNull(driver, "HtmlUnitDriver must not be null");
 		driver.setJavascriptEnabled(this.javascriptEnabled);
-		driver.setWebConnection(createConnection(driver.getWebConnection()));
+		driver.setWebConnection(createConnection(driver.getWebClient()));
 		this.driver = driver;
 		return this;
 	}

spring-test/src/main/java/org/springframework/test/web/servlet/htmlunit/webdriver/WebConnectionHtmlUnitDriver.java

@@ -41,7 +41,6 @@ public class WebConnectionHtmlUnitDriver extends HtmlUnitDriver {
 
 	private WebClient webClient;
 
-
 	public WebConnectionHtmlUnitDriver(BrowserVersion browserVersion) {
 		super(browserVersion);
 	}
@@ -107,4 +106,11 @@ public void setWebConnection(WebConnection webConnection) {
 		this.webClient.setWebConnection(webConnection);
 	}
 
+	/**
+	 * Gets the current {@link WebClient}
+	 * @return the current {@link WebClient}
+	 */
+	public WebClient getWebClient() {
+		return this.webClient;
+	}
 }

spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/MockMvcConnectionBuilderSupportTests.java

@@ -36,6 +36,7 @@
 import org.springframework.web.context.WebApplicationContext;
 import org.springframework.web.servlet.config.annotation.EnableWebMvc;
 
+import com.gargoylesoftware.htmlunit.WebClient;
 import com.gargoylesoftware.htmlunit.WebConnection;
 import com.gargoylesoftware.htmlunit.WebRequest;
 import com.gargoylesoftware.htmlunit.WebResponse;
@@ -45,6 +46,7 @@
 import static org.hamcrest.Matchers.notNullValue;
 import static org.junit.Assert.assertThat;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 /**
  * Integration tests for {@link MockMvcWebConnectionBuilderSupport}.
@@ -55,10 +57,12 @@
 @RunWith(SpringJUnit4ClassRunner.class)
 @ContextConfiguration
 @WebAppConfiguration
-@SuppressWarnings("rawtypes")
+@SuppressWarnings({"rawtypes","deprecation"})
 public class MockMvcConnectionBuilderSupportTests {
 
-	private final WebConnection delegateConnection = mock(WebConnection.class);
+	private WebConnection delegateConnection;
+
+	private WebClient webClient;
 
 	@Autowired
 	private WebApplicationContext wac;
@@ -69,10 +73,16 @@ public class MockMvcConnectionBuilderSupportTests {
 
 	@Before
 	public void setup() {
+		delegateConnection = mock(WebConnection.class);
+
+		webClient = mock(WebClient.class);
+		when(webClient.getWebConnection()).thenReturn(delegateConnection);
+
+
 		mockMvc = MockMvcBuilders.webAppContextSetup(wac).build();
 
 		connection = new MockMvcWebConnectionBuilderSupport(mockMvc){}
-				.createConnection(delegateConnection);
+				.createConnection(webClient);
 	}
 
 	@Test(expected = IllegalArgumentException.class)
@@ -86,10 +96,61 @@ public void constructorContextNull() {
 	}
 
 	@Test
-	public void context() throws Exception {
+	public void contextDeprecated() throws Exception {
+		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
+				.createConnection(webClient);
+
+		assertMvcProcessed("http://localhost/");
+		assertDelegateProcessed("http://example.com/");
+	}
+
+	@Test
+	public void mockMvcDeprecated() throws Exception {
+		assertMvcProcessed("http://localhost/");
+		assertDelegateProcessed("http://example.com/");
+	}
+
+	@Test
+	public void mockMvcExampleDotComDeprecated() throws Exception {
+		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
+				.useMockMvcForHosts("example.com")
+				.createConnection(delegateConnection);
+
+		assertMvcProcessed("http://localhost/");
+		assertMvcProcessed("http://example.com/");
+		assertDelegateProcessed("http://other.com/");
+	}
+
+	@Test
+	public void mockMvcAlwaysUseMockMvcDeprecated() throws Exception {
+		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
+				.alwaysUseMockMvc()
+				.createConnection(delegateConnection);
+
+		assertMvcProcessed("http://other.com/");
+	}
+
+	@Test
+	public void defaultContextPathEmptyDeprecated() throws Exception {
 		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
 				.createConnection(delegateConnection);
 
+		assertThat(getWebResponse("http://localhost/abc").getContentAsString(), equalTo(""));
+	}
+
+	@Test
+	public void defaultContextPathCustomDeprecated() throws Exception {
+		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
+				.contextPath("/abc").createConnection(delegateConnection);
+
+		assertThat(getWebResponse("http://localhost/abc/def").getContentAsString(), equalTo("/abc"));
+	}
+
+	@Test
+	public void context() throws Exception {
+		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
+				.createConnection(webClient);
+
 		assertMvcProcessed("http://localhost/");
 		assertDelegateProcessed("http://example.com/");
 	}
@@ -104,7 +165,7 @@ public void mockMvc() throws Exception {
 	public void mockMvcExampleDotCom() throws Exception {
 		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
 				.useMockMvcForHosts("example.com")
-				.createConnection(delegateConnection);
+				.createConnection(webClient);
 
 		assertMvcProcessed("http://localhost/");
 		assertMvcProcessed("http://example.com/");
@@ -115,23 +176,23 @@ public void mockMvcExampleDotCom() throws Exception {
 	public void mockMvcAlwaysUseMockMvc() throws Exception {
 		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
 				.alwaysUseMockMvc()
-				.createConnection(delegateConnection);
+				.createConnection(webClient);
 
 		assertMvcProcessed("http://other.com/");
 	}
 
 	@Test
 	public void defaultContextPathEmpty() throws Exception {
 		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
-				.createConnection(delegateConnection);
+				.createConnection(webClient);
 
 		assertThat(getWebResponse("http://localhost/abc").getContentAsString(), equalTo(""));
 	}
 
 	@Test
 	public void defaultContextPathCustom() throws Exception {
 		connection = new MockMvcWebConnectionBuilderSupport(wac) {}
-				.contextPath("/abc").createConnection(delegateConnection);
+				.contextPath("/abc").createConnection(webClient);
 
 		assertThat(getWebResponse("http://localhost/abc/def").getContentAsString(), equalTo("/abc"));
 	}

spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebClientBuilderTests.java

@@ -34,6 +34,7 @@
 import org.springframework.test.web.servlet.setup.MockMvcBuilders;
 import org.springframework.tests.Assume;
 import org.springframework.tests.TestGroup;
+import org.springframework.web.bind.annotation.CookieValue;
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RestController;
 import org.springframework.web.context.WebApplicationContext;
@@ -42,6 +43,7 @@
 import com.gargoylesoftware.htmlunit.WebClient;
 import com.gargoylesoftware.htmlunit.WebRequest;
 import com.gargoylesoftware.htmlunit.WebResponse;
+import com.gargoylesoftware.htmlunit.util.Cookie;
 
 import static org.hamcrest.CoreMatchers.*;
 import static org.junit.Assert.*;
@@ -99,6 +101,17 @@ public void mockMvcSetupWithCustomWebClientDelegate() throws Exception {
 		Assume.group(TestGroup.PERFORMANCE, () -> assertDelegateProcessed("http://example.com/"));
 	}
 
+	// SPR-14066
+	@Test
+	public void cookieManagerShared() throws Exception {
+		this.mockMvc = MockMvcBuilders.standaloneSetup(new CookieController()).build();
+		this.webClient = mockMvcSetup(this.mockMvc).build();
+
+		assertThat(getWebResponse("http://localhost/").getContentAsString(), equalTo(""));
+		this.webClient.getCookieManager().addCookie(new Cookie("localhost", "cookie", "cookieManagerShared"));
+		assertThat(getWebResponse("http://localhost/").getContentAsString(), equalTo("cookieManagerShared"));
+	}
+
 	private void assertMvcProcessed(String url) throws Exception {
 		assertThat(getWebResponse(url).getContentAsString(), equalTo("mvc"));
 	}
@@ -126,4 +139,11 @@ public String contextPath(HttpServletRequest request) {
 		}
 	}
 
+	@RestController
+	static class CookieController {
+		@RequestMapping("/")
+		public String cookie(@CookieValue("cookie") String cookie) {
+			return cookie;
+		}
+	}
 }

spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebConnectionTests.java

@@ -36,6 +36,7 @@
  * @author Rob Winch
  * @since 4.2
  */
+@SuppressWarnings("deprecation")
 public class MockMvcWebConnectionTests {
 
 	private final WebClient webClient = new WebClient();
@@ -50,7 +51,7 @@ public void setup() {
 
 	@Test
 	public void contextPathNull() throws IOException {
-		this.webClient.setWebConnection(new MockMvcWebConnection(this.mockMvc, null));
+		this.webClient.setWebConnection(new MockMvcWebConnection(this.mockMvc, (String) null));
 
 		Page page = this.webClient.getPage("http://localhost/context/a");