Skip to content
This repository has been archived by the owner on Nov 23, 2021. It is now read-only.

Proposal: Some suggestions for better, more readble API #83

Open
amosavian opened this issue Nov 3, 2017 · 8 comments
Open

Proposal: Some suggestions for better, more readble API #83

amosavian opened this issue Nov 3, 2017 · 8 comments

Comments

@amosavian
Copy link

amosavian commented Nov 3, 2017 •
edited
Loading

I don't know where I can submit proposals for api changes so I opened this issue.

HTTPServer port

server port is determined in start(port:, handler:) method, while I think a server can start once with a one specified port. I could find a port property in HTTPServer class while with this design it have to be ports array. I propose relocating port argument from start() to class init() method.

HTTPResponse write methods

Personally I prefer something like response.header.write() instead of response.writeHeader(), response.body.write() instead of response.writeBody() and response.trailer.write() instead of response.writeTrailer(). Also we can keep room to extend header, body and trailer properties later.

HTTP version static variables

HTTP versions are limited, thus having HTTPVersion.v1_1 and HTTPVersion.v2_0 static vars might be fair.

A more complicated HTTPHeaders

HTTP headers are simply defined as a pair of strings. But for many of headers there have a set of defined values or a structured value. This implementation is prone to typo and human errors. So I propose to have some setters for frequent ones.

Implementation detail
// To set 'Range'
let range = 100... // half open or closed
headers.set(range: range) // result: 'Range: bytes 100-'

// To set 'Accept-Encoding'
headers.set(acceptEncodings: [.deflate, .gzip, .brotli, .identity])
// or
headers.set(acceptEncoding: .deflate, quality: 1.0)
headers.add(acceptEncoding: .gzip, quality: 0.8) // Note add instead of set
headers.add(acceptEncoding: .identity, quality: 0.5)
// values are defined in a enum/struct

// To set 'Cookie'
let cookie = HTTPCookie(properties: [.name : "name", .expires: Date(timeIntervalSinceNow: 3600)])
headers.set(cookie: cookie)
...
headers.add(cookie: cookie2)

// To set 'Accept-Language'
let locale = Locale.current
headers.set(acceptLanguages: [locale])
// also 'add(acceptLanguage:)' method to add progressively like Accept-Encoding

// To set 'Accept-Charset'
headers.set(acceptCharset: String.Encoding.utf8)

For Content-Type, we would define this struct in HTTPHeaders (should be extended with more types):

extension HTTPHeaders {
    struct MIMEType: RawRepresentable {
        public var rawValue: String
        public typealias RawValue = String
        
        public init(rawValue: String) {
            self.rawValue = rawValue
        }
        
        static let javascript = MIMEType(rawValue: "application/javascript")
        static let json = MIMEType(rawValue: "application/json")
        static let pdf = MIMEType(rawValue: "application/pdf")
        static let stream = MIMEType(rawValue: "application/octet-stream")
        static let zip = MIMEType(rawValue: "application/zip")
        
        // Texts
        static let css = MIMEType(rawValue: "text/css")
        static let html = MIMEType(rawValue: "text/html")
        static let plainText = MIMEType(rawValue: "text/plain")
        static let xml = MIMEType(rawValue: "text/xml")
        
        // Images
        static let gif = MIMEType(rawValue: "image/gif")
        static let jpeg = MIMEType(rawValue: "image/jpeg")
        static let png = MIMEType(rawValue: "image/png")
    }
}

And then we can set MIMEs this way:

headers.set(accept: .plainText)
headers.set(contentType: .json, encoding: .utf8)
// method converts 'NSStringEncoding' to IANA by 'CFStringConvertEncodingToIANACharSetName()' method

// Also similar approach for 'Pragma', 'TE' , etc...
headers.set(pragma: .noCache)
headers.set(transferEncodings: [.trailers, .deflate])

For some headers that accept date or integer or url, we implement methods accordingly:

// to set 'Content-Length'
headers.set(contentLength: 100)

// to set 'Date', 'If-Modified-Since', etc...
let fileModifiedDate = file.modifiedDate // or Date(timeIntervalSinceReferenceDate: 1000000)
headers.set(ifModifiedSince: fileModifiedDate)

// to set 'Referer' or 'Origin'
headers.set(referer: URL(string: "https://www.apple.com")!)

About Authorization header, we can have something like that, though I think it should be more refined and must be evaluated either we need them or not:

headers.set(authorizationType: .basic, user: "example", password: "pass")

Obviously, we must have some methods to fetch values. For example headers.contentLength will return a Int64? value, if it's defined by user, or headers.cookies will return a [HTTPCookie] array, headers.contentType will return a HTTPHeaders.MIMEType? optional, and so on and so forth.

I hope I can participate in implementing some parts if proposals are accepted

Best wish
amosavian added a commit to amosavian/http that referenced this issue Nov 16, 2017
@amosavian
Partialy implemented swift-server#83 (easy accessors for HTTPHeaders)
95133e9
@DeFrenZ
Copy link

DeFrenZ commented Nov 18, 2017

About the content type, I have this code from 2+ years ago (Swift 1.x or 2) for having it more statically defined. I don't suggest using this directly, as performance is probably not great and names could be more fitting, but I remember having a look at the specs to make this so at least the structure should be good 👍

private let permittedMIMECharacters: NSCharacterSet = .alphanumericCharacterSet()
private let trimmedMIMECharacters: NSCharacterSet = .whitespaceAndNewlineCharacterSet()
public struct MIMEContentType {
	public enum MIMEType: String {
		case Application = "application"
		case Audio = "audio"
		case Example = "example"
		case Image = "image"
		case Message = "message"
		case Model = "model"
		case MultiPart = "multipart"
		case Text = "text"
		case Video = "video"
	}
	public enum RegistrationTree: String {
		case Standards = ""
		case Vendor = "vnd."
		case Personal = "prs."
		case Unregistered = "x."
		
		static func splitStringWithRegistrationTree(string: String) -> (RegistrationTree, String) {
			if let dotIndex = string.characters.indexOf(".") {
				let treeString = string[string.startIndex ... dotIndex]
				if let tree = RegistrationTree(rawValue: treeString) {
					return (tree, string[dotIndex.successor() ..< string.endIndex])
				}
			}
			return (.Standards, string)
		}
	}
	public enum Suffix: String {
		case XML = "xml"
		case JSON = "json"
		case BER = "ber"
		case DER = "der"
		case FastInfoSet = "fastinfoset"
		case WBXML = "wbxml"
		case Zip = "zip"
		case CBOR = "cbor"
	}
	
	public let type: MIMEType
	public let tree: RegistrationTree
	public let subtype: String
	public let suffix: Suffix?
	public let parameters: [String: String]
	
	public init(type: MIMEType, tree: RegistrationTree = .Standards, subtype: String, suffix: Suffix? = nil, parameters: [String: String] = [:]) {
		self.type = type
		self.tree = tree
		self.subtype = subtype
		self.suffix = suffix
		self.parameters = parameters
	}
	
	public init?(contentType: String) {
		let trimmedContentType = contentType.stringByTrimmingCharactersInSet(trimmedMIMECharacters)
		
		let colonSeparatedComponents = trimmedContentType.componentsSeparatedByString(";").map({ $0.stringByTrimmingCharactersInSet(trimmedMIMECharacters) })
		let	(fullTypeString, parametersStrings) = (colonSeparatedComponents.first!, colonSeparatedComponents.dropFirst())
		
		let slashSeparatedComponents = fullTypeString.componentsSeparatedByString("/")
		if slashSeparatedComponents.count != 2 { return nil }
		
		let (typeString, fullSubtypeString) = (slashSeparatedComponents.first!, slashSeparatedComponents.last!)
		if let type = MIMEType(rawValue: typeString) {
			self.type = type
		} else { return nil }
		
		let (tree, afterTreeSubtypeString) = RegistrationTree.splitStringWithRegistrationTree(fullSubtypeString)
		self.tree = tree
		
		let suffixSeparatedComponents = afterTreeSubtypeString.componentsSeparatedByString("+")
		if suffixSeparatedComponents.count > 2 { return nil }
		let (subtypeString, suffixString) = (suffixSeparatedComponents.first!, suffixSeparatedComponents.count > 1 ? suffixSeparatedComponents.last : nil)
		self.subtype = subtypeString
		self.suffix = suffixString.flatMap({ Suffix(rawValue: $0)})
		
		let parametersStringsPairs = Array(parametersStrings).flatMap({ (parameterString: String) -> (String, String)? in
			let parameterComponents = parameterString.componentsSeparatedByString("=")
			if parameterComponents.count != 2 { return nil }
			return (parameterComponents.first!, parameterComponents.last!)
		})
		self.parameters = [String: String](parametersStringsPairs)
	}
	public var string: String {
		return "\(type.rawValue)/\(tree.rawValue)\(subtype)" + ((suffix?.rawValue).map({ "+\($0)" }) ?? "") + parameters.map({ "; \($0)=\($1)" }).joinWithSeparator("")
	}
}

@amosavian
Copy link
Author

amosavian commented Nov 19, 2017 via email

@DeFrenZ
Copy link

DeFrenZ commented Nov 19, 2017

First I don't know how a server can use parts of MIME.

It's not a likely need, but I can think of a few examples:

  • Using a third party library/another service for all image/ types, so checking only on that before forwarding
  • Reading the charset parameter to use different String decodings

"Accept" and "Content-Type" should be "exactly" same I'm afraid.

Agree. Naming it MIMEContentType was just because when I wrote that I needed it only for that header field, and I didn't know better.

Second, Parsing from/to string is a time-taking procedure for this struct, which makes it unsuitable for a server with hundreds of request per second

I'm not completely certain on this one. While I agree that it's probably an unnecessary cost to just structure the field for any incoming request, whenever the server is interested in comparing only a subset of it then it ultimately does the same computation: skip the type + compare the subtype. It actually saves comparing until the /! (Though I think that enumerating and comparing are very similar in performance terms)

Your proposal is certainly working and stronger typed than just using String, but I feel like other types in this project try to expose the rules around it as better as possible (see status codes and methods).

I’m also not too convinced about defining the most common constants by "hiding" the rest, e.g. json. I know that's the grand majority of JSON usage, but there are other things which might seem related. Even at the cost of verbosity, I think .applicationJSON or .application_json would be better.

Also I think ExpressibleByStringLiteral is a good idea in your example ;)

@amosavian
Copy link
Author

amosavian commented Nov 19, 2017 via email

@helje5
Copy link
Contributor

helje5 commented Nov 19, 2017 •
edited
Loading

Second, Parsing from/to string is a time-taking procedure for this struct, which makes it unsuitable for a server with hundreds of request per second

I'm not completely certain on this one. While I agree that it's probably an unnecessary cost to just structure the field for any incoming request

If we are concerned about performance, we should not use Swift strings for headers in the first place. Very few HTTP headers allow or cary Unicode strings, and Swift Strings are very complex objects. Everything about them is pretty expensive, creating them (lots of copying here, no interning), working them, etc.
Note that I'm not saying they are slow in a general sense, or maybe not even too slow for the use case, just that they are very heavyweight objects for identifying a recurring "Content-Length" header name and a '1343' value. In a scenario where Unicode is rarely used.

Wrt the parsing. Parsing the byte buffers at the HTTP parser level (which has to look at every byte anyways) and before creating real objects is very likely cheaper than doing unicode parsing on unicode strings.

Instead of creating 3(+!) objects and a copy tons of buffer:

header = Header(String(cString: header), String(cString: value))

create a single object (e.g. an enum, or class) and parse ASCII byte buffers:

header = .ContentLength(atoi(value))

Also remember that this is intended as a base library for higher level frameworks. Those will have different abstractions and APIs on how to access those things.

@amosavian
Copy link
Author

There was a discussion here about String performance.

@helje5 Our discussion was about cost/benefit of ContentType parsing overhead, not how to get better performance.
Of course parsing MIME into tree would provide more details, but I doubt it will had any benefit for a server which must compare Accept header value with predefined ones character by character.

@helje5
Copy link
Contributor

helje5 commented Nov 19, 2017 •
edited
Loading

Sorry if I expressed my point poorly. My specific point wasn't about String performance. If you need what String provides (e.g. because you are dealing with a inherently-Unicode document, like XML), it may be fast (or not - no idea, but I assume Swift will get better in every iteration). But in HTTP/1 we are dealing with a plain ASCII (more exactly Latin 1) protocol, not Unicode.
While some headers may have a proper Unicode meaning, such are really rare. (actually I'm not sure what HTTP actually header does, I think none)

@helje5 Our discussion was about cost/benefit of ContentType parsing overhead, not how to get better performance.

This statement contradicts itself.

Not sure what you mean by "parsing MIME into tree". Do we want to parse MIME multiparts? That would be very useful but belongs into a separate library for sure. I think in here we really just want the flat HTTP subset of MIME, if at all.

The parsing of low-level objects into very high level objects like Strings, could still be done on demand. I'm talking about the low level parsing. Accept is actually a really good example for this. It makes no sense to turn it into a Array<Enum<String|*>>, that is just utter waste. Primarily because the header is usually just checked a single time.
An ideal backing of the Accept "value type" would IMO be the underlying buffer with maybe ptrs into the separators. But probably just the buffer as for many HTTP header value types. Plus a matches function (which by default works on C strings or UnsafeBufferPtr, but can obviously provided convenience wrappers for higher level stuff).

But again: All this has been discussed on list. Please just check the archives, it makes no sense to re-raise the discussion again. I think the conclusion was: we do [(String:String)]. You can re-raise it on the list (ideally with references to the past discussion and why you think this was wrong).

@amosavian
Copy link
Author

@DeFrenZ you were right about design. I've changed it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@DeFrenZ @amosavian @helje5