Make Result#withCookies override existing cookies with the same name #7177
Conversation
| .withCookies(new Http.Cookie("bar", "Mars", 1000, "/", "example.com", false, true, null)) | ||
| } | ||
| }) { response => | ||
| response.allHeaders("Set-Cookie") must contain((s: String) => s.startsWith("bar=Mars;")) |
There was a problem hiding this comment.
would this test have failed without this change? i.e. should we also check that it doesn't contain bar=KitKat?
There was a problem hiding this comment.
You're right. This is testing the cookies after they are serialized, so there would be no difference. I can add another test for the Result object.
| @@ -290,8 +292,12 @@ public Cookie get(String name) { | |||
| * @return the transformed copy. | |||
| */ | |||
| public Result withCookies(Cookie... newCookies) { | |||
| List<Cookie> finalCookies = new ArrayList<>(cookies); | |||
| finalCookies.addAll(Arrays.asList(newCookies)); | |||
| List<Cookie> finalCookies = Stream.concat(cookies.stream().filter(cookie -> { | |||
There was a problem hiding this comment.
this really makes me think that cookies should be a map instead of a list. that seems like it'd be easier to use for end users as well since you're always going to want to be getting a cookie by name
There was a problem hiding this comment.
Http.Cookies already allows getting a cookie by name. This API in Result is for adding new cookies. I guess we could use a map as the underlying data structure but it would require more changes. Obviously this is not the most efficient algorithm, but the expectation is that most people will have no more than a few cookies.
There was a problem hiding this comment.
You can also do result.cookies().get("name")
Fixes #7174. Basically, this makes sure if we add new cookies, any existing cookies with the same name are removed from the list.