Skip to content

Loading…

Do not eliminate duplicate Set-Cookie headers. #199

Closed
wants to merge 1 commit into from

3 participants

@bfrog

This fixes issue #195 and allows multiple response Set-Cookie headers using set_resp_cookie

@essen essen commented on the diff
src/cowboy_http_req.erl
@@ -835,6 +835,8 @@ response_head(Status, Headers, RespHeaders, DefaultHeaders) ->
-> cowboy_http:headers().
merge_headers(Headers, []) ->
Headers;
+merge_headers(Headers, [{<<"Set-Cookie">>=Name, Value}|Tail]) ->
+ merge_headers(Headers ++ [{Name, Value}], Tail);
@essen Nine Nines member
essen added a note

Hm do we need the ++ here?

@bfrog
bfrog added a note

Its true of the line below that existed before this, I asked about it before and you simply said there was a reason but you couldn't remember.

I can make another patch if you accept this one that tries to remove the ++'s in merge_headers and see if that causes problems for anyone.

@essen Nine Nines member
essen added a note

I have no idea! Will look sometimes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@essen
Nine Nines member

Can we have a ct test for this? Also, does it work with 'Set-Cookie'? Thanks.

@bfrog

Do we allow 'Set-Cookie' in place of <<"Set-Cookie">> currently? set_resp_cookie uses <<"Set-Cookie">>

@essen
Nine Nines member

Cookies can be set manually. Didn't look if at this point it'd be converted to binary yet, it's just something to check.

@essen
Nine Nines member

At the point where you are doing it everything should be in binary.

@essen
Nine Nines member

Just needs a test for this. Would be good to have before 0.6. :)

@josevalim josevalim added a commit that referenced this pull request
@josevalim josevalim Do not remove duplicated Set-Cookie entries
This commit closes #195, closes #199, closes #246.
9782c6e
@josevalim

This can be closed in favor of the pull request #247.

@essen
Nine Nines member

Thanks guys.

@essen essen closed this
@josevalim josevalim added a commit that referenced this pull request
@josevalim josevalim Do not remove duplicated Set-Cookie entries
This commit closes #195, closes #199, closes #246.
0d0b962
@bfrog bfrog added a commit to treetopllc/cowboy that referenced this pull request
@josevalim josevalim Do not remove duplicated Set-Cookie entries
This commit closes #195, closes #199, closes #246.
8a6daf0
@bfrog bfrog added a commit to treetopllc/cowboy that referenced this pull request
@josevalim josevalim Do not remove duplicated Set-Cookie entries
This commit closes #195, closes #199, closes #246.
7028577
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 16, 2012
  1. @bfrog
Showing with 2 additions and 0 deletions.
  1. +2 −0 src/cowboy_http_req.erl
View
2 src/cowboy_http_req.erl
@@ -835,6 +835,8 @@ response_head(Status, Headers, RespHeaders, DefaultHeaders) ->
-> cowboy_http:headers().
merge_headers(Headers, []) ->
Headers;
+merge_headers(Headers, [{<<"Set-Cookie">>=Name, Value}|Tail]) ->
+ merge_headers(Headers ++ [{Name, Value}], Tail);
@essen Nine Nines member
essen added a note

Hm do we need the ++ here?

@bfrog
bfrog added a note

Its true of the line below that existed before this, I asked about it before and you simply said there was a reason but you couldn't remember.

I can make another patch if you accept this one that tries to remove the ++'s in merge_headers and see if that causes problems for anyone.

@essen Nine Nines member
essen added a note

I have no idea! Will look sometimes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
merge_headers(Headers, [{Name, Value}|Tail]) ->
Headers2 = case lists:keymember(Name, 1, Headers) of
true -> Headers;
Something went wrong with that request. Please try again.