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

Added scopes, make headers and status code public #410

Merged
merged 1 commit into from
May 24, 2019

Conversation

apocolipse
Copy link
Contributor

@apocolipse apocolipse commented May 13, 2019

  • Added the manifest, ontouchstart and dataText vars to Scopes
  • Made HttpResponse headers() and statusCode public, statusCode a computed variable.

These are just a few minor changes that I've made for a project I'm using Swifter for.
Making the response properties public is necessary to create custom middle-middle-wares (proper terminology? /shrug) like the ones I'm using below:

typealias HTTPMiddleware = ((HttpRequest) -> HttpResponse)
typealias ThrowableHTTPMiddleware = ((HttpRequest) throws -> HttpResponse)

let TryingRequestHandler: (@escaping ThrowableHTTPMiddleware) -> HTTPMiddleware = { requestHandler in
  return {
    do {
      return try requestHandler($0)
    } catch let error {
      return .badRequest(.text(error.localizedDescription))
    }
  }
}

let LoggingRequestHandler: (@escaping HTTPMiddleware) -> HTTPMiddleware = { requestHandler in
  return { request in
    let startTime = Date()
    let response = requestHandler(request)
    let stopTime = Date()
    defer {
      loggingQueue.async {
        let pinfo = ProcessInfo.processInfo
        let df = (DateFormatter())
        df.dateFormat = "MMM dd HH:mm:ss"
        
        let date = df.string(from: startTime)
        let host = Info.hostname
        let proc = pinfo.processName
        let pid  = pinfo.processIdentifier
        let addr = request.address ?? "127.0.0.1"
        let time = stopTime.timeIntervalSince(startTime)
        let meth = request.method
        let path = request.path
        let stat = response.statusCode
        let len  = response.headers()["Content-Length"] ?? "0"
        let org  = request.headers["origin"] ?? "\"-\""
        let hdrs = request.headers["user-agent"] ?? ""

        print("\(date) \(host) \(proc)[\(pid)]: \(addr) - - Time Taken:\(time)  \"\(meth) \(path) HTTP/1.1\" \(stat) \(len) \"\(org)\" \"\(hdrs)\"")
      }
    }
    return response
  }
}

These can be combined/chained to wrap around request handlers, and are extremely easy to add to normal request handling closures by dropping the name right in front, examples:

// Works with normal requests
server.GET["/refresh"] = LoggingRequestHandler { request in
  doRefreshStuff()
  return .ok(.text("OK"))
}

// Trying request Handler automatically responds to errors with a .badRequest wrapping localizedDescription, no need to do/catch!
server.GET["/info"] = TryingRequestHandler { request in
  return .ok(.text(try getInfoJson()))
}

// You can even chain them!  Keep in mind the TryingRequestHandler is the one that takes a throwing block so it should be inner, and it has to be "function like" so wrap the whole block in parens
server.GET["/somethingElse"] = LoggingRequestHandler( TryingReqeustHandler { request in
  try doOtherThing()
  return .ok(.text("OK"))
})

// Use like a function for shared files or scopes
server["js/compiled/:path"]  = LoggingRequestHandler(shareFilesFromDirectory("/path/to/resources"))
server.GET["/"] = LoggingRequestHandler(scopes { ... })

// Maybe even WebSockets?  IDK havent tested but the syntax works
server["/websocket-echo"] = LoggingRequestHandler(websocket(text: { session, text in
  session.writeText(text)
}, binary: { session, binary in
  session.writeBinary(binary)
}))

@swifter-bot
Copy link

swifter-bot commented May 13, 2019

2 Warnings
⚠️ It seems like you’ve added new tests to the library. If that’s the case, please update the XCTestManifests.swift file running in your terminal the command swift test --generate-linuxmain.
⚠️ If you ran the command swift test --generate-linuxmain in your terminal, please remove the line testCase(IOSafetyTests.__allTests__IOSafetyTests), from public func __allTests() -> [XCTestCaseEntry] in the bottom of the file. For more reference see #366.
1 Message
📖 Hey, @apocolipse 👋.

Generated by 🚫 Danger

@Vkt0r
Copy link
Member

Vkt0r commented May 13, 2019

Hey, @apocolipse Thanks for taking the time to create this PR 👍. The PR in general looks good just I just have a few comments about it:

  • The library exposes a Middleware (the docs don't reflect that at this point but it will soon). Examples of its use can be seen in the DemoServer.swift file.
server.middleware.append { request in
   print("Middleware: \(request.address ?? "unknown address") -> \(request.method) -> \(request.path)")
   return nil
}

In the above example, it's used to print all the request that comes through the server but it can be used for the same purpose you used above.

  • IMHO the use of computed read-only properties vs functions is a highly opiniated debate. So I'm wondering what's the cause you're converting thestatusCode to a computed read-only property and not the reasonPhrase too ?

  • Now you mention the access modifier in the HttpResponse I think all the functions should be public and we need to do it explicitly as Swift doesn't make the internal members public by default.

@apocolipse
Copy link
Contributor Author

apocolipse commented May 13, 2019

@Vkt0r When I looked at the middleware in the library i wasn't able to figure out an easy way to "wrap" the whole request handler (in order to get time taken to print), is there an easy way for that?

In any case the TryingRequestHandler example is something that probably should be added to the library (though I'm averse to default behavior that users can't change, so maybe just an example in the README? idk)

RE: statusCode, Didn't notice reasonPhrase since I didn't actually use it, but my personal philosophy is computed properties should be used for returning values that are fixed or stored anyway, functions for anything that actually must be "computed". For enums and values likestatusCode, the switch (in my opinion) can/should be ignored as "computing". Really anything that's relatively idempotent and low cost. (i.e. if i call rand() 10x, i'll get different results, but if i call resp.statusCode() 10 x, its never going to change, and its compute cost is always the same).
A really gray area in my opinion is responseHeaders(). In terms of how a Server API should feel, these should always be a property not a function, though as used here there is a decent bit of putting things together (not low computational cost), which is ultimately why I didn't change that. In the subjective terms of "feel", though, statusCode() just feels really wrong to me.

RE: Access levels, yeah its annoying but everything needs to be evaluated in a library for who's going to use it, basic stuff like headers, statusCode, body, etc. should be openly available

@Vkt0r
Copy link
Member

Vkt0r commented May 16, 2019

@apocolipse

When I looked at the middleware in the library I wasn't able to figure out an easy way to "wrap" the whole request handler (in order to get time taken to print), is there an easy way for that?

The current Middleware can be used exactly like you're using it right now for the case of measuring the time of the response but you need to be careful as it would be used for all the requests in case a response is given I'm afraid.

In any case the TryingRequestHandler example is something that probably should be added to the library (though I'm averse to default behavior that users can't change, so maybe just an example in the README? idk)

Yes, maybe it could be added in a way to handle errors for certain functions I can think in #407 🤔. But I think your point of docs is very good, we could add some example of this uses of Middlewares to the docs. I've been working in docs for the library so I will mention some of your examples there 👍.

A really gray area, in my opinion, is responseHeaders(). In terms of how a Server API should feel, these should always be a property, not a function.

Yes, that's true it would be more readable and easy to use if it would be a read-only computed property. It's true that the syntax responseHeaders()[""] is a little annoying.

Can you please update the entry in the CHANGELOG.md. I would happily merge your PR when this is done.

@apocolipse
Copy link
Contributor Author

@Vkt0r Updated CHANGELOG.md, also updated reasonPhrase to match the same reasoning behind changing statusCode, and made it public as well in case someone wants to use it.

@Vkt0r Vkt0r self-requested a review May 21, 2019 15:02
@Vkt0r
Copy link
Member

Vkt0r commented May 21, 2019

@apocolipse Thanks for your changes, this looks very good! Just a minor comment, I'm seeing a lot of extra spaces and extra lines added in several files, can you please take care of these?

@Vkt0r Vkt0r merged commit 0152210 into httpswift:stable May 24, 2019
tomieq pushed a commit to tomieq/swifterfork that referenced this pull request Apr 1, 2021
Added scopes, make headers and status code public
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