Add toSQL(dialect) escaping support

[kevinmartin]

Adds the ability to raw "escape" by passing an object which contains a toSQL method.

// ES5:
function Point(x, y) {
    this.x = x;
    this.y = y;
}

Point.prototype.toSQL = function (dialect) {
    switch (dialect) {
        case 'mysql':
           return 'POINT(' + [x, y].map(parseFloat).join(', ') + ')';

        ...
    }
}

SqlString.escape(new Point(123, 456)); // -> POINT(123, 456)

This is also great for generic function handling:

// ES6 Proxies:
const db = new Proxy({}, {
    get(target, name) {
        if (name in target) {
            return target[name];
        }

        return (...args) => ({
            toSQL() {
                const escapedArgs = args.map(SqlString.escape).join(', ');
                return `${name.toUpperCase()}(${escapedArgs})`;
            }
        });
    }
});

const a = db.POINT(123, 456);
const b = db.CURRENT_TIMESTAMP();
const c = db.CONCAT(db.UPPER('abc'), db.LOWER('ABC'));

SqlString.escape(a); // -> POINT(123, 456)
SqlString.escape(b); // -> CURRENT_TIMESTAMP()
SqlString.escape(c); // -> CONCAT(UPPER("abc"), LOWER("ABC"))

This is per the discussion on #8 with @dougwilson

[kevinmartin]

@dougwilson update?

[dougwilson]

Sorry I didn't get back right away; have been very busy :) I just took a look through this PR and it looks good. The only other thing I want to do really quick is go through as many other SQL driver modules for Node.js to see if any of them are doing .toSQL or something similar to make sure there is no prior art before introducing this new pattern :)

[kevinmartin]

Sounds good to me. I'll be waiting.

[dougwilson]

haha, love the image :) So I browsed around and looked at the following:

• https://github.com/mscdex/node-mariasql
• https://github.com/brianc/node-postgres
• https://github.com/patriksimek/node-mssql

and didn't see a similar feature (though I just looked through the documentation). This means that there doesn't seem to be any prior art to copy, so nothing to even worry about :)

I did see that http://knexjs.org/ produces objects with the method .toSQL and looks like it would expect like this PR would use it if it were there, so even better 👍

I'm going to look some more around tonight, otherwise it seems good to merge, which I will try and to tomorrow :D

[kevinmartin]

I went ahead and did my own searches as well.

Knex's toSQL() output is an object (Link). ie:

knex.select('*').from('users').where(knex.raw('id = ?', [1])).toSQL()

// Ouputs: { bindings: [1], method: 'select', sql: 'select * from "users" where id = ?', options: undefined, }

So I don't really see any compatibility with Knex.

I also went into a deeper dive into the other repos you mentioned. No toSQL in the code at all.

I went ahead and also looked at Sequelize. They have a toSql() method (notice difference in capitalization), but only for internal data type structures.

On the other hand, I did find a small project called sql-mason that does have a toSQL method and functions like the one in this PR:

var Mason = require('sql-mason');
var builder = new Mason();
var query = builder.table('users').where('id', 1);
var sql = query.toSql(); //=> select * from `users` where `id` = ?;
var bindings = query.getBindings(); //=> [1]

[dougwilson]

Ah, my mistake on the Knex :) Thank you so much for helping as well :) ! My initial thoughts (I forget if I said this before) around thinking .toSQL vs .toSql is to match the already existing .toJSON casing on objects, as it feels like similar behavior. But I suppose if it seems like we are seeing the casing .toSql doing what we are expecting while also finding existing .toSQL not, you can convince me we need to change the casing :) As of now, unless you think otherwise, it seems like we can just go forward with the current implementation as-is

[kevinmartin]

[dougwilson]

Ugh, I need to merge this already :) I've been sitting there with your PR rebased on top of current master wanting to merge, but trying to think if there are security ramifications. I know all the code should be controlled by the app, but this module used to call buf.toString('hex') and apparently that was enough to alarm security folks and I added escaping around the out put _just in case somehow there was an external influence on the .toString of that object... -_-

I really do think using a method on the object is a good way, as the only other way would be to introduce a way to add a filter to all object such that you define converters separately from the objects themselves, such that tampering would have to happen outside of the user-controlled object. IDK man

[kevinmartin]

To be fair, if someone has the ability to externally influence the .toString of any object, they also have the ability influence SqlString.escape...

But I understand your concern. Created #13 as a possible alternative...