Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #567 - Allow duplicate cookies #756

Closed

Conversation

mushanga
Copy link

@mushanga mushanga commented Sep 16, 2017

Fixed #567

Issue
https://github.com/tomakehurst/wiremock/issues/567#issue-199911597

If I make a request with two values for one cookie, like this:
$ curl 'http://localhost:8080/test' -H 'Cookie: cookie1=190283aJDS;cookie1=190283aJDS'
I get the following response:

<title>Error 500 </title>

But if I remove the duplication, it works.

Reply
https://github.com/tomakehurst/wiremock/issues/567#issuecomment-272279919

Cookies are represented by a map internally and the map builder blows up if you try to add more than one with the same key.

Fix

  • Changes in Cookie:
    • String name property added
    • Json value representation replaced with object
  • Changes in interface Request: Map<String, Cookie> getCookies() method replaced with List<Cookie> getCookies()

@mushanga mushanga changed the title wiremock-567 Allow duplicate cookies #567 Allow duplicate cookies Sep 16, 2017
@mushanga mushanga closed this Sep 16, 2017
@mushanga mushanga reopened this Sep 16, 2017
@mushanga mushanga changed the title #567 Allow duplicate cookies Fixed #567 - Allow duplicate cookies Sep 21, 2017
@mushanga mushanga closed this Sep 26, 2017
@mushanga mushanga reopened this Sep 26, 2017
@mushanga
Copy link
Author

mushanga commented Oct 2, 2017

Is there a problem with openjdk7 tests? Seems that it's been failing since 7th of September

Please let me know if I should fix something in my pull request.

@tomakehurst
Copy link
Member

Yeah, sorry about this. It's not your PR - I'm having problems with the Travis CI build image at the moment. Some combination of openjdk7, Gradle 4 and Travis' Linux image. I'm going to try and migrate to Circle CI today or tomorrow.

@mushanga
Copy link
Author

mushanga commented Oct 2, 2017

Great, thanks! :)

@tomakehurst
Copy link
Member

OK, the build is fixed on master now. If you merge across, it should go green.

Also, please could you make a few amendments to this PR:

The switch from Map<String, Cookie> to List<Cookie> on the Request interface breaks backwards compatibility (since this is an API class), so please could you restore this. I suggest instead that you modify the Cookie class so that it can take one or many values. Cookie is also an API class, so I suggest that you maintain its current interface but add extra methods to cope with the many values case.

This brings me to my 2nd point, which is that the way multi-valued headers are handled is to present the value as an array rather than a string when JSON serialised. I think it would make a lot of sense for cookies to be consistent with this, and a Cookie class that can handle many values would make this a lot easier.

Lastly, please could you add an acceptance test case for response templating with multiple cookie values.

Thanks!

@tomakehurst
Copy link
Member

I've fixed this in 827e7cc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants