diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml new file mode 100644 index 0000000..d97f78c --- /dev/null +++ b/.github/workflows/main.yml @@ -0,0 +1,33 @@ +name: tests +on: + push: + pull_request: + branches: [master] + schedule: + - cron: '0 0 * * 1' + +jobs: + tests: + runs-on: ${{ matrix.os }} + strategy: + matrix: + nimversion: + - stable + - devel + os: + - ubuntu-latest + - macOS-latest + - windows-latest + steps: + - uses: actions/checkout@v1 + with: + submodules: true + - uses: iffy/install-nim@v1.1 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + with: + nimversion: ${{ matrix.nimversion }} + - name: Test + run: | + nimble test + nimble refresh diff --git a/changelog.markdown b/changelog.markdown index 060c8e6..5d5fb5f 100644 --- a/changelog.markdown +++ b/changelog.markdown @@ -1,5 +1,9 @@ # Jester changelog +## x.x.x + +Fix a bug that prevented redirecting from within error handlers ([#269]([#269](https://github.com/dom96/jester/issues/269))) + ## 0.4.3 - 12/08/2019 Minor release correcting a few packaging issues and includes some other diff --git a/jester.nim b/jester.nim index 536335a..6aaf2fd 100644 --- a/jester.nim +++ b/jester.nim @@ -42,6 +42,14 @@ type request: Request, error: RouteError ): Future[ResponseData] {.gcsafe, closure.} + MatchPair* = tuple + matcher: MatchProc + errorHandler: ErrorProc + + MatchPairSync* = tuple + matcher: MatchProcSync + errorHandler: ErrorProc + Jester* = object when not useHttpBeast: httpServer*: AsyncHttpServer @@ -53,10 +61,11 @@ type MRegex, MSpecial, MStatic RawHeaders* = seq[tuple[key, val: string]] + ResponseHeaders* = Option[RawHeaders] ResponseData* = tuple[ action: CallbackAction, code: HttpCode, - headers: Option[RawHeaders], + headers: ResponseHeaders, content: string, matched: bool ] @@ -344,17 +353,18 @@ proc handleRequestSlow( var respData: ResponseData # httpReq.send(Http200, "Hello, World!", "") - try: - when respDataFut is Future[ResponseData]: - respData = await respDataFut + when respDataFut is Future[ResponseData]: + yield respDataFut + if respDataFut.failed: + # Handle any errors by showing them in the browser. + # TODO: Improve the look of this. + let exc = respDataFut.readError() + respData = await dispatchError(jes, req, initRouteError(exc)) + dispatchedError = true else: - respData = respDataFut - except: - # Handle any errors by showing them in the browser. - # TODO: Improve the look of this. - let exc = getCurrentException() - respData = await dispatchError(jes, req, initRouteError(exc)) - dispatchedError = true + respData = respDataFut.read() + else: + respData = respDataFut # TODO: Put this in a custom matcher? if not respData.matched: @@ -449,18 +459,20 @@ proc initJester*( result.errorHandlers = @[] proc initJester*( - matcher: MatchProc, + pair: MatchPair, settings: Settings = newSettings() ): Jester = result = initJester(settings) - result.register(matcher) + result.register(pair.matcher) + result.register(pair.errorHandler) proc initJester*( - matcher: MatchProcSync, # TODO: Annoying nim bug: `MatchProc | MatchProcSync` doesn't work. + pair: MatchPairSync, # TODO: Annoying nim bug: `MatchPair | MatchPairSync` doesn't work. settings: Settings = newSettings() ): Jester = result = initJester(settings) - result.register(matcher) + result.register(pair.matcher) + result.register(pair.errorHandler) proc serve*( self: var Jester @@ -510,7 +522,7 @@ proc serve*( asyncCheck serveFut runForever() -template setHeader(headers: var Option[RawHeaders], key, value: string): typed = +template setHeader(headers: var ResponseHeaders, key, value: string): typed = bind isNone if isNone(headers): headers = some(@({key: value})) @@ -576,8 +588,9 @@ template resp*(code: HttpCode): typed = result.matched = true break route -template redirect*(url: string): typed = +template redirect*(url: string, halt = true): typed = ## Redirects to ``url``. Returns from this request handler immediately. + ## If ``halt`` is true, skips executing future handlers, too. ## Any set response headers are preserved for this request. bind TCActionSend, newHttpHeaders result[0] = TCActionSend @@ -585,7 +598,10 @@ template redirect*(url: string): typed = setHeader(result[2], "Location", url) result[3] = "" result.matched = true - break route + if halt: + break allRoutes + else: + break route template pass*(): typed = ## Skips this request handler. @@ -711,10 +727,24 @@ template uri*(address = "", absolute = true, addScriptName = true): untyped = ## Convenience template which can be used in a route. request.makeUri(address, absolute, addScriptName) +template responseHeaders*(): var ResponseHeaders = + ## Access the Option[RawHeaders] response headers + result[2] + proc daysForward*(days: int): DateTime = ## Returns a DateTime object referring to the current time plus ``days``. return getTime().utc + initTimeInterval(days = days) +template setCookie*(headersOpt: var ResponseHeaders, name, value: string, expires="", + sameSite: SameSite=Lax, secure = false, + httpOnly = false, domain = "", path = "") = + let newCookie = makeCookie(name, value, expires, domain, path, secure, httpOnly, sameSite) + if isSome(headersOpt) and + (let headers = headersOpt.get(); headers.toTable.hasKey("Set-Cookie")): + headersOpt = some(headers & @({"Set-Cookie": newCookie})) + else: + setHeader(headersOpt, "Set-Cookie", newCookie) + template setCookie*(name, value: string, expires="", sameSite: SameSite=Lax, secure = false, httpOnly = false, domain = "", path = "") = @@ -725,12 +755,7 @@ template setCookie*(name, value: string, expires="", ## should protect you from most vulnerabilities. Note that this is only ## supported by some browsers: ## https://caniuse.com/#feat=same-site-cookie-attribute - let newCookie = makeCookie(name, value, expires, domain, path, secure, httpOnly, sameSite) - if isSome(result[2]) and - (let headers = result[2].get(); headers.toTable.hasKey("Set-Cookie")): - result[2] = some(headers & @({"Set-Cookie": newCookie})) - else: - setHeader(result[2], "Set-Cookie", newCookie) + responseHeaders.setCookie(name, value, expires, sameSite, secure, httpOnly, domain, path) template setCookie*(name, value: string, expires: DateTime, sameSite: SameSite=Lax, secure = false, @@ -1282,7 +1307,7 @@ proc routesEx(name: string, body: NimNode): NimNode = `afterRoutes` ) - let matchIdent = newIdentNode(name) + let matchIdent = newIdentNode(name & "Matcher") let reqIdent = newIdentNode("request") let needsAsync = needsAsync(body) case needsAsync @@ -1317,31 +1342,49 @@ proc routesEx(name: string, body: NimNode): NimNode = # Error handler proc let errorHandlerIdent = newIdentNode(name & "ErrorHandler") let errorIdent = newIdentNode("error") - let exceptionIdent = newIdentNode("exception") - let resultIdent = newIdentNode("result") - var errorHandlerProc = quote do: - proc `errorHandlerIdent`( - `reqIdent`: Request, `errorIdent`: RouteError - ): Future[ResponseData] {.gcsafe, async.} = - block `routesListIdent`: - `setDefaultRespIdent`() - case `errorIdent`.kind - of RouteException: - discard - of RouteCode: - discard + let allRoutesIdent = ident("allRoutes") + var exceptionStmts = newStmtList() if exceptionBranches.len != 0: - var stmts = newStmtList() for branch in exceptionBranches: - stmts.add(newIfStmt(branch)) - errorHandlerProc[6][0][1][^1][1][1][0] = stmts + exceptionStmts.add(newIfStmt(branch)) + var codeStmts = newStmtList() if httpCodeBranches.len != 0: - var stmts = newStmtList() for branch in httpCodeBranches: - stmts.add(newIfStmt(branch)) - errorHandlerProc[6][0][1][^1][2][1][0] = stmts + codeStmts.add(newIfStmt(branch)) + var errorHandlerProc = quote do: + proc `errorHandlerIdent`( + `reqIdent`: Request, `errorIdent`: RouteError + ): Future[ResponseData] {.gcsafe, async.} = + block `allRoutesIdent`: + block `routesListIdent`: + `setDefaultRespIdent`() + case `errorIdent`.kind + of RouteException: + `exceptionStmts` + of RouteCode: + `codeStmts` result.add(errorHandlerProc) + # Pair the matcher and error matcher + let pairIdent = newIdentNode(name) + let matchProcVarIdent = newIdentNode(name & "MatchProc") + let errorProcVarIdent = newIdentNode(name & "ErrorProc") + if needsAsync in {ImplicitTrue, ExplicitTrue}: + # TODO: I don't understand why I have to assign these procs to intermediate + # variables in order to get them into the tuple. It would be nice if it could + # just be: + # let `pairIdent`: MatchPair = (`matchIdent`, `errorHandlerIdent`) + result.add quote do: + let `matchProcVarIdent`: MatchProc = `matchIdent` + let `errorProcVarIdent`: ErrorProc = `errorHandlerIdent` + let `pairIdent`: MatchPair = (`matchProcVarIdent`, `errorProcVarIdent`) + else: + result.add quote do: + let `matchProcVarIdent`: MatchProcSync = `matchIdent` + let `errorProcVarIdent`: ErrorProc = `errorHandlerIdent` + let `pairIdent`: MatchPairSync = (`matchProcVarIdent`, `errorProcVarIdent`) + + # TODO: Replace `body`, `headers`, `code` in routes with `result[i]` to # get these shortcuts back without sacrificing usability. # TODO2: Make sure you replace what `guessAction` used to do for this. @@ -1352,13 +1395,11 @@ proc routesEx(name: string, body: NimNode): NimNode = macro routes*(body: untyped) = result = routesEx("match", body) let jesIdent = genSym(nskVar, "jes") - let matchIdent = newIdentNode("match") - let errorHandlerIdent = newIdentNode("matchErrorHandler") + let pairIdent = newIdentNode("match") let settingsIdent = newIdentNode("settings") result.add( quote do: - var `jesIdent` = initJester(`matchIdent`, `settingsIdent`) - `jesIdent`.register(`errorHandlerIdent`) + var `jesIdent` = initJester(`pairIdent`, `settingsIdent`) ) result.add( quote do: diff --git a/jester.nimble b/jester.nimble index 4b9b82c..625fc63 100644 --- a/jester.nimble +++ b/jester.nimble @@ -13,10 +13,15 @@ skipDirs = @["tests"] requires "nim >= 0.18.1" when not defined(windows): - requires "httpbeast >= 0.2.2" + # When https://github.com/cheatfate/asynctools/pull/28 is fixed, + # change this back to normal httpbeast + # requires "httpbeast >= 0.2.2" + requires "https://github.com/iffy/httpbeast#github-actions" # For tests -requires "https://github.com/timotheecour/asynctools#pr_fix_compilation" +# When https://github.com/cheatfate/asynctools/pull/28 is fixed, +# change this back to normal asynctools +requires "https://github.com/iffy/asynctools#pr_fix_for_latest" task test, "Runs the test suite.": exec "nimble c -y -r tests/tester" \ No newline at end of file diff --git a/tests/alltest.nim b/tests/alltest.nim index 026615c..2b41f33 100644 --- a/tests/alltest.nim +++ b/tests/alltest.nim @@ -45,6 +45,12 @@ routes: get "/halt": resp "

Not halted!

" + + before re"/halt-before/.*?": + halt Http502, "Halted!" + + get "/halt-before/@something": + resp "Should never reach this" get "/guess/@who": if @"who" != "Frank": pass() @@ -55,6 +61,16 @@ routes: get "/redirect/@url/?": redirect(uri(@"url")) + + get "/redirect-halt/@url/?": + redirect(uri(@"url")) + resp "ok" + + before re"/redirect-before/.*?": + redirect(uri("/nowhere")) + + get "/redirect-before/@url/?": + resp "should not get here" get "/win": cond rand(5) < 3 diff --git a/tests/customRouter.nim b/tests/customRouter.nim new file mode 100644 index 0000000..e007231 --- /dev/null +++ b/tests/customRouter.nim @@ -0,0 +1,25 @@ +import jester + +router myrouter: + get "/": + resp "Hello world" + + get "/404": + resp "you got 404" + + get "/raise": + raise newException(Exception, "Foobar") + + error Exception: + resp Http500, "Something bad happened: " & exception.msg + + error Http404: + redirect uri("/404") + +when isMainModule: + let s = newSettings( + Port(5454), + bindAddr="127.0.0.1", + ) + var jest = initJester(myrouter, s) + jest.serve() diff --git a/tests/issue150.nim b/tests/issue150.nim index 2f5ce83..250cbee 100644 --- a/tests/issue150.nim +++ b/tests/issue150.nim @@ -4,6 +4,7 @@ import jester settings: port = Port(5454) + bindAddr = "127.0.0.1" routes: get "/": diff --git a/tests/tester.nim b/tests/tester.nim index 240a2d7..6469854 100644 --- a/tests/tester.nim +++ b/tests/tester.nim @@ -11,12 +11,20 @@ const var serverProcess: AsyncProcess proc readLoop(process: AsyncProcess) {.async.} = - while process.running: + var wholebuf: string + while true: var buf = newString(256) let len = await readInto(process.outputHandle, addr buf[0], 256) + if len == 0: + break buf.setLen(len) - styledEcho(fgBlue, "Process: ", resetStyle, buf.strip()) - + wholebuf.add(buf) + while "\l" in wholebuf: + let parts = wholebuf.split("\l", 1) + styledEcho(fgBlue, "Process: ", resetStyle, parts[0]) + wholebuf = parts[1] + if wholebuf != "": + styledEcho(fgBlue, "Process: ", resetStyle, wholebuf) styledEcho(fgRed, "Process terminated") proc startServer(file: string, useStdLib: bool) {.async.} = @@ -40,10 +48,10 @@ proc startServer(file: string, useStdLib: bool) {.async.} = asyncCheck readLoop(serverProcess) # Wait until server responds: - + await sleepAsync(10) # give it a chance to start for i in 0..10: var client = newAsyncHttpClient() - styledEcho(fgBlue, "Getting ", address) + styledEcho(fgBlue, "Getting ", address, " - attempt " & $i) let fut = client.get(address) yield fut or sleepAsync(3000) if not fut.finished: @@ -53,6 +61,8 @@ proc startServer(file: string, useStdLib: bool) {.async.} = return else: echo fut.error.msg client.close() + if not serverProcess.running: + doAssert false, "Server died." await sleepAsync(1000) doAssert false, "Failed to start server." @@ -64,6 +74,8 @@ proc allTest(useStdLib: bool) = test "doesn't crash on missing script name": # If this fails then alltest is likely not running. let resp = waitFor client.get(address) + checkpoint (waitFor resp.body) + checkpoint $resp.code check resp.code.is5xx test "can access root": @@ -82,6 +94,11 @@ proc allTest(useStdLib: bool) = let resp = waitFor client.get(address & "/foo/halt") check resp.status.startsWith("502") check (waitFor resp.body) == "I'm sorry, this page has been halted." + + test "/halt-before": + let resp = waitFor client.request(address & "/foo/halt-before/something", HttpGet) + let body = waitFor resp.body + check body == "Halted!" test "/guess": let resp = waitFor client.get(address & "/foo/guess/foo") @@ -92,6 +109,17 @@ proc allTest(useStdLib: bool) = test "/redirect": let resp = waitFor client.request(address & "/foo/redirect/halt", HttpGet) check resp.headers["location"] == "http://localhost:5454/foo/halt" + + test "/redirect-halt": + let resp = waitFor client.request(address & "/foo/redirect-halt/halt", HttpGet) + check resp.headers["location"] == "http://localhost:5454/foo/halt" + check (waitFor resp.body) == "" + + test "/redirect-before": + let resp = waitFor client.request(address & "/foo/redirect-before/anywhere", HttpGet) + check resp.headers["location"] == "http://localhost:5454/foo/nowhere" + let body = waitFor resp.body + check body == "" test "regex": let resp = waitFor client.get(address & "/foo/02.html") @@ -219,12 +247,32 @@ proc issue150(useStdLib: bool) = check resp.code == Http500 check (waitFor resp.body).startsWith("Something bad happened") +proc customRouterTest(useStdLib: bool) = + waitFor startServer("customRouter.nim", useStdLib) + var client = newAsyncHttpClient(maxRedirects = 0) + + suite "customRouter useStdLib=" & $useStdLib: + test "error handler": + let resp = waitFor client.get(address & "/raise") + check resp.code == Http500 + let body = (waitFor resp.body) + checkpoint body + check body.startsWith("Something bad happened: Foobar") + + test "redirect in error": + let resp = waitFor client.get(address & "/definitely404route") + check resp.code == Http303 + check resp.headers["location"] == address & "/404" + check (waitFor resp.body) == "" + when isMainModule: try: allTest(useStdLib=false) # Test HttpBeast. allTest(useStdLib=true) # Test asynchttpserver. issue150(useStdLib=false) issue150(useStdLib=true) + customRouterTest(useStdLib=false) + customRouterTest(useStdLib=true) # Verify that Nim in Action Tweeter still compiles. test "Nim in Action - Tweeter":