Fix redirecting in error handlers

.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/[email protected]
+      env:
+        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+      with:
+        nimversion: ${{ matrix.nimversion }}
+    - name: Test
+      run: |
+        nimble test
+        nimble refresh

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

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,16 +588,20 @@ 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
   result[1] = Http303
   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:

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"

tests/alltest.nim

@@ -45,6 +45,12 @@ routes:
 
   get "/halt":
     resp "<h1>Not halted!</h1>"
+
+  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

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()

tests/issue150.nim

@@ -4,6 +4,7 @@ import jester
 
 settings:
   port = Port(5454)
+  bindAddr = "127.0.0.1"
 
 routes:
   get "/":