The Wayback Machine - https://web.archive.org/web/20200706014151/https://github.com/IBM-Swift/Kitura/issues/1071
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

Better error reporting #1071

Open
svanimpe opened this issue Apr 15, 2017 · 9 comments
Open

Better error reporting #1071

svanimpe opened this issue Apr 15, 2017 · 9 comments
Assignees

Comments

@svanimpe
Copy link
Contributor

@svanimpe svanimpe commented Apr 15, 2017

I'm in the process of writing my first (real) Kitura app and have been lost quite a lot. Kitura often sends Cannot GET /some/endpoint. responses without any error message in the log to help debug the issue. My log consists almost exclusively of [VERBOSE] [HTTPServerRequest.swift:215 parsingCompleted()] HTTP request from=127.0.0.1; proto=http; messages, even when there have been errors. I am using HeliumLogger and have tried different log levels but have never seen an actual error message.

@youming-lin
Copy link
Collaborator

@youming-lin youming-lin commented Apr 15, 2017

Are these errors due to no matched routes found? Kitura sends 404 Cannot GET /some/endpoint by default if no middleware/handler for a route was found, or if none of the found middleware/handlers sent data.

@svanimpe
Copy link
Contributor Author

@svanimpe svanimpe commented Apr 15, 2017

It might be that second option (none of the middleware/handlers sent data), but it would be nice to know why that happened. Some examples I encountered include:

  • A typo in a stencil file did not log an error, just "Cannot GET ..."
  • When using CredentialsFacebook, if there is no returnTo address set (meaning I willingly navigate to /login/facebook instead of being redirected there because of failed authentication) I get a "Cannot GET /login/facebook/callback" response instead of a message saying there is no returnTo address set.
  • Registering a template engine on a subrouter didn't work. See #1070
@svanimpe
Copy link
Contributor Author

@svanimpe svanimpe commented Apr 26, 2017

Can I add some weight to this issue? I have wasted hours trying to debug even the simplest bugs, simply because Kitura does not print any errors in its log. I am using HeliumLogger.use() as in the sample code, but every time an error is thrown, Kitura swallows it and simply returns "Cannot GET ...".

@svanimpe
Copy link
Contributor Author

@svanimpe svanimpe commented May 13, 2017

I have tried using router.error as per @shmuelk 's suggestion but that only works when my handlers set response.error explicitly. Wouldn't it make more sense if Kitura did this by default when a handler throws an error?

I did notice there's a line that sets the error in RouterMiddlewareWalker.swift but that seems to be unrelated as this line gets executed only after the default 404 response is sent.

@svanimpe
Copy link
Contributor Author

@svanimpe svanimpe commented May 13, 2017

I figured out the cause of my problem.
I (and lots of people with me I assume) have the habit of starting handlers with:

defer {
  next()
}

When a handler throws an error, that deferred next() causes the remaining handlers to be invoked and the response to be sent. These remaining handlers include the error handler, but it gets skipped as there is no error set yet. Only after the response has been sent, does the catch in RouterMiddlewareWalker.swift gets executed and response.error set.

So now I'm wondering if I'm using next() in the appropriate way. I did notice next() has to be called for a response to be sent. Since RouterMiddlewareWalker.swift includes an invocation of next() in its catch clause, should I just use next() instead of defer { next() } and assume that is enough to ensure next gets called in all cases?

@bridger
Copy link

@bridger bridger commented Jun 10, 2017

I've found that I need to put a breakpoint on this line to find errors. Could we just add logging to this one line?

https://github.com/IBM-Swift/Kitura/blob/master/Sources/Kitura/RouterMiddlewareWalker.swift#L67

            } catch {
                response.error = error
                self.next()
            }
@tettoffensive
Copy link

@tettoffensive tettoffensive commented Oct 25, 2017

@bridger What did you end up doing? With my custom error class I have an enum MyServiceError which for each error has an associated description and status error code. But this doesn't cover errors thrown by Kitura.

// Handles any errors that get set
    router.error { request, response, next in
      let errorDescription: String
      if let error = response.error as? MyServiceError {
        errorDescription = error.debugDescription
        response.status(error.errorStatus)
      } else if let error = response.error {
        errorDescription = error.localizedDescription
      } else {
        errorDescription = "Unknown error"
      }
      try response.send(json: JSON(["error": errorDescription])).end()
    }
@seabaylea
Copy link
Member

@seabaylea seabaylea commented Nov 6, 2017

@christiancompton can you pick this up?

@tettoffensive
Copy link

@tettoffensive tettoffensive commented Nov 8, 2017

As I play with 2.0, I'm seeing a limitation with codables. If I want to return a custom object in the case of an error. Rather than returning MyCodable, I'd like to return something like { "error": "this is the reason it errored"}.

router.post("/endpoint") { (codables: [MyCodable], respondWith: ([MyCodable?], RequestError?) -> Void ) in
      if !codables.isEmpty {
        do {
          let newCodables = try self.createOrUpdate(codables) // add to db
         respondWith(newCodables, nil) // return successfully updated models
        } catch {
          respondWith([], .internalServerError) // here i'd like to return an error obj rather than a MyCodable
        }
      }
      respondWith(codables, nil)
    }

I guess I could solve this by creating a Codable which contains a variable for data and one for status.

So...

{ 
 "data" : [ { "id": 1}, { "id": 2}, {"id": 2} ],
 "status": { "success": 1, "message" : "this is where an error msg could go on failure" }
}

Maybe this is the way to do things, though it adds a bit of complication to my api

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.