Merge pull request #31 from NodeRedis/fix-errors

Fix stack traces

Author: BridgeAR

.gitignore

@@ -3,3 +3,4 @@ logs
 coverage
 node_modules
 .idea
+.vscode

.npmignore

@@ -11,3 +11,5 @@ test
 .travis.yml
 .gitignore
 *.log
+.vscode
+.codeclimate.yml

README.md

@@ -36,8 +36,12 @@ var myParser = new Parser(options);
 
 ### Error classes
 
-All errors returned by the parser are of the class `ReplyError` that is a sub class of `RedisError`.  
-Both types are exported by the parser.
+* `RedisError` sub class of Error
+* `ReplyError` sub class of RedisError
+* `ParserError` sub class of RedisError
+
+All Redis errors will be returned as `ReplyErrors` while a parser error is returned as `ParserError`.  
+All error classes are exported by the parser.
 
 ### Example
 

changelog.md

@@ -1,3 +1,14 @@
+## v.2.5.0 - 11 Mar, 2017
+
+Features
+
+-  Added a `ParserError` class to differentiate them to ReplyErrors. The class is also exported
+
+Bugfixes
+
+-  All errors now show their error message again next to the error name in the stack trace
+-  ParserErrors now show the offset and buffer attributes while being logged
+
 ## v.2.4.1 - 05 Feb, 2017
 
 Bugfixes

index.js

@@ -3,3 +3,4 @@
 module.exports = require('./lib/parser')
 module.exports.ReplyError = require('./lib/replyError')
 module.exports.RedisError = require('./lib/redisError')
+module.exports.ParserError = require('./lib/redisError')

lib/hiredis.js

@@ -2,20 +2,21 @@
 
 var hiredis = require('hiredis')
 var ReplyError = require('../lib/replyError')
+var ParserError = require('../lib/parserError')
 
 /**
  * Parse data
  * @param parser
  * @returns {*}
  */
-function parseData (parser) {
+function parseData (parser, data) {
   try {
     return parser.reader.get()
   } catch (err) {
     // Protocol errors land here
     // Reset the parser. Otherwise new commands can't be processed properly
     parser.reader = new hiredis.Reader(parser.options)
-    parser.returnFatalError(new ReplyError(err.message))
+    parser.returnFatalError(new ParserError(err.message, JSON.stringify(data), -1))
   }
 }
 
@@ -37,15 +38,15 @@ function HiredisReplyParser (options) {
 
 HiredisReplyParser.prototype.execute = function (data) {
   this.reader.feed(data)
-  var reply = parseData(this)
+  var reply = parseData(this, data)
 
   while (reply !== undefined) {
     if (reply && reply.name === 'Error') {
       this.returnError(new ReplyError(reply.message))
     } else {
       this.returnReply(reply)
     }
-    reply = parseData(this)
+    reply = parseData(this, data)
   }
 }
 

lib/parser.js

@@ -3,6 +3,7 @@
 var StringDecoder = require('string_decoder').StringDecoder
 var decoder = new StringDecoder()
 var ReplyError = require('./replyError')
+var ParserError = require('./parserError')
 var bufferPool = new Buffer(32 * 1024)
 var bufferOffset = 0
 var interval = null
@@ -290,10 +291,11 @@ function parseType (parser, type) {
     case 45: // -
       return parseError(parser)
     default:
-      var err = new ReplyError('Protocol error, got ' + JSON.stringify(String.fromCharCode(type)) + ' as reply type byte', 20)
-      err.offset = parser.offset
-      err.buffer = JSON.stringify(parser.buffer)
-      return handleError(parser, err)
+      return handleError(parser, new ParserError(
+        'Protocol error, got ' + JSON.stringify(String.fromCharCode(type)) + ' as reply type byte',
+        JSON.stringify(parser.buffer),
+        parser.offset
+      ))
   }
 }
 

lib/parserError.js

@@ -0,0 +1,25 @@
+'use strict'
+
+var util = require('util')
+var assert = require('assert')
+var RedisError = require('./redisError')
+var ADD_STACKTRACE = false
+
+function ParserError (message, buffer, offset) {
+  assert(buffer)
+  assert.strictEqual(typeof offset, 'number')
+  RedisError.call(this, message, ADD_STACKTRACE)
+  this.offset = offset
+  this.buffer = buffer
+  Error.captureStackTrace(this, ParserError)
+}
+
+util.inherits(ParserError, RedisError)
+
+Object.defineProperty(ParserError.prototype, 'name', {
+  value: 'ParserError',
+  configurable: true,
+  writable: true
+})
+
+module.exports = ParserError

lib/redisError.js

@@ -2,19 +2,22 @@
 
 var util = require('util')
 
-function RedisError (message) {
-  Error.call(this, message)
-  Error.captureStackTrace(this, this.constructor)
+function RedisError (message, stack) {
   Object.defineProperty(this, 'message', {
     value: message || '',
+    configurable: true,
     writable: true
   })
+  if (stack || stack === undefined) {
+    Error.captureStackTrace(this, RedisError)
+  }
 }
 
 util.inherits(RedisError, Error)
 
 Object.defineProperty(RedisError.prototype, 'name', {
   value: 'RedisError',
+  configurable: true,
   writable: true
 })
 

lib/replyError.js

@@ -2,18 +2,21 @@
 
 var util = require('util')
 var RedisError = require('./redisError')
+var ADD_STACKTRACE = false
 
-function ReplyError (message, newLimit) {
-  var limit = Error.stackTraceLimit
-  Error.stackTraceLimit = newLimit || 2
-  RedisError.call(this, message)
-  Error.stackTraceLimit = limit
+function ReplyError (message) {
+  var tmp = Error.stackTraceLimit
+  Error.stackTraceLimit = 2
+  RedisError.call(this, message, ADD_STACKTRACE)
+  Error.captureStackTrace(this, ReplyError)
+  Error.stackTraceLimit = tmp
 }
 
 util.inherits(ReplyError, RedisError)
 
 Object.defineProperty(ReplyError.prototype, 'name', {
   value: 'ReplyError',
+  configurable: true,
   writable: true
 })
 

package.json

@@ -6,7 +6,8 @@
   "scripts": {
     "test": "npm run coverage",
     "benchmark": "node ./benchmark",
-    "posttest": "standard && npm run coverage:check",
+    "lint": "standard --fix",
+    "posttest": "npm run lint && npm run coverage:check",
     "coverage": "node ./node_modules/istanbul/lib/cli.js cover --preserve-comments ./node_modules/mocha/bin/_mocha -- -R spec",
     "coverage:check": "node ./node_modules/istanbul/lib/cli.js check-coverage --branch 100 --statement 100"
   },

test/errors.spec.js

@@ -0,0 +1,22 @@
+'use strict'
+
+/* eslint-env mocha */
+
+var assert = require('assert')
+var ReplyError = require('../lib/replyError')
+var ParserError = require('../lib/parserError')
+var RedisError = require('../lib/redisError')
+
+describe('errors', function () {
+  it('errors should have a stack trace with error message', function () {
+    var err1 = new RedisError('test')
+    var err2 = new ReplyError('test')
+    var err3 = new ParserError('test', new Buffer(''), 0)
+    assert(err1.stack)
+    assert(err2.stack)
+    assert(err3.stack)
+    assert(/RedisError: test/.test(err1.stack))
+    assert(/ReplyError: test/.test(err2.stack))
+    assert(/ParserError: test/.test(err3.stack))
+  })
+})

test/parsers.spec.js

@@ -3,9 +3,11 @@
 /* eslint-env mocha */
 
 var assert = require('assert')
+var util = require('util')
 var JavascriptParser = require('../')
 var HiredisParser = require('../lib/hiredis')
 var ReplyError = JavascriptParser.ReplyError
+var ParserError = JavascriptParser.ParserError
 var RedisError = JavascriptParser.RedisError
 var parsers = [HiredisParser, JavascriptParser]
 
@@ -318,10 +320,14 @@ describe('parsers', function () {
         Abc.prototype.checkReply = function (err) {
           assert.strictEqual(typeof this.log, 'function')
           assert.strictEqual(err.message, 'Protocol error, got "a" as reply type byte')
-          assert.strictEqual(err.name, 'ReplyError')
+          assert.strictEqual(err.name, 'ParserError')
           assert(err instanceof RedisError)
-          assert(err instanceof ReplyError)
+          assert(err instanceof ParserError)
           assert(err instanceof Error)
+          assert(err.offset)
+          assert(err.buffer)
+          assert(/\[97,42,49,13,42,49,13,36,49,96,122,97,115,100,13,10,97]/.test(err.buffer))
+          assert(/ParserError: Protocol error, got "a" as reply type byte/.test(util.inspect(err)))
           replyCount++
         }
         Abc.prototype.log = console.log