MockHttpServletRequest setCookies should join cookies to single Cookie header

[chas678]

Version

spring-test-5.1.6.RELEASE

Overview

setCookies on a MockHttpServletRequest sets multiple "Cookie" Headers. It should concatenate the cookie name/value pairs into a single "Cookie" header string joined with delimiter "; ".

MockHttpServletRequest setCookies javadoc has little detail on what behavior a user should expect. If the behavior deviates from rfc6265 syntax by design then could the documentation be amended to reflect that?

Test case exposing defect

    @Test
    public void cookiesRegressionIssue() {
        // potentially regression of https://github.com/spring-projects/spring-framework/issues/19790 ?
        // ** arrange
        MockHttpServletRequest request = new MockHttpServletRequest();
        Cookie cookie1 = new Cookie("foo", "bar");
        Cookie cookie2 = new Cookie("baz", "qux");

        // ** act
        // Javadoc gives no detail:
        // https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/mock/web/MockHttpServletRequest.html#setCookies-javax.servlet.http.Cookie...-
        request.setCookies(cookie1, cookie2);

        // ** assert
        Cookie[] cookies = request.getCookies();
        assertAll("Cookies>Headers conversion should work",
                () -> assertThat(cookies.length, is(2)),
                () -> assertThat(cookies[0].getName(), is("foo")),
                () -> assertThat(cookies[0].getValue(), is("bar")),
                () -> assertThat(cookies[1].getName(), is("baz")),
                () -> assertThat(cookies[1].getValue(), is("qux")),
                () -> assertThat(request.getHeader("Cookie"), is("foo=bar; baz=qux")), 
                () -> assertThat(Collections.list(request.getHeaders("Cookie")), hasSize(1))
        );
    }

[sbrannen]

Thanks for raising the issue.

I believe your interpretation of the RFC is correct: MockHttpServletRequest.setCookies(...) should only create a single Cookie header.

In addition, we should also add Javadoc to that method.

Related issues:

• Update Cookie headers in MockHttpServletRequest and Response [SPR-15225] #19790
• Update Cookie headers in MockServerHttpRequest and Response [SPR-15522] #20081

[sbrannen]

Tentatively slated for 5.2 M3 as an enhancement

[sbrannen]

Updated test case using AssertJ so that it is compatible with master.

@Test
public void cookiesRegressionIssue() {
	MockHttpServletRequest request = new MockHttpServletRequest();
	Cookie cookie1 = new Cookie("foo", "bar");
	Cookie cookie2 = new Cookie("baz", "qux");
	request.setCookies(cookie1, cookie2);

	Cookie[] cookies = request.getCookies();

	assertAll("Cookies>Headers conversion should work", //
		() -> assertThat(cookies.length).isEqualTo(2), //
		() -> assertThat(cookies[0].getName()).isEqualTo("foo"),
		() -> assertThat(cookies[0].getValue()).isEqualTo("bar"),
		() -> assertThat(cookies[1].getName()).isEqualTo("baz"),
		() -> assertThat(cookies[1].getValue()).isEqualTo("qux"),
		() -> assertThat(request.getHeader("Cookie")).isEqualTo("foo=bar; baz=qux"),
		() -> assertThat(Collections.list(request.getHeaders("Cookie"))).size().isEqualTo(1));
}

[rstoyanchev]

What do Tomcat or Jetty do for comparison's sake? If there is any consensus there then I would align with that.

[sbrannen]

What do Tomcat or Jetty do for comparison's sake? If there is any consensus there then I would align with that.

I glanced at their code bases, and it appears that they both accept one or more Cookie headers for incoming requests.

But since this issue is about MockHttpServletRequest, I suppose we'd be better off investigating what browsers or clients actually send to servers.

@chas678, do you have a particular use case that requires a single Cookie header in the request? In other words, is there a reason you cannot use javax.servlet.http.HttpServletRequest.getCookies() which abstracts over the actual headers present?

[chas678]

@sbrannen - Use case was I was having to extract and manipulate a cookie value. The code was using getCookies, but the test was using the header and failing as was checking/expocting multiple values after manipulation. It took me a while to work out there were multiple Cookie header entries and I was a little surprised. I now loop over the getCookies array in test also so not a blocker.

[sbrannen]

I now loop over the getCookies array in test also so not a blocker.

OK. Thanks for the feedback.

[rstoyanchev]

we'd be better off investigating what browsers or clients actually send to servers.

Right, and I think I misread the issue. It looks like a bug indeed. Here is for comparison Netty and Jetty clients which do create a single value.

[sbrannen]

Right, and I think I misread the issue. It looks like a bug indeed. Here is for comparison Netty and Jetty clients which do create a single value.

OK. I'll change it to a "bug", and I'll review the PR submitted in #23074.

[sbrannen]

Superseded by #23074