Fix error handler redirecting

changelog.markdown

@@ -3,6 +3,8 @@
 ## 0.6.0 - ??/07/2021
 
 - **Breaking change:** the `@` operator used for retrieving request parameters now automatically decodes special characters using `decodeUrl`.
+- Fix for [#211](https://github.com/dom96/jester/issues/211) - custom routers now have the same error handling as normal routes.
+- Fix for [#269](https://github.com/dom96/jester/issues/269) - a bug that prevented redirecting from within error handlers.
 
 ## 0.5.0 - 17/10/2020
 

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
@@ -98,6 +106,10 @@ proc unsafeSend(request: Request, content: string) =
     # TODO: This may cause issues if we send too fast.
     asyncCheck request.getNativeReq.client.send(content)
 
+proc newCompletedFuture(): Future[void] =
+  result = newFuture[void]()
+  complete(result)
+
 proc send(
   request: Request, code: HttpCode, headers: Option[RawHeaders], body: string
 ): Future[void] =
@@ -106,9 +118,7 @@ proc send(
       if headers.isNone: ""
       else: headers.get().createHeaders
     request.getNativeReq.send(code, body, h)
-    var fut = newFuture[void]()
-    complete(fut)
-    return fut
+    return newCompletedFuture()
   else:
     return request.getNativeReq.respond(
       code, body, newHttpHeaders(headers.get(@({:})))
@@ -121,6 +131,7 @@ proc statusContent(request: Request, status: HttpCode, content: string,
     when not defined(release):
       logging.debug("  $1 $2" % [$status, toStr(headers)])
   except:
+    result = newCompletedFuture()
     logging.error("Could not send response: $1" % osErrorMsg(osLastError()))
 
 # TODO: Add support for proper Future Streams instead of this weird raw mode.
@@ -450,6 +461,22 @@ proc initJester*(
   result.matchers = @[]
   result.errorHandlers = @[]
 
+proc initJester*(
+  pair: MatchPair,
+  settings: Settings = newSettings()
+): Jester =
+  result = initJester(settings)
+  result.register(pair.matcher)
+  result.register(pair.errorHandler)
+
+proc initJester*(
+  pair: MatchPairSync, # TODO: Annoying nim bug: `MatchPair | MatchPairSync` doesn't work.
+  settings: Settings = newSettings()
+): Jester =
+  result = initJester(settings)
+  result.register(pair.matcher)
+  result.register(pair.errorHandler)
+
 proc initJester*(
   matcher: MatchProc,
   settings: Settings = newSettings()
@@ -458,7 +485,7 @@ proc initJester*(
   result.register(matcher)
 
 proc initJester*(
-  matcher: MatchProcSync, # TODO: Annoying nim bug: `MatchProc | MatchProcSync` doesn't work.
+  matcher: MatchProcSync,
   settings: Settings = newSettings()
 ): Jester =
   result = initJester(settings)
@@ -515,7 +542,7 @@ proc serve*(
     runForever()
 
 template setHeader*(headers: var ResponseHeaders, key, value: string): typed =
-  ## Sets a response header using the given key and value. 
+  ## Sets a response header using the given key and value.
   ## Overwrites if the header key already exists.
   bind isNone
   if isNone(headers):
@@ -1303,7 +1330,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
@@ -1338,31 +1365,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.
@@ -1373,13 +1418,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:

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/issue296.nim

@@ -0,0 +1,12 @@
+# Note, this isn't ran as part of the test suite as it relies on randomness too much.
+
+import jester, asyncdispatch, random, logging
+
+setLogFilter(lvlInfo)
+routes:
+  before "/":
+    setLogFilter(lvlInfo)
+  get "/":
+    let dur = rand(2000)
+    await sleepAsync(dur)
+    resp "hi"

tests/tester.nim

@@ -253,12 +253,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":