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

Add the ability to pass title case headers to an HTTP server #13968

Merged
merged 3 commits into from
Apr 22, 2020

Conversation

wonderix
Copy link
Contributor

@wonderix wonderix commented Apr 13, 2020

Some older HTTP servers (especially those used in embedded devices) expect title case HTTP headers. Normally HTTP headers should be case insensitive. To also support the communication to these HTTP servers, I added a flag titleCase to the HttpHeaders
This was also added as feature to other programming languages (e.g. rust)

upper = s[i] == '-'


proc newHttpHeaders*(titleCase: bool =false): HttpHeaders =
Copy link

Choose a reason for hiding this comment

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

you can just write "titleCase = false" here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Changed


proc newHttpHeaders*(keyValuePairs:
openArray[tuple[key: string, val: string]]): HttpHeaders =
openArray[tuple[key: string, val: string]],titleCase: bool =false): HttpHeaders =
Copy link

@ghost ghost Apr 13, 2020

Choose a reason for hiding this comment

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

same here, also add a space like "openArray[tuple[key: string, val: string]], titleCase"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Changed.

@ghost
Copy link

ghost commented Apr 13, 2020

I think like this indeed might be a useful feature, but it needs more thought on how to do it properly

@ghost
Copy link

ghost commented Apr 13, 2020

About your Rust example - see hyperium/hyper#1497, we only need to convert headers to TitleCase when actually sending them, not every time you try to assign keys or something like that.

@wonderix
Copy link
Contributor Author

I also thought about putting my changes in httpclient.nim. But I decided against it, because in this case the conversion has to be done twice. (key->toLowerAscii -> toTitleCase)

I also decided against simply replacing toLowerAscii by toTitleCase because this could be a breaking change.


proc newHttpHeaders*(keyValuePairs:
openArray[tuple[key: string, val: string]]): HttpHeaders =
openArray[tuple[key: string, val: string]], titleCase =false): HttpHeaders =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
openArray[tuple[key: string, val: string]], titleCase =false): HttpHeaders =
openArray[tuple[key: string, val: string]], titleCase=false): HttpHeaders =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a look at other files and it seems that normally a space is used as separator before and after =. Therefore I added a space after the =

@@ -100,16 +101,30 @@ const
const httpNewLine* = "\c\L"
const headerLimit* = 10_000

proc newHttpHeaders*(): HttpHeaders =
proc toTitleCase(s: string): string =
Copy link
Contributor

Choose a reason for hiding this comment

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

separate PR to add this into strutils would be nice :)

@@ -17,6 +17,7 @@ import tables, strutils, parseutils
type
HttpHeaders* = ref object
table*: TableRef[string, seq[string]]
convert: proc (str: string): string
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we really need this, it just adds extra complexity. Just store isTitleCase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this accordingly.

@Araq
Copy link
Member

Araq commented Apr 14, 2020

Or instead: You write HTTP servers that adhere to the spec and are not case sensitive...

@wonderix wonderix requested a review from dom96 April 14, 2020 09:15
@Araq Araq merged commit 5571d48 into nim-lang:devel Apr 22, 2020
wonderix added a commit to wonderix/Nim that referenced this pull request Jun 1, 2020
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.

3 participants