From 595d3e89e45a02ddd278d335d7c341a31f832035 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 15 Mar 2016 16:56:13 +0100 Subject: [PATCH] Print warnings for explicitly requested parsers that do not exist Remove obsolete variable in the js parser by using already existing ones Add stringNumbers option --- README.md | 11 ++++++-- changelog.md | 6 +++++ lib/hiredis.js | 13 +++------- lib/javascript.js | 49 ++++++++++++++++++------------------ lib/parser.js | 30 ++++++++++++++++------ package.json | 7 +++--- test/parsers.spec.js | 60 +++++++++++++++++++++++++++++++++++++++++++- 7 files changed, 130 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 9889c46..c2bc0c3 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,8 @@ npm install redis-parser ## Usage ```js +var Parser = require('redis-parser'); + new Parser(options); ``` @@ -28,7 +30,8 @@ new Parser(options); * `returnError`: *function*; mandatory * `returnFatalError`: *function*; optional, defaults to the returnError function * `returnBuffers`: *boolean*; optional, defaults to false -* `name`: *javascript|hiredis*; optional, defaults to hiredis and falls back to the js parser if not available +* `name`: *javascript|hiredis*; optional, defaults to hiredis and falls back to the js parser if not available or if the stringNumbers option is choosen +* `stringNumbers`: *boolean*; optional, defaults to false. This is only available for the javascript parser at the moment! ### Example @@ -64,7 +67,9 @@ Library.prototype.streamHandler = function () { ``` You do not have to use the returnFatalError function. Fatal errors will be returned in the normal error function in that case. -And if you want to return buffers instead of strings, you can do this by adding the returnBuffers option. +And if you want to return buffers instead of strings, you can do this by adding the `returnBuffers` option. + +If you handle big numbers, you should pass the `stringNumbers` option. That case numbers above 2^53 can be handled properly without reduced precision. ```js // Same functions as in the first example @@ -76,6 +81,8 @@ var parser = new Parser({ returnError: function(err) { lib.returnError(err); }, + name: 'javascript', // Use the Javascript parser + stringNumbers: true, // Return all numbers as string instead of a js number returnBuffers: true // All strings are returned as buffer e.g. }); diff --git a/changelog.md b/changelog.md index da67a91..2bf2953 100644 --- a/changelog.md +++ b/changelog.md @@ -1,3 +1,9 @@ +## v.1.2.0 - 27 Mar, 2016 + +Features + +- Added `stringNumbers` option to make sure all numbers are returned as string instead of a js number for precision +- The parser is from now on going to print warnings if a parser is explicitly requested that does not exist and gracefully chooses the JS parser ## v.1.1.0 - 26 Jan, 2016 diff --git a/lib/hiredis.js b/lib/hiredis.js index 49df82e..965b660 100644 --- a/lib/hiredis.js +++ b/lib/hiredis.js @@ -2,12 +2,10 @@ var hiredis = require('hiredis'); -function HiredisReplyParser(returnBuffers) { +function HiredisReplyParser(options) { this.name = 'hiredis'; - this.returnBuffers = returnBuffers; - this.reader = new hiredis.Reader({ - return_buffers: returnBuffers - }); + this.options = options; + this.reader = new hiredis.Reader(options); } HiredisReplyParser.prototype.parseData = function () { @@ -16,11 +14,8 @@ HiredisReplyParser.prototype.parseData = function () { } catch (err) { // Protocol errors land here // Reset the parser. Otherwise new commands can't be processed properly - this.reader = new hiredis.Reader({ - return_buffers: this.returnBuffers - }); + this.reader = new hiredis.Reader(this.options); this.returnFatalError(err); - return void 0; } }; diff --git a/lib/javascript.js b/lib/javascript.js index c4740c9..a110345 100644 --- a/lib/javascript.js +++ b/lib/javascript.js @@ -1,8 +1,6 @@ 'use strict'; -var util = require('util'); - -function JavascriptReplyParser(return_buffers) { +function JavascriptReplyParser(options) { this.name = 'javascript'; this.buffer = new Buffer(0); this.offset = 0; @@ -12,23 +10,29 @@ function JavascriptReplyParser(return_buffers) { this.type = 0; this.protocolError = false; this.offsetCache = 0; - if (return_buffers) { + // If returnBuffers is active, all return values are returned as buffers besides numbers and errors + if (options.return_buffers) { this.handleReply = function (start, end) { return this.buffer.slice(start, end); }; + } else { + this.handleReply = function (start, end) { + return this.buffer.toString('utf-8', start, end); + }; + } + // If stringNumbers is activated the parser always returns numbers as string + // This is important for big numbers (number > Math.pow(2, 53)) as js numbers are 64bit floating point numbers with reduced precision + if (options.string_numbers) { + this.handleNumbers = function (start, end) { + return this.buffer.toString('ascii', start, end); + }; + } else { + this.handleNumbers = function (start, end) { + return +this.buffer.toString('ascii', start, end); + }; } } -JavascriptReplyParser.prototype.handleReply = function (start, end) { - return this.buffer.toString('utf-8', start, end); -}; - -function IncompleteReadBuffer(message) { - this.name = 'IncompleteReadBuffer'; - this.message = message; -} -util.inherits(IncompleteReadBuffer, Error); - JavascriptReplyParser.prototype.parseResult = function (type) { var start = 0, end = 0, @@ -48,7 +52,7 @@ JavascriptReplyParser.prototype.parseResult = function (type) { this.chunksSize = this.buffers[0].length; // Include the packetHeader delimiter this.bigStrSize = packetHeader + 2; - throw new IncompleteReadBuffer('Wait for more data.'); + throw new Error('Wait for more data.'); } // Set the offset to after the delimiter this.offset = end + 2; @@ -60,7 +64,7 @@ JavascriptReplyParser.prototype.parseResult = function (type) { // Include the delimiter this.offset = end + 2; // Return the coerced numeric value - return +this.buffer.toString('ascii', start, end); + return this.handleNumbers(start, end); } else if (type === 43) { // + end = this.packetEndOffset(); start = this.offset; @@ -74,7 +78,7 @@ JavascriptReplyParser.prototype.parseResult = function (type) { reply = []; for (var i = 0; i < packetHeader; i++) { if (this.offset >= this.buffer.length) { - throw new IncompleteReadBuffer('Wait for more data.'); + throw new Error('Wait for more data.'); } reply.push(this.parseResult(this.buffer[this.offset++])); } @@ -84,8 +88,6 @@ JavascriptReplyParser.prototype.parseResult = function (type) { start = this.offset; this.offset = end + 2; return new Error(this.buffer.toString('utf-8', start, end)); - } else { - return void 0; } }; @@ -107,7 +109,6 @@ JavascriptReplyParser.prototype.execute = function (buffer) { this.buffer = Buffer.concat([this.buffer.slice(this.offset), buffer]); } this.offset = 0; - this.protocolError = true; this.run(); }; @@ -118,8 +119,8 @@ JavascriptReplyParser.prototype.tryParsing = function () { // Catch the error (not enough data), rewind if it's an array, // and wait for the next packet to appear this.offset = this.offsetCache; - this.protocolError = false; - return void 0; + // Indicate that there's no protocol error by resetting the type too + this.type = undefined; } }; @@ -139,7 +140,7 @@ JavascriptReplyParser.prototype.run = function () { this.type = this.buffer[this.offset++]; reply = this.tryParsing(); } - if (this.type !== undefined && this.protocolError === true) { + if (this.type !== undefined) { // Reset the buffer so the parser can handle following commands properly this.buffer = new Buffer(0); this.returnFatalError(new Error('Protocol error, got ' + JSON.stringify(String.fromCharCode(this.type)) + ' as reply type byte')); @@ -162,7 +163,7 @@ JavascriptReplyParser.prototype.packetEndOffset = function () { offset++; if (offset >= len) { - throw new IncompleteReadBuffer('Did not see LF after NL reading multi bulk count (' + offset + ' => ' + this.buffer.length + ', ' + this.offset + ')'); + throw new Error('Did not see LF after NL reading multi bulk count (' + offset + ' => ' + this.buffer.length + ', ' + this.offset + ')'); } } return offset; diff --git a/lib/parser.js b/lib/parser.js index 9482116..045dedc 100644 --- a/lib/parser.js +++ b/lib/parser.js @@ -10,7 +10,7 @@ try { } catch (err) { /* ignore errors */ } function Parser (options) { - var parser; + var parser, msg; if ( !options || @@ -21,18 +21,34 @@ function Parser (options) { } /* istanbul ignore if: hiredis should always be installed while testing */ - if (options.name === 'hiredis' && !parsers.hiredis) { - console.warn('<< WARNING >> You explicitly required the hiredis parser but hiredis is not installed. The js parser is going to be returned instead.'); + if (options.name === 'hiredis') { + if (!parsers.hiredis) { + msg = 'You explicitly required the hiredis parser but hiredis is not installed. The JS parser is going to be returned instead.'; + } else if (options.stringNumbers) { + msg = 'You explicitly required the hiredis parser in combination with the stringNumbers option. Only the JS parser can handle that and is choosen instead.'; + } + } else if (options.name && !parsers[options.name]) { + msg = 'The requested parser "' + options.name + '" is unkown and the JS parser is choosen instead.'; + } + + if (msg) { + console.warn(new Error(msg).stack.replace('Error: ', 'Warning: ')); + options.name = 'javascript'; } options.name = options.name || 'hiredis'; options.name = options.name.toLowerCase(); - options.returnBuffers = options.returnBuffers || false; - if (options.name === 'javascript' || !parsers.hiredis) { - parser = new parsers.javascript(options.returnBuffers); + var innerOptions = { + // The hiredis parser expects underscores + return_buffers: options.returnBuffers || false, + string_numbers: options.stringNumbers || false + }; + + if (options.name === 'javascript' || !parsers.hiredis || options.stringNumbers) { + parser = new parsers.javascript(innerOptions); } else { - parser = new parsers.hiredis(options.returnBuffers); + parser = new parsers.hiredis(innerOptions); } parser.returnError = options.returnError; diff --git a/package.json b/package.json index 3b30ea2..fe63b87 100644 --- a/package.json +++ b/package.json @@ -28,10 +28,11 @@ "node": ">=0.10.0" }, "devDependencies": { - "jshint": "^2.8.0", - "mocha": "^2.3.2", + "codeclimate-test-reporter": "^0.1.1", + "intercept-stdout": "^0.1.2", "istanbul": "^0.4.0", - "codeclimate-test-reporter": "^0.1.1" + "jshint": "^2.8.0", + "mocha": "^2.3.2" }, "optionalDependency": { "hiredis": "^0.4.1" diff --git a/test/parsers.spec.js b/test/parsers.spec.js index f0b05fe..9092f16 100644 --- a/test/parsers.spec.js +++ b/test/parsers.spec.js @@ -1,5 +1,6 @@ 'use strict'; +var intercept = require('intercept-stdout'); var assert = require('assert'); var Parser = require('../'); var parsers = ['javascript', 'hiredis']; @@ -28,10 +29,44 @@ describe('parsers', function () { returnReply: returnReply, returnBuffers: true }); - }, 'Please provide all return functions while initiating the parser'); + }, function (err) { + assert.strictEqual(err.message, 'Please provide all return functions while initiating the parser'); + return true; + }); }); + it('unknown parser', function () { + var str = ''; + var unhookIntercept = intercept(function (data) { + str += data; + return ''; + }); + new Parser({ + returnReply: returnReply, + returnError: returnError, + name: 'something_unknown' + }); + unhookIntercept(); + assert(/^Warning: The requested parser "something_unknown" is unkown and the JS parser is choosen instead\.\n +at new Parser/.test(str), str); + }); + + it('hiredis and stringNumbers', function () { + var str = ''; + var unhookIntercept = intercept(function (data) { + str += data; + return ''; + }); + new Parser({ + returnReply: returnReply, + returnError: returnError, + name: 'hiredis', + stringNumbers: true + }); + unhookIntercept(); + assert(/^Warning: You explicitly required the hiredis parser in combination with the stringNumbers option. .+.\.\n +at new Parser/.test(str), str); + }); + }); parsers.forEach(function (parserName) { @@ -401,6 +436,29 @@ describe('parsers', function () { parser.execute(new Buffer('\n')); assert.strictEqual(replyCount, 3); }); + + it('return numbers as strings', function () { + var replyCount = 0; + var entries = ['123', '590295810358705700002', '-99999999999999999']; + function checkReply(reply) { + assert.strictEqual(typeof reply, 'string'); + assert.strictEqual(reply, entries[replyCount]); + replyCount++; + } + var unhookIntercept = intercept(function () { + return ''; + }); + var parser = new Parser({ + returnReply: checkReply, + returnError: returnError, + returnFatalError: returnFatalError, + name: parserName, + stringNumbers: true + }); + unhookIntercept(); + parser.execute(new Buffer(':123\r\n:590295810358705700002\r\n:-99999999999999999\r\n')); + assert.strictEqual(replyCount, 3); + }); }); }); });