Cookies 1.0 api

[kvosper]

A PR to make improvements to the way that Styx handles cookies.
The main changes are:

• Use the Cookie and Set-Cookie headers as the sole source-of-truth instead of trying to keep a separate collection of cookies in the message object.
• Introduce a clear distinction between request cookies (key-value pairs set in Cookie header) and response cookies (individual Set-Cookie headers that have attributes like Domain and Path).

[mikkokar]

This should be using a yet-to-be-added addCookie method.

[mikkokar]

static import?

[mikkokar]

I would just say:

Adds cookies into the Cookie header.

What happens if the cookie name is already present?

[mikkokar]

I would also elaborate that:

Adds cookies into Cookie header, or modified existing cookie values.

[mikkokar]

I cannot see any unit tests for addCookies. Please add tests for addCookies(), or am I just missing something?

[mikkokar]

Please add unit tests for the following addTests scenarios:

1. add a new cookie when Cookie header is not present.
2. add a new cookie when Cookie header is already present.
3. modify a cookie value using addCookie when a Cookie header is present and the cookie is already set.

[kvosper]

Yes, there are missing tests. I will add tests for the new methods.

[mikkokar]

This is perhaps bit contrived example, but perhaps worth looking into @kvosper:

    @Test
    public void removesCookies2() {
        HttpRequest r1 = HttpRequest.get("/")
                .addCookies(requestCookie("x", "x1"))
                .removeCookies("x")
                .build();

        assertThat(r1.cookie("x"), is(Optional.empty()));
    }

This test would fail as follows:

java.lang.AssertionError: 
Expected: is <Optional.empty>
     but: was <Optional[x=x1]>

I'll find this counter-intuitive.

[dvlato]

I don´t think having to decode the cookies for each getCookies() is the best approach. Can´t we apply some memoization so once decoded a cookie list will be kept until we modify the Cookie header?

[dvlato]

I think we usually do a null check in these cases.

[dvlato]

We might want to get rid of the creation of the Temporary list (e.g., using Stream.concatenate() and passing in an Stream or Iterables.xx in Guava and an Iterator ).

Also, the fact that we'll override previously existing values is quite difficult to understand in this code (you add the new cookies first so they will be the ones picked up in the Collectors.toSet() in the following method). That´s a path hard to follow and it depends on a non-documented behaviour of Collectors.toSet(). Probably it is better to try to make this clear somehow.

EDIT: probably my next comments will contradict my first request.

[dvlato]

The RFC clearly states that there should be a single Cookie header. I think we can return an error in that case.

[kvosper]

We would need to return the error in the builder. The cookies method could be changed to assume only one Cookie header exists though.

[kvosper]

The treatment of duplicate cookies is part of the netty encoding logic - it is not something introduced by Styx.

[kvosper]

The treatment of duplicate cookies is part of the netty encoding logic - it is not something introduced by Styx.