v2 / Errors: improve impl. and tests.

Author: aseemk

API_v2.md

@@ -305,11 +305,11 @@ using `Error` subclasses. Importantly:
 
 - Special care is taken to provide `message` and `stack` properties rich in
   info, so that no special serialization is needed to debug production errors.
+  That is, simply logging the `stack` (as most error handlers tend to do)
+  should get you meaningful and useful data.
 
 - Structured info returned by Neo4j is also available on the `Error` instances
   under a `neo4j` property, for deeper introspection and analysis if desired.
-  In addition, if this error is associated with a full HTTP response, the HTTP
-  `statusCode`, `headers`, and `body` are available via an `http` property.
 
 ```coffee
 class Error {name, message, stack, http, neo4j}

lib-new/GraphDatabase.coffee

@@ -1,5 +1,5 @@
 $ = require 'underscore'
-errors = require './errors'
+{Error} = require './errors'
 lib = require '../package.json'
 Node = require './Node'
 Relationship = require './Relationship'
@@ -55,20 +55,10 @@ module.exports = class GraphDatabase
 
             {body, headers, statusCode} = resp
 
-            if statusCode >= 500
-                # TODO: Parse errors, and differentiate w/ TransientErrors.
-                err = new errors.DatabaseError 'TODO',
-                    http: {body, headers, statusCode}
+            if err = Error._fromResponse resp
                 return cb err
 
-            if statusCode >= 400
-                # TODO: Parse errors.
-                err = new errors.ClientError 'TODO',
-                    http: {body, headers, statusCode}
-                return cb err
-
-            # Parse nodes and relationships in the body, and return:
-            return cb null, _transform body
+            cb null, _transform body
 
 
 ## HELPERS

lib-new/errors.coffee

@@ -1,13 +1,41 @@
 $ = require 'underscore'
+http = require 'http'
 
 class @Error extends Error
 
-    Object.defineProperties @::,
-        name: {get: -> 'neo4j.' + @constructor.name}
-
-    constructor: (@message='Unknown error', {@http, @neo4j}={}) ->
+    constructor: (@message='Unknown error', @neo4j={}) ->
+        @name = 'neo4j.' + @constructor.name
         Error.captureStackTrace @, @constructor
 
+    #
+    # Accepts the given HTTP client response, and if it represents an error,
+    # creates and returns the appropriate Error instance from it.
+    # If the response doesn't represent an error, returns null.
+    #
+    @_fromResponse: (resp) ->
+        {body, headers, statusCode} = resp
+
+        return null if statusCode < 400
+
+        ErrorType = if statusCode >= 500 then 'Database' else 'Client'
+        ErrorClass = exports["#{ErrorType}Error"]
+
+        message = "#{statusCode} "
+        logBody = statusCode >= 500     # TODO: Config to always log body?
+
+        if body?.exception
+            message += "#{body.exception}: #{body.message or '(no message)'}"
+        else
+            statusText = http.STATUS_CODES[statusCode]  # E.g. "Not Found"
+            reqText = "#{resp.request.method} #{resp.request.path}"
+            message += "#{statusText} response for #{reqText}"
+            logBody = true  # always log body if non-error returned
+
+        if logBody and body?
+            message += ": #{JSON.stringify body, null, 4}"
+
+        new ErrorClass message, body
+
     # TODO: Helper to rethrow native/inner errors? Not sure if we need one.
 
 class @ClientError extends @Error

test-new/http._coffee

@@ -45,6 +45,19 @@ expectNeo4jRoot = (body) ->
     expect(body).to.be.an 'object'
     expect(body).to.have.keys 'data', 'management'
 
+#
+# Asserts that the given object is a proper instance of the given Neo4j Error
+# subclass, including with the given message.
+# Additional checks, e.g. of the `neo4j` property's contents, are up to you.
+#
+expectError = (err, ErrorClass, message) ->
+    expect(err).to.be.an.instanceOf ErrorClass
+    expect(err.name).to.equal "neo4j.#{ErrorClass.name}"
+    expect(err.neo4j).to.be.an 'object'
+    expect(err.message).to.equal message
+    expect(err.stack).to.contain '\n'
+    expect(err.stack.split('\n')[0]).to.equal "#{err.name}: #{err.message}"
+
 
 ## TESTS
 
@@ -64,21 +77,19 @@ describe 'GraphDatabase::http', ->
 
         expectNeo4jRoot body
 
-    it 'should throw errors for 4xx responses by default', (_) ->
-        try
-            thrown = false
-            DB.http
-                method: 'POST'
-                path: '/'
-            , _
-        catch err
-            thrown = true
-            expect(err).to.be.an.instanceOf neo4j.ClientError
-            expect(err.name).to.equal 'neo4j.ClientError'
-            expect(err.http).to.be.an 'object'
-            expect(err.http.statusCode).to.equal 405
-
-        expect(thrown).to.be.true()
+    it 'should throw errors for 4xx responses by default', (done) ->
+        DB.http
+            method: 'POST'
+            path: '/'
+        , (err, body) ->
+            expect(err).to.exist()
+            expect(body).to.not.exist()
+
+            expectError err, neo4j.ClientError,
+                '405 Method Not Allowed response for POST /'
+            expect(err.neo4j).to.be.empty()
+
+            done()
 
     it 'should support returning raw responses', (_) ->
         resp = DB.http
@@ -100,23 +111,27 @@ describe 'GraphDatabase::http', ->
 
         expectResponse resp, 405 # Method Not Allowed
 
-    it 'should throw native errors always', (_) ->
+    it 'should throw native errors always', (done) ->
         db = new neo4j.GraphDatabase 'http://idontexist.foobarbaz.nodeneo4j'
+        db.http
+            path: '/'
+            raw: true
+        , (err, resp) ->
+            expect(err).to.exist()
+            expect(resp).to.not.exist()
 
-        try
-            thrown = false
-            db.http
-                path: '/'
-                raw: true
-            , _
-        catch err
-            thrown = true
+            # NOTE: *Not* using `expectError` here, because we explicitly don't
+            # wrap native (non-Neo4j) errors.
             expect(err).to.be.an.instanceOf Error
             expect(err.name).to.equal 'Error'
             expect(err.code).to.equal 'ENOTFOUND'
             expect(err.syscall).to.equal 'getaddrinfo'
+            expect(err.message).to.contain "#{err.syscall} #{err.code}"
+                # NOTE: Node 0.11 adds the hostname to the message.
+            expect(err.stack).to.contain '\n'
+            expect(err.stack.split('\n')[0]).to.equal "#{err.name}: #{err.message}"
 
-        expect(thrown).to.be.true()
+            done()
 
     it 'should support streaming'
         # Test that it immediately returns a duplex HTTP stream.