MockMvcWebConnection should share CookieManager with HtmlUnit driver [SPR-14066]

[spring-projects-issues]

Christopher Smith opened SPR-14066 and commented

While working on the other side of #18623 (trying to explicitly set a cookie as a given and then loading a page, expecting the controller to see the cookie's value), I discovered that the MockMvcWebConnection used by the HtmlUnit driver maintains a completely separate copy of WebClient with its own state. This is presumably because the DelegatingWebConnection operates as a sort of decorator.

However, this means that any cookies set in the driver.manage().addCookie() (or equivalent) are simply missing from the context used to issue local MockMvc requests.

The connection object should, when creating the delegate WebClient, use the same CookieManager as the parent WebClient.

This code, inserted in setup(), appears to induce the expected behavior:

WebClient topLevelWebClient = driver.@webClient
CookieManager topLevelCookieManager = topLevelWebClient.cookieManager

(topLevelWebClient.webConnection as DelegatingWebConnection).@connections
    .findAll { DelegatingWebConnection.DelegateWebConnection connection ->
        connection.delegate instanceof MockMvcWebConnection
    }.each { (it.@delegate as MockMvcWebConnection).@webClient.cookieManager = topLevelCookieManager }

---

Affects: 4.2.5

Issue Links:

• MockMvcWebConnection should update cookie manager with MockHttpServletResponse cookies [SPR-14265] #18837 MockMvcWebConnection should update cookie manager with MockHttpServletResponse cookies

Referenced from: commits 411ff84, 4a6c2db, 7d96ad1

[spring-projects-issues]

Rossen Stoyanchev commented

Indeed it looks like MockMvcWebClientBuilder and MockMvcHtmlUnitDriverBuilder each configure the WebClient and HtmlUnitDriver they create with a MockMvcWebConnection but that in turn creates its own WebClient internally rather than getting a reference to the WebClient it was created for. I'm not sure why that is. The purpose of the DelegatingWebConnection seems to be for limiting the use of MockMvc to specific requests (e.g. hosts) so that seems unrelated.

Rob Winch any reason why MockMvcWebClientBuilder and MockMvcHtmlUnitDriverBuilder only inject a MockMvcWebConnection into the WebClient and the HtmlUnitDriver respectively but don't give the MockMvcWebConnection a reference back to the WebClient they were injected into? I can see a setter on MockMvcWebConnection but it's not being used.

[spring-projects-issues]

Rob Winch commented

Thanks for the report. This does look like a problem. I submitted Pull Request #1009 for review.

cc Sam Brannen Rossen Stoyanchev

[spring-projects-issues]

Christopher Smith commented

I presumed the clients and connections were different on purpose; it appears that the current consensus is that the connection should be shared. Why different clients, in that case, especially since the CookieManager belongs to the client?

[spring-projects-issues]

Rossen Stoyanchev commented

Christopher Smith I'm not sure what you mean by different clients. When MockMvcWebConnection is created it is immediately given a reference to the WebClient it was created for. So there is only 1 client instance.

[spring-projects-issues]

Sam Brannen commented

Rob Winch, thanks for putting together the PR so quickly!

It looks good to me (at least at a quick glance).

[spring-projects-issues]

Christopher Smith commented

Rossen Stoyanchev, that is definitely not the case. When debugging, I found that there were two client objects. Setting a breakpoint in a controller method, you can walk up the stack and inspect the state. The two "client" entries in the stack are separate instances.

[spring-projects-issues]

Rob Winch commented

Christopher Smith To be clear....are you talking about after the pull request (fix) was merged or prior to the pull request being merged?

[spring-projects-issues]

Christopher Smith commented

Current release, premerge, though if the PR changes that I missed it.

[spring-projects-issues]

Rob Winch commented

Christopher Smith I think everyone agrees that this is broken in the current release.

The PR does change this in the latest SNAPSHOT. It injects the WebClient into the MockMvcWebConnection. For example, MockMvcHtmlUnitDriverBuilder now injects the WebClient instance into the MockMvcWebConnection.

I added a test for WebClient and HtmlUnitDriver to ensure that the cookies are now shared (both tests failed prior to my changes).

Go ahead and give the latest SNAPSHOT a try and check to see that things are working for you. You can use the following to try out the changes:

<dependencies>
    <dependency>
        <groupId>org.springframework</groupId>
        <artifactId>spring-test</artifactId>
        <scope>test</scope>
        <version>4.3.0.BUILD-SNAPSHOT</version>
    </dependency>
</dependencies>
<repositories>
    <repository>
        <id>spring-snapshots</id>
        <name>Spring Snapshots</name>
        <url>https://repo.spring.io/libs-snapshot</url>
        <snapshots>
            <enabled>true</enabled>
        </snapshots>
    </repository>
</repositories>

If you are using Gradle, you can use:

dependencies {
    testCompile 'org.springframework:spring-test:4.3.0.BUILD-SNAPSHOT'
}
repositories {
    maven {
        url 'https://repo.spring.io/libs-snapshot'
    }
}

If you continue to have problems, please don't hesitate to let us know. If it resolves your issue, it would be nice to hear back from you as well. Thanks again for reporting this issue!

[spring-projects-issues]

Christopher Smith commented

I saw the new cookie assertions, which cover my exact use case, but did not see assertions that the clients are the same. This will fully fix the instant issue, but if the intent is for same-object clients, that still appears not to be enforced (I may be completely missing it, especially if it's implicit). I have no idea whether this could cause issues with, e.g., the AJAX controller, or whether this would be handled by the top-level client.