Include neo4j's original error in error objects

[Acconut]

This way you can use error.originalError.fullname to retrieve the status code.

[Acconut]

Travis fails when using neo4j 1.8.3 because it doesn't support fullname there (see 18.3.5. Server errors) and I only tested it against 2.0.

[aseemk]

Cool, thanks @Acconut.

Just wondering: might it be better for us to instead just add e.g. a neo4jName property to our own Error object? Or perhaps we should keep it nested, but just change originalError to e.g. neo4j?

I've been wanting to tackle this in general (issue #73), so thanks for sending this!

[aseemk]

I've also been wondering if all of node-neo4j's errors that come from Neo4j errors should get their message prefixed with e.g. "Neo4j FooBarException:" (just like the no-message case just above this).

[aseemk]

(Not something you need to "fix" in this PR of course. Just an observation; curious if you have thoughts.)

[Acconut]

We could add properties for the exception, fullname and stacktrace but should not overwrite the defaults in an Error object (don't overwrite error.stack with neo4j's stacktrace).
I could think of following structure:

{
    message: "Neo4j ForBarException: message here...", // prefix it as you said
    stack: "...", // native stack from error object
    exception: "ForBarException",
    name: "Neo4jError",
    originalError: {
        fullname: "...",
        stacktrace: []
    }
}

We could built a custom error object but I'm not sure whether this is totally necessary.

[aseemk]

I think that's a great start!

One request: Can you think of a way for the caller to easily know whether the error was the client's fault or the server's? (The equivalent of HTTP 4xx vs. 5xx.)

We do have the status code to work with, but it'd be nice not to require the caller to do logic like statusCode >= 400 && statusCode < 500 directly.

[Acconut]

If you mean by client's fault something like invalid cypher syntax I would suggest parsing the exception name but this is quite hard.

According to the docs server-side errors (by plugins) returns a 500:

If the extension throws an exception response code 500 and a detailed error message is returned.

And 400 are Bad Request (http://docs.neo4j.org/chunked/stable/rest-api-cypher.html) so I think your way is the best, although I'm not sure for what you need this.

[aseemk]

Okay, how about we just add a status or statusCode property then?

If you wouldn't mind making these changes in this PR, that'd be fantastic and I'd be happy to merge. Thanks!

[Acconut]

Ok, I'll try.

[Acconut]

I hope the last commit will meet your requirements. :)
An error now looks like:

{ [Neo4jError: Neo4j SyntaxException: Query cannot conclude with START (must be RETURN or an update clause) (line 1, column 1)
"START notfound=node(999999)"
 ^]
  stack: [Getter/Setter],
  exception: 'SyntaxException',
  name: 'Neo4jError',
  message: 'Neo4j SyntaxException: Query cannot conclude with START (must be RETURN or an update clause) (line 1, column 1)\n"START notfound=node(999999)"\n ^',

  originalError:
   { message: 'Query cannot conclude with START (must be RETURN or an update clause) (line 1, column 1)\n"START notfound=node(999999)"\n ^',
     exception: 'SyntaxException',
     fullname: 'org.neo4j.cypher.SyntaxException',
     stacktrace:
      [ ... ] },
  statusCode: 400
}

[aseemk]

Looking great, thanks!

[aseemk]

One thing: let's keep the or serverError here. That accounts for improper HTML error responses from Neo4j.

[Acconut]

I've never seen such a case but I think know what you're talking about.

[Acconut]

But serverError.message will be set because of https://github.com/Acconut/node-neo4j/blob/originalError/lib/util.coffee#L87-L92

[aseemk]

Ah, the difference is that serverError itself may be a string instead of an object. This happens if Neo4j throws an exception in its pipeline before it knows how to properly return JSON objects for errors. (Or it could happen if the error comes from a proxy in between.)

With your changes so far, serverError.message in this case is '(no message)' now, but there's no serverError.exception defined, so the error we throw itself has a message of '(no message)'. That's okay, but previously, the error we threw had the string serverError as its message. Maybe the best would be something like "Neo4j error: #{serverError}" now.

Here's my proposal your current lines 87-100:

if serverError.exception and not serverError.message
    serverError.message = '(no message)'

status = error.statusCode

error = new Error
error.name = 'Neo4jError'

if serverError.exception
    error.message = "Neo4j #{serverError.exception}: #{serverError.message}"
    error.exception = serverError.exception
else
    error.message = "Neo4j unrecognized error: #{serverError.message or serverError}"

WDYT?

[Acconut]

I haven't thought of this case because I had no idea why we even use serverError but I'll try to implement this ASAP. It looks good; I can't think of anything against this. 👍

[aseemk]

LGTM otherwise! Will merge right after the requested changes. Thanks very much again! =)