Skip to content

Implement Iterable Support #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
+142 −6
Closed

Implement Iterable Support #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
588e99f
support iterables -> array
krmannix Jun 19, 2016
c5c2ff4
Maps behave like Objects
krmannix Jun 20, 2016
f24ab03
support node v0.12
krmannix Jul 2, 2016
4c72db8
remove unnecessary checks
krmannix Jul 2, 2016
9fb600e
Merge branch 'master' of github.com:mysqljs/sqlstring
krmannix Jan 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 64 additions & 3 deletions lib/SqlString.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ var charsMap = {
'\'': '\\\'',
'\\': '\\\\'
};
var iterableSupport = isIterableSupported();

SqlString.escapeId = function escapeId(val, forbidQualified) {
if (Array.isArray(val)) {
if (isArrayLike(val)) {
val = toArray(val);
var sql = '';

for (var i = 0; i < val.length; i++) {
Expand Down Expand Up @@ -45,6 +47,8 @@ SqlString.escape = function escape(val, stringifyObjects, timeZone) {
return SqlString.arrayToList(val, timeZone);
} else if (Buffer.isBuffer(val)) {
return SqlString.bufferToString(val);
} else if (isArrayLike(val)) {
return SqlString.arrayToList(toArray(val), timeZone);
} else if (stringifyObjects) {
return escapeString(val.toString());
} else {
Expand All @@ -60,8 +64,8 @@ SqlString.arrayToList = function arrayToList(array, timeZone) {
for (var i = 0; i < array.length; i++) {
var val = array[i];

if (Array.isArray(val)) {
sql += (i === 0 ? '' : ', ') + '(' + SqlString.arrayToList(val, timeZone) + ')';
if (isArrayLike(val)) {
sql += (i === 0 ? '' : ', ') + '(' + SqlString.arrayToList(toArray(val), timeZone) + ')';
} else {
sql += (i === 0 ? '' : ', ') + SqlString.escape(val, true, timeZone);
}
Expand All @@ -75,6 +79,14 @@ SqlString.format = function format(sql, values, stringifyObjects, timeZone) {
return sql;
}

if (isArrayLike(values)) {
values = toArray(values);
}

if (isMap(values)) {
values = mapToObject(values);
}

if (!(values instanceof Array || Array.isArray(values))) {
values = [values];
}
Expand Down Expand Up @@ -161,6 +173,10 @@ SqlString.bufferToString = function bufferToString(buffer) {
SqlString.objectToValues = function objectToValues(object, timeZone) {
var sql = '';

if (isMap(object)) {
object = mapToObject(object);
}

for (var key in object) {
var val = object[key];

Expand Down Expand Up @@ -216,3 +232,48 @@ function convertTimezone(tz) {
}
return false;
}

function isArrayLike(val) {
if (!iterableSupport && Array.isArray(val)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it is not expected that the state of iterableSupport would change at runtime, is that correct? If so, it is possible to simply split apart this functions into two versions instead of checking the iterableSupport boolean constantly? I am seeing a bit of a performance dip in production from this PR, and I'm not 100% sure where it is coming from (it's running in an environment where iterableSupport === false), but when I simply changed these functions to only have the iterableSupport === false code, it helped bring it pretty much all the way back up to previous levels.

return true;
}
if (iterableSupport &&
!isMap(val) &&
typeof val[Symbol.iterator] === 'function' &&
typeof val !== 'string') {
return true;
}
return false;
}

function isMap(val) {
return iterableSupport && val instanceof Map;
}

function toArray(val) {
if (!iterableSupport || Array.isArray(val)) return val;
if (typeof Array.from === 'undefined') {
var arr = [];
val.forEach(function(key) {
arr.push(key);
});
return arr;
}
return Array.from(val);
}

function mapToObject(map) {
var object = {};
map.forEach(function(val, key) {
object[key] = val;
});
// if custom toString was implemented, attach to new object
Copy link
Member

@dougwilson dougwilson Sep 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive me for not fully understanding Map objects, but what is the purpose of this? Is it even performent to constantly create new Map() over and over again?

if (map.toString !== new Map().toString) {
object.toString = map.toString.bind(object);
}
return object;
}

function isIterableSupported() {
return 'function' === typeof Map && 'function' === typeof Set;
}
6 changes: 6 additions & 0 deletions test/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ common.root = path.resolve(__dirname, '..', (process.env.TEST_COVERAGE || ''));
// Export module
common.SqlString = require(common.root + '/index');

common.iterableSupport = isIterableSupported();

// Setup coverage hook
if (process.env.TEST_COVERAGE) {
process.on('exit', function () {
Expand All @@ -26,3 +28,7 @@ function writeCoverage(coverage) {

fs.writeFileSync(out, JSON.stringify(coverage));
}

function isIterableSupported() {
return 'function' === typeof Map && 'function' === typeof Set;
}
75 changes: 72 additions & 3 deletions test/unit/test-SqlString.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
var assert = require('assert');
var SqlString = require('../common').SqlString;
var test = require('utest');
var assert = require('assert');
var SqlString = require('../common').SqlString;
var test = require('utest');
var iterableSupport = require('../common').iterableSupport;

test('SqlString.escapeId', {
'value is quoted': function() {
Expand Down Expand Up @@ -31,6 +32,15 @@ test('SqlString.escapeId', {
assert.equal(SqlString.escapeId(['a', 'b', 't.c']), "`a`, `b`, `t`.`c`");
},

'sets are turned into lists': function() {
if (!iterableSupport) return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically it is best that if a test is going to be skipped, it's simply just not added to the object so it doesn't show up like there is a test happening. At least, the pattern with utest, which does not have a skip mechanism.

var set = new Set();
set.add('a');
set.add('b');
set.add('t.c');
assert.equal(SqlString.escapeId(set), "`a`, `b`, `t`.`c`");
},

'nested arrays are flattened': function() {
assert.equal(SqlString.escapeId(['a', ['b', ['t.c']]]), "`a`, `b`, `t`.`c`");
}
Expand Down Expand Up @@ -58,6 +68,14 @@ test('SqlString.escape', {
assert.equal(SqlString.escape({a: 'b', c: 'd'}), "`a` = 'b', `c` = 'd'");
},

'maps are turned into key value pairs': function() {
if (!iterableSupport) return;
var map = new Map();
map.set('a', 'b');
map.set('c', 'd');
assert.equal(SqlString.escape(map), "`a` = 'b', `c` = 'd'");
},

'objects function properties are ignored': function() {
assert.equal(SqlString.escape({a: 'b', c: function() {}}), "`a` = 'b'");
},
Expand All @@ -70,10 +88,20 @@ test('SqlString.escape', {
assert.equal(SqlString.escape([1, 2, 'c']), "1, 2, 'c'");
},

'sets are turned into lists': function() {
if (!iterableSupport) return;
assert.equal(SqlString.escape(new Set([1, 2, 'c'])), "1, 2, 'c'");
},

'nested arrays are turned into grouped lists': function() {
assert.equal(SqlString.escape([[1,2,3], [4,5,6], ['a', 'b', {nested: true}]]), "(1, 2, 3), (4, 5, 6), ('a', 'b', '[object Object]')");
},

'array of sets are turned into grouped lists': function() {
if (!iterableSupport) return;
assert.equal(SqlString.escape([new Set([1,2,3]), new Set([4,5,6]), new Set(['a', 'b', {nested: true}])]), "(1, 2, 3), (4, 5, 6), ('a', 'b', '[object Object]')");
},

'nested objects inside arrays are cast to strings': function() {
assert.equal(SqlString.escape([1, {nested: true}, 2]), "1, '[object Object]', 2");
},
Expand Down Expand Up @@ -212,6 +240,15 @@ test('SqlString.format', {
assert.equal(sql, "'a' and 'b'");
},

'question marks are replaced with escaped set values': function() {
if (!iterableSupport) return;
var set = new Set();
set.add('a');
set.add('b');
var sql = SqlString.format('? and ?', set);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, what? I don't think this makes sense. I don't think a top-level Set object should act like an array, because isn't a Set order independent? The top-level is very much order-dependent.

assert.equal(sql, "'a' and 'b'");
},

'double quest marks are replaced with escaped id': function () {
var sql = SqlString.format('SELECT * FROM ?? WHERE id = ?', ['table', 42]);
assert.equal(sql, 'SELECT * FROM `table` WHERE id = 42');
Expand All @@ -227,6 +264,16 @@ test('SqlString.format', {
assert.equal(sql, "'a' and 'b'");
},

'extra arguments in a set are not used': function() {
if (!iterableSupport) return;
var set = new Set();
set.add('a');
set.add('b');
set.add('c');
var sql = SqlString.format('? and ?', set);
assert.equal(sql, "'a' and 'b'");
},

'question marks within values do not cause issues': function() {
var sql = SqlString.format('? and ?', ['hello?', 'b']);
assert.equal(sql, "'hello?' and 'b'");
Expand All @@ -242,6 +289,14 @@ test('SqlString.format', {
assert.equal(sql, "`hello` = 'world'");
},

'maps is converted to values': function () {
if (!iterableSupport) return;
var map = new Map();
map.set('hello', 'world');
var sql = SqlString.format('?', map, false);
assert.equal(sql, "`hello` = 'world'");
},

'objects is not converted to values': function () {
var sql = SqlString.format('?', { 'hello': 'world' }, true);
assert.equal(sql, "'[object Object]'");
Expand All @@ -250,6 +305,20 @@ test('SqlString.format', {
assert.equal(sql, "'hello'");
},

'maps is not converted to values': function () {
if (!iterableSupport) return;
var map = new Map();
map.set('hello', 'world');

var sql = SqlString.format('?', map, true);
assert.equal(sql, "'[object Object]'");

map.toString = function () { return 'hello'; };

var sql = SqlString.format('?', map, true);
assert.equal(sql, "'hello'");
},

'sql is untouched if no values are provided': function () {
var sql = SqlString.format('SELECT ??');
assert.equal(sql, 'SELECT ??');
Expand Down