swift-server / http

:warning: Historical HTTP API - please use https://github.com/swift-server/async-http-client instead
Apache License 2.0
703 stars 48 forks source link

Implemented #83 (HTTPHeaders accessors) #104

Closed amosavian closed 5 years ago

amosavian commented 6 years ago

This pull request includes static variables for HTTPVersion struct. Also implemented easy accessors for HTTPHeaders to strict types for header values. ~Authentication, Cookie and Set-Cookie accessors are not implemented yet due to complexity or designing limitations. I will make another PR when they became ready in near future.~

UPDATE: All methods are implemented now. Here is mailing list post: mailing list: https://lists.swift.org/pipermail/swift-server-dev/Week-of-Mon-20171113/000713.html

helje5 commented 6 years ago

May I also suggest that you take this to the mailing list for discussion. I think we already discussed this kind of stuff and decided not to do typed headers at this level of the framework. It would be part of a real framework on top of this driver.

If we really go the route of typed headers (which I personally kinda like for efficiency), we should actually do that in the storage itself, not just wrapping the String key/value. I wrote some email on that in the list.

amosavian commented 6 years ago

@helje5 please check new implementation of EntryTag.

amosavian commented 6 years ago

I don't know how to use mailing list unfortunately. I wonder if you could help me.

Headers are not something that you would access frequently. For requests, you may access them once or twice. For response, you set them once. Parsing all request headers when initializing is not an optimized behavior unless they got accessed (lazy). We may introduce some kind of caching later btw.

helje5 commented 6 years ago

https://lists.swift.org/mailman/listinfo/swift-server-dev

helje5 commented 6 years ago

You first need to subscribe to the list with the email address you want to use for sending messages to it. Once this is through, you don't need to wait for moderator approval.

amosavian commented 6 years ago

Posted in mailing list: https://lists.swift.org/pipermail/swift-server-dev/Week-of-Mon-20171113/000713.html

amosavian commented 6 years ago

@seabaylea @carlbrown Any chance to get merged? I must begin working on tests.

aleksey-mashanov commented 6 years ago

Great work! Typed headers are definitely the thing a good framework should have. But I have a sense that this too much for an API between an HTTP framework and an HTTP server. Maybe this should be made as a separate module?

I suggest to replace Int64 with Int to follow Swift guidelines:

Unless you need to work with a specific size of integer, always use Int for integer values in your code.

amosavian commented 6 years ago

@aleksey-mashanov Please find my response for Int64 here. HTTPURLResponse.expectedContentLength and URLSession methods return Int64. NSFileManager returns UInt64 for file size but Content-Length can have negative values. Furthermore it's common to have a file larger than 2GB on a 32-bit system.

Please find my explanation here on why we should implement typed header in this layer. RFC 5987 is a reason e.g.

aleksey-mashanov commented 6 years ago

Furthermore it's common to have a file larger than 2GB on a 32-bit system.

Agree. But 32bit server? We still have some in production for historical reasons but it is a pain. In almost every place in a surrounding code this value will be converted to size_t (Int) and used for generation of dynamic content. I really don't know what is better: precise API or comfortable API. For low-level of this HTTP API maybe first but this PR is out of scope of low-levelness.

RFC 5987 is a reason e.g.

Agree. For me RFC5987 and parsing of additional parameters (after ;) are the most interesting parts of this PR. Maybe they should be implemented lower in HTTPHeaders as an additional subscripts? There are a lot of RFCs, a lot of additional headers and even more will arise. It will be hard to follow all of them correctly and most of changes will be source compatibility breaking.

Again. This PR doesn't provide features which are required to efficiently pass data from a framework to a server and back. Nevertheless it is very useful.

amosavian commented 6 years ago

every place in a surrounding code this value will be converted to size_t (Int)

Swift Foundation's FileManager class works with file size as UInt64. In case we don't want typecast, it's better to consider UInt64 rather than Int. On the other hand, Swift compiler may give a warning if user tries to cast UInt64 to Int64 or Int.

Furthermore, range, contentRange and contentLength must have same type imo. range must support negative values.

There are a lot of RFCs, a lot of additional headers and even more will arise.

I mentioned RFC5987 just as an example because it's mentioned in source code as TODO. Definitely There are more cases which I'm not aware of.

PR doesn't provide features which are required to efficiently pass data from a framework to a server and back. Nevertheless it is very useful.

This design is implemented as an extension to current design. My primary goal was not to touch current architecture because any change in architecture usually needs extensive workgroup paperworks (PS: I'm not aware how swift-server workgroup works). But my design, which uses initializers for casting instead of functions (except parsing parameters and q values), gives the flexibility to change underlying design anytime.

amosavian commented 6 years ago

List of optional but unimplemented headers, which are nice to have in portfolio. I won't implement them as a part of this PR: Access-Control-*, Accept-Patch, Alt-Svc, Content-Security-Policy, DNT, Forwarded, Proxy-Authenticate, Proxy-Authorization, Retry-After, Tk, Warning, X-XSS-Protection

amosavian commented 6 years ago

This PR is completely implemented.

mohsen1 commented 5 years ago

FYI DOM Headers API allows setting multiple values for the same cookie and ignores the key casing:

var h = new Headers()
h.append('Cookie', 'foo bar')
h.append('cooKie', 'baz')
for (var e of h.entries()) { console.log(e) } 

prints:

["cookie", "foo bar, baz"]
ianpartridge commented 5 years ago

Closing as this project is historical, per the README.