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.

Sign up for GitHub

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

Jump to bottom

fix #224 Unable to set multiple cookies in a single web request #237

Closed
wants to merge 4 commits into from
Closed

fix #224 Unable to set multiple cookies in a single web request #237

wants to merge 4 commits into from

Conversation

itsumura-h
Copy link
Contributor

fix #224 Unable to set multiple cookies in a single web request
multiple cookies should be allowed

@itsumura-h
fix #224
eb54d47 
multiple cookies should be allowed
@itsumura-h itsumura-h changed the title fix #224 fix #224 Unable to set multiple cookies in a single web request Jan 30, 2020
dom96
dom96 reviewed Feb 7, 2020
View reviewed changes
jester.nim Outdated
h[i][1] = value
headers = some(h)
break outer
if key != "Set-cookie": # multiple cookies should be allowed
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it makes sense to change semantics just for Set-Cookie, would it make sense to allow multiple header values for all keys?

Copy link
Contributor Author

@itsumura-h itsumura-h Feb 8, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dom96
Seeing existing code, I thought your design aims for not allowing multiple header for same key. However at least Set-cookie should allow multiple header.
Would it be better to allow multiple header for any key?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just allow multiple headers for every key and accept the fact that HTTP is just a bunch of silly bytes with no underlying design.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itsumura-h I think so, I'd rather be consistent here.

@itsumura-h
Copy link
Contributor Author

@dom96
I updated to allow multiple header whatever key is

dom96
dom96 requested changes Apr 13, 2020
View reviewed changes
Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to ask for changes again, but I think we need another template for this called addHeader. setHeader implies that values are overwritten.

@itsumura-h
update #224
1135b1f 
new value is add to existing value
@itsumura-h
Copy link
Contributor Author

itsumura-h commented Apr 13, 2020 •
edited

@dom96
It works like this

import jester, httpcore

routes:
  get "/":
    let headers = @[
      ("aaa", "111"),
      ("aaa", "222"),
      ("aaa", "333"),
      ("Set-cookie", "aaa"),
      ("Set-cookie", "bbb"),
    ]
    resp Http200, headers, "test"

response header

aaa: 111, 222, 333
Content-Length: 4
Date: Mon, 13 Apr 2020 22:10:36 GMT
Server: HttpBeast
Set-cookie: aaa
Set-cookie: bbb

@dom96
Copy link
Owner

dom96 commented Apr 17, 2020

I still don't want the behaviour of setHeader to change. If you want resp to work like this then make it call a new addHeader template.

@itsumura-h itsumura-h closed this Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

@dom96 dom96 dom96 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to set multiple cookies in a single web request
3 participants
@itsumura-h @dom96 @Araq