From 26e3e9747583d8d39a2f568764fc86ef0c037c5c Mon Sep 17 00:00:00 2001 From: "Timothy J. Warren" Date: Fri, 23 Jan 2015 15:33:41 -0500 Subject: [PATCH] Improve query parser to handle functions in where clauses --- lib/adapter.js | 9 ++ lib/driver.js | 8 ++ lib/query-builder.js | 92 +++++++--------- lib/query-parser.js | 189 +++++++++++++++++++++++++++++++-- lib/state.js | 29 +++++ tests/adapters/dblite_test.js | 16 +++ tests/adapters/mysql_test.js | 7 +- tests/adapters/sqlite3_test.js | 16 +++ tests/query-builder-base.js | 68 +++++------- tests/query-parser_test.js | 129 +++++++++++++++++++++- 10 files changed, 447 insertions(+), 116 deletions(-) create mode 100644 lib/state.js diff --git a/lib/adapter.js b/lib/adapter.js index d0e0a0a..5485e41 100755 --- a/lib/adapter.js +++ b/lib/adapter.js @@ -13,5 +13,14 @@ module.exports = { */ execute: function(sql, params, callback) { throw new Error("Correct adapter not defined for query execution"); + }, + + /** + * Close the connection that is open on the current adapter + * + * @return void + */ + close: function() { + throw new Error("Close method not defined for the current adapter"); } }; \ No newline at end of file diff --git a/lib/driver.js b/lib/driver.js index 7b77c2c..6988556 100755 --- a/lib/driver.js +++ b/lib/driver.js @@ -20,6 +20,8 @@ var d = { * @private */ _quote: function(str) { + //if (/[0-9]+|\'(.*?)\'/ig.test(str)) return str; + return (helpers.isString(str) && ! (str.startsWith(d.identifierChar) || str.endsWith(d.identifierChar))) ? d.identifierChar + str + d.identifierChar : str; @@ -71,6 +73,12 @@ var d = { return str.map(d.quoteIdentifiers); } +if ( ! helpers.isString(str)) +{ + console.error(str); + return str; +} + // Handle commas if (str.contains(',')) { diff --git a/lib/query-builder.js b/lib/query-builder.js index ffb7030..59a8e70 100755 --- a/lib/query-builder.js +++ b/lib/query-builder.js @@ -2,14 +2,15 @@ /** @module query-builder */ var getArgs = require('getargs'), - helpers = require('./helpers'); + helpers = require('./helpers'), + State = require('./state'); /** * Variables controlling the sql building * * @private */ -var state = {}; +var state = new State(); /* * SQL generation object @@ -155,10 +156,17 @@ var QueryBuilder = function(driver, adapter) { var obj = {}; - if (helpers.isScalar(args.$key) && !helpers.isUndefined(args.$val) && !helpers.isNull(args.$val)) + + if (helpers.isScalar(args.$key) && !helpers.isUndefined(args.$val)) { + // Convert key/val pair to a simple object obj[args.$key] = args.$val; } + else if (helpers.isScalar(args.$key) && helpers.isUndefined(args.$val)) + { + // If just a string for the key, and no value, create a simple object with duplicate key/val + obj[args.$key] = args.$key; + } else { obj = args.$key; @@ -177,15 +185,17 @@ var QueryBuilder = function(driver, adapter) { } }); + return state[args.$varName]; }, whereMixedSet: function(/*key, val*/) { var args = getArgs('key:string|object, [val]', arguments); state.whereMap = []; + state.rawWhereValues = []; _p.mixedSet('whereMap', 'both', args.key, args.val); - _p.mixedSet('whereValues', 'value', args.key, args.val); + _p.mixedSet('rawWhereValues', 'value', args.key, args.val); }, fixConjunction: function(conj) { var lastItem = state.queryMap[state.queryMap.length - 1]; @@ -210,24 +220,16 @@ var QueryBuilder = function(driver, adapter) { // Normalize key and value and insert into state.whereMap _p.whereMixedSet(key, val); - Object.keys(state.whereMap).forEach(function(field) { - // Split each key by spaces, in case there - // is an operator such as >, <, !=, etc. - var fieldArray = field.trim().split(' ').map(helpers.stringTrim); + // Parse the where condition to account for operators, + // functions, identifiers, and literal values + state = parser.parseWhere(driver, state); - var item = driver.quoteIdentifiers(fieldArray[0]); - - // Simple key value, or an operator? - item += (fieldArray.length === 1 || fieldArray[1] === '') ? '=?' : " " + fieldArray[1] + " ?"; - - // Determine the correct conjunction + state.whereMap.forEach(function(clause) { var conj = _p.fixConjunction(defaultConj); - - _p.appendMap(conj, item, 'where'); - - // Clear the where Map - state.whereMap = {}; + _p.appendMap(conj, clause, 'where'); }); + + state.whereMap = {}; }, whereNull: function(field, stmt, conj) { field = driver.quoteIdentifiers(field); @@ -243,20 +245,15 @@ var QueryBuilder = function(driver, adapter) { // Normalize key/val and put in state.whereMap _p.whereMixedSet(args.key, args.val); - Object.keys(state.whereMap).forEach(function(field) { - // Split each key by spaces, in case there - // is an operator such as >, <, !=, etc. - var fieldArray = field.split(' ').map(helpers.stringTrim); - - var item = driver.quoteIdentifiers(fieldArray[0]); - - // Simple key value, or an operator? - item += (fieldArray.length === 1) ? '=?' : " " + fieldArray[1] + " ?"; + // Parse the having condition to account for operators, + // functions, identifiers, and literal values + state = parser.parseWhere(driver, state); + state.whereMap.forEach(function(clause) { // Put in the having map state.havingMap.push({ conjunction: (state.havingMap.length > 0) ? " " + args.conj + " " : ' HAVING ', - string: item + string: clause }); }); @@ -291,7 +288,7 @@ var QueryBuilder = function(driver, adapter) { vals = state.values.concat(state.whereValues); } -//console.log(state.queryMap); +//console.log(state); //console.log(sql); //console.log(vals); //console.log(callback); @@ -314,28 +311,7 @@ var QueryBuilder = function(driver, adapter) { return sql; }, resetState: function() { - state = { - // Arrays/Maps - queryMap: [], - values: [], - whereValues: [], - setArrayKeys: [], - orderArray: [], - groupArray: [], - havingMap: [], - whereMap: {}, - - // Partials - selectString: '', - fromString: '', - setString: '', - orderString: '', - groupString: '', - - // Other various values - limit: null, - offset: null - }; + state = new State(); } }; @@ -363,10 +339,14 @@ var QueryBuilder = function(driver, adapter) { return state; }; - // ------------------------------------------------------------------------ - - // Set up state object - this.resetQuery(); + /** + * Closes the database connection for the current adapter + * + * @return void + */ + this.end = function() { + adapter.close(); + }; // ------------------------------------------------------------------------ // ! Query Builder Methods diff --git a/lib/query-parser.js b/lib/query-parser.js index 72f984e..5a27368 100644 --- a/lib/query-parser.js +++ b/lib/query-parser.js @@ -3,21 +3,39 @@ var helpers = require('./helpers'); var matchPatterns = { - 'function': /([a-zA-Z0-9_]+\((.*?)\))/i, - identifier: /([a-zA-Z0-9_\-]+\.?)+/ig, - operator: /\=|AND|&&?|~|\|\|?|\^|\/|>=?|<=?|-|%|OR|\+|NOT|\!=?|<>|XOR/i + 'function': /([a-z0-9_]+\((.*)\))/i, + operator: /\!=?|\=|\+|&&?|~|\|\|?|\^|\/|<>|>=?|<=?|\-|%|OR|AND|NOT|XOR/ig, + literal: /([0-9]+)|'(.*?)'|true|false/ig }; +// Full pattern for identifiers +// Making sure that literals and functions aren't matched +matchPatterns.identifier = new RegExp( + '(' + + '(?!' + + matchPatterns['function'].source + '|' + + matchPatterns.literal.source + + ')' + + '([a-z_\-]+[0-9]*\\.?)' + + ')+' +, 'ig'); + // Full pattern for determining ordering of the pieces -matchPatterns.combined = new RegExp(matchPatterns['function'].source + "+|" +matchPatterns.joinCombined = new RegExp( + matchPatterns['function'].source + "+|" + + matchPatterns.literal.source + '+|' + matchPatterns.identifier.source - + '|(' + matchPatterns.operator.source + ')+', 'ig'); + + '|(' + matchPatterns.operator.source + ')+' +, 'ig'); + +var identifierBlacklist = ['true','false','null']; var filterMatches = function(array) { var output = []; // Return non-array matches - if (helpers.isScalar(array) || helpers.isNull(array) || helpers.isUndefined(array)) return output; + if (helpers.isNull(array)) return null; + if (helpers.isScalar(array) || helpers.isUndefined(array)) return output; array.forEach(function(item) { output.push(item); @@ -38,6 +56,17 @@ var QueryParser = function(driver) { // That 'new' keyword is annoying if ( ! (this instanceof QueryParser)) return new QueryParser(driver); + /** + * Check if the string contains an operator, and if so, return the operator(s). + * If there are no matches, return null + * + * @param {String} string - the string to check + * @return {Array|null} + */ + this.hasOperator = function(string) { + return filterMatches(string.match(matchPatterns.operator)); + } + /** * Tokenize the sql into parts for additional processing * @@ -49,12 +78,13 @@ var QueryParser = function(driver) { var output = {}; // Get clause components - matches['function'] = sql.match(matchPatterns['function']); + matches.functions = sql.match(new RegExp(matchPatterns['function'].source, 'ig')); matches.identifiers = sql.match(matchPatterns.identifier); matches.operators = sql.match(matchPatterns.operator); + matches.literals = sql.match(matchPatterns.literal); // Get everything at once for ordering - matches.combined = sql.match(matchPatterns.combined); + matches.combined = sql.match(matchPatterns.joinCombined); // Flatten the matches to increase relevance Object.keys(matches).forEach(function(key) { @@ -83,8 +113,147 @@ var QueryParser = function(driver) { } }); - return parts.combined.join(''); + return parts.combined.join(' '); + }; + + /** + * Parse a where clause to separate functions from values + * + * @param {Object} driver + * @param {State} state + * @return {String} - The parsed/escaped where condition + */ + this.parseWhere = function(driver, state) { + var whereMap = state.whereMap, + whereValues = state.rawWhereValues; + + var outputMap = []; + var outputValues = []; + var that = this; + + Object.keys(whereMap).forEach(function(key) { + // Combine fields, operators, functions and values into a full clause + // to have a common starting flow + var fullClause = ''; + + // Add an explicit = sign where one is inferred + if ( ! that.hasOperator(key)) + { + fullClause = key + ' = ' + whereMap[key]; + } + else if (whereMap[key] === key) + { + fullClause = key; + } + else + { + fullClause = key + ' ' + whereMap[key]; + } + + // Separate the clause into separate pieces + var parts = that.parseJoin(fullClause); + + // Filter explicit literals from lists of matches + if (whereValues.indexOf(whereMap[key]) !== -1) + { + var value = whereMap[key]; + var identIndex = (helpers.isArray(parts.identifiers)) ? parts.identifiers.indexOf(value) : -1; + var litIndex = (helpers.isArray(parts.literals)) ? parts.literals.indexOf(value) : -1; + var combIndex = (helpers.isArray(parts.combined)) ? parts.combined.indexOf(value) : -1; + var funcIndex = (helpers.isArray(parts.functions)) ? parts.functions.indexOf(value) : -1; + var inOutputArray = outputValues.indexOf(value) !== -1; + + // Remove the identifier in question, + // and add to the output values array + if (identIndex !== -1) + { + parts.identifiers.splice(identIndex, 1); + + if ( ! inOutputArray) + { + outputValues.push(value); + inOutputArray = true; + } + } + + // Remove the value from the literals list + // so it is not added twice + if (litIndex !== -1) + { + parts.literals.splice(litIndex, 1); + + if ( ! inOutputArray) + { + outputValues.push(value); + inOutputArray = true; + } + } + + // Remove the value from the combined list + // and replace it with a placeholder + if (combIndex !== -1) + { + // Make sure to skip functions when replacing values + if (funcIndex === -1) + { + parts.combined[combIndex] = '?'; + + if ( ! inOutputArray) + { + outputValues.push(value); + inOutputArray = true; + } + } + } + } + + // Filter false positive identifiers + parts.identifiers = parts.identifiers.filter(function(item) { + var isInCombinedMatches = parts.combined.indexOf(item) !== -1; + var isNotInBlackList = identifierBlacklist.indexOf(item.toLowerCase()) === -1; + + return isInCombinedMatches && isNotInBlackList; + }); + + // Quote identifiers + if (helpers.isArray(parts.identifiers)) + { + parts.identifiers.forEach(function(ident) { + var index = parts.combined.indexOf(ident); + if (index !== -1) + { + parts.combined[index] = driver.quoteIdentifiers(ident); + } + }); + } + + // Replace each literal with a placeholder in the map + // and add the literal to the values, + // This should only apply to literal values that are not + // explicitly mapped to values, but have to be parsed from + // a where condition, + if (helpers.isArray(parts.literals)) + { + parts.literals.forEach(function(lit) { + var litIndex = parts.combined.indexOf(lit); + + if (litIndex !== -1) + { + parts.combined[litIndex] = (helpers.isArray(parts.operators)) ? '?' : '= ?'; + outputValues.push(lit); + } + }); + } + + outputMap.push(parts.combined.join(' ')); + }); + + state.rawWhereValues = []; + state.whereValues = state.whereValues.concat(outputValues); + state.whereMap = outputMap; + + return state; }; }; -module.exports = QueryParser; +module.exports = QueryParser; \ No newline at end of file diff --git a/lib/state.js b/lib/state.js new file mode 100644 index 0000000..549f491 --- /dev/null +++ b/lib/state.js @@ -0,0 +1,29 @@ +'use strict'; + +/** @module State */ +module.exports = function State() { + return { + // Arrays/Maps + queryMap: [], + values: [], + whereValues: [], + setArrayKeys: [], + orderArray: [], + groupArray: [], + havingMap: [], + whereMap: {}, + rawWhereValues: [], + + // Partials + selectString: '', + fromString: '', + setString: '', + orderString: '', + groupString: '', + + // Other various values + limit: null, + offset: null + }; +}; +// End of module State \ No newline at end of file diff --git a/tests/adapters/dblite_test.js b/tests/adapters/dblite_test.js index a648e9b..d2d7ddc 100644 --- a/tests/adapters/dblite_test.js +++ b/tests/adapters/dblite_test.js @@ -54,6 +54,22 @@ if (connection) test.done(); }; + tests['Select tests']['Select with function and argument in WHERE clause'] = function(test) { + test.expect(1); + qb.select('id') + .from('create_test') + .where('id', 'ABS(-88)') + .get(function(err, rows) { + if (err != null) { + test.done(); + throw new Error(err); + } + + test.ok(rows, 'dblite: Valid result for generated query'); + test.done(); + }); + }; + tests["dblite adapter with query builder"] = function(test) { test.expect(1); test.ok(testBase.qb); diff --git a/tests/adapters/mysql_test.js b/tests/adapters/mysql_test.js index 4cd5e22..c6d3a32 100644 --- a/tests/adapters/mysql_test.js +++ b/tests/adapters/mysql_test.js @@ -39,9 +39,12 @@ tests['nodeQuery.getQuery = nodeQuery.init'] = function(test) { tests["mysql adapter with query builder"] = function(test) { test.expect(1); test.ok(testBase.qb); - test.done(); + // Close the db connection - connection.end(); + qb = null; + connection.destroy(); + + test.done(); }; // Export the final test object diff --git a/tests/adapters/sqlite3_test.js b/tests/adapters/sqlite3_test.js index 9100f3b..8ed4fce 100644 --- a/tests/adapters/sqlite3_test.js +++ b/tests/adapters/sqlite3_test.js @@ -51,6 +51,22 @@ if (connection) test.done(); }); + tests['Select tests']['Select with function and argument in WHERE clause'] = function(test) { + test.expect(1); + qb.select('id') + .from('create_test') + .where('id', 'ABS(-88)') + .get(function(err, rows) { + if (err != null) { + test.done(); + throw new Error(err); + } + + test.ok(rows, 'sqlite3: Valid result for generated query'); + test.done(); + }); + }; + tests['nodeQuery.getQuery = nodeQuery.init'] = function(test) { test.expect(1); test.deepEqual(qb, nodeQuery.getQuery(), "getQuery returns same object"); diff --git a/tests/query-builder-base.js b/tests/query-builder-base.js index 05ee2bd..ea037da 100644 --- a/tests/query-builder-base.js +++ b/tests/query-builder-base.js @@ -1,6 +1,7 @@ 'use strict'; var helpers = require('../lib/helpers'); +var State = require('../lib/state'); module.exports = (function QueryBuilderTestBase() { @@ -143,6 +144,27 @@ module.exports = (function QueryBuilderTestBase() { .where('id', 3) .orWhereIsNull('id') .get(base.testCallback.bind(this, test)); + }, + 'Select with string where value': function(test) { + test.expect(1); + base.qb.select('id','key as k', 'val') + .from('create_test ct') + .where('id > 3') + .get(base.testCallback.bind(this, test)); + }, + /*'Select with function in WHERE clause': function(test) { + test.expect(1); + base.qb.select('id', 'key as k', 'val') + .from('create_test') + .where('val !=', 'CURRENT_TIMESTAMP()') + .get(base.testCallback.bind(this, test)); + },*/ + 'Select with function and argument in WHERE clause': function(test) { + test.expect(1); + base.qb.select('id') + .from('create_test') + .where('id', 'CEILING(SQRT(88))') + .get(base.testCallback.bind(this, test)); } }, // ! Grouping tests @@ -488,28 +510,7 @@ module.exports = (function QueryBuilderTestBase() { .from('bar') .where('baz', 'foobar'); - var state = { - // Arrays/Maps - queryMap: [], - values: [], - whereValues: [], - setArrayKeys: [], - orderArray: [], - groupArray: [], - havingMap: [], - whereMap: {}, - - // Partials - selectString: '', - fromString: '', - setString: '', - orderString: '', - groupString: '', - - // Other various values - limit: null, - offset: null - }; + var state = new State(); test.notDeepEqual(JSON.stringify(state), JSON.stringify(base.qb.getState())); test.done(); @@ -523,28 +524,7 @@ module.exports = (function QueryBuilderTestBase() { base.qb.resetQuery(); - var state = { - // Arrays/Maps - queryMap: [], - values: [], - whereValues: [], - setArrayKeys: [], - orderArray: [], - groupArray: [], - havingMap: [], - whereMap: {}, - - // Partials - selectString: '', - fromString: '', - setString: '', - orderString: '', - groupString: '', - - // Other various values - limit: null, - offset: null - }; + var state = new State(); test.deepEqual(state, base.qb.getState()); diff --git a/tests/query-parser_test.js b/tests/query-parser_test.js index 77c4f32..a1ccf02 100644 --- a/tests/query-parser_test.js +++ b/tests/query-parser_test.js @@ -2,10 +2,131 @@ // Use the base driver as a mock for testing delete require.cache[require.resolve('../lib/driver')]; +var getArgs = require('getargs'); +var helpers = require('../lib/helpers'); var driver = require('../lib/driver'); var parser = require('../lib/query-parser')(driver); +var State = require('../lib/state'); + +// Simulate query builder state +var state = new State(); + +var mixedSet = function(/* $varName, $valType, $key, [$val] */) { + var args = getArgs('$varName:string, $valType:string, $key:object|string|number, [$val]', arguments); + + var obj = {}; + + + if (helpers.isScalar(args.$key) && !helpers.isUndefined(args.$val)) + { + // Convert key/val pair to a simple object + obj[args.$key] = args.$val; + } + else if (helpers.isScalar(args.$key) && helpers.isUndefined(args.$val)) + { + // If just a string for the key, and no value, create a simple object with duplicate key/val + obj[args.$key] = args.$key; + } + else + { + obj = args.$key; + } + + Object.keys(obj).forEach(function(k) { + // If a single value for the return + if (['key','value'].indexOf(args.$valType) !== -1) + { + var pushVal = (args.$valType === 'key') ? k : obj[k]; + state[args.$varName].push(pushVal); + } + else + { + state[args.$varName][k] = obj[k]; + } + }); + + + return state[args.$varName]; +} + +var whereMock = function() { + var args = getArgs('key:string|object, [val]', arguments); + + state.whereMap = []; + state.whereValues = []; + + mixedSet('rawWhereValues', 'value', args.key, args.val); + mixedSet('whereMap', 'both', args.key, args.val); +} + +// ----------------------------------------------------------------------------- +// ! Start Tests +// ----------------------------------------------------------------------------- module.exports = { + 'Has operator tests': { + 'Has operator': function(test) { + var matches = parser.hasOperator('foo <> 2'); + test.deepEqual(['<>'], matches); + test.done(); + }, + 'Has no operator': function(test) { + var matches = parser.hasOperator('foo'); + test.equal(null, matches); + test.done(); + } + }, + 'Where parser tests': { + 'Has function full string': function(test) { + test.expect(1); + whereMock('time < SUM(FOO(BAR()))'); + var result = parser.parseWhere(driver, state); + test.deepEqual(['"time" < SUM(FOO(BAR()))'], state.whereMap); + + test.done(); + }, + 'Has function key/val': function(test) { + test.expect(1); + var map = whereMock('time <', 'SUM(FOO(BAR()))'); + state = parser.parseWhere(driver, state); + test.deepEqual(['"time" < SUM(FOO(BAR()))'], state.whereMap); + + test.done(); + }, + 'Has function key/val object': function(test) { + test.expect(1); + var map = whereMock({ + 'time <': "SUM(FOO(BAR('x')))" + }); + state = parser.parseWhere(driver, state); + test.deepEqual(['"time" < SUM(FOO(BAR(\'x\')))'], state.whereMap); + + test.done(); + }, + 'Has literal value': function(test) { + test.expect(2); + var map = whereMock({ + 'foo': 3 + }); + state = parser.parseWhere(driver, state); + test.deepEqual(['"foo" = ?'], state.whereMap); + test.deepEqual(['3'], state.whereValues); + + test.done(); + }, + 'Has multiple literal values': function(test) { + test.expect(2); + var map = whereMock({ + foo: 3, + bar: 5 + }); + state = parser.parseWhere(driver, state); + test.deepEqual(['"foo" = ?', '"bar" = ?'], state.whereMap); + test.deepEqual(['3','5'], state.whereValues); + + test.done(); + } + }, 'Parse join tests' : { 'Simple equals condition': function(test) { var matches = parser.parseJoin('table1.field1=table2.field2'); @@ -31,22 +152,22 @@ module.exports = { 'Compile join tests': { 'Simple equals condition': function(test) { var join = parser.compileJoin('table1.field1=table2.field2'); - test.deepEqual('"table1"."field1"="table2"."field2"', join); + test.deepEqual('"table1"."field1" = "table2"."field2"', join); test.done(); }, 'Db.table.field condition': function(test) { var join = parser.compileJoin('db1.table1.field1!=db2.table2.field2'); - test.deepEqual('"db1"."table1"."field1"!="db2"."table2"."field2"', join); + test.deepEqual('"db1"."table1"."field1" != "db2"."table2"."field2"', join); test.done(); }, 'Underscore in identifier': function(test) { var join = parser.compileJoin('table_1.field1 = tab_le2.field_2'); - test.deepEqual('"table_1"."field1"="tab_le2"."field_2"', join); + test.deepEqual('"table_1"."field1" = "tab_le2"."field_2"', join); test.done(); }, 'Function in condition': function(test) { var join = parser.compileJoin('table1.field1 > SUM(3+6)'); - test.deepEqual('"table1"."field1">SUM(3+6)', join); + test.deepEqual('"table1"."field1" > SUM(3+6)', join); test.done(); } }