TryGhost / node-sqlite3

SQLite3 bindings for Node.js
BSD 3-Clause "New" or "Revised" License
6.14k stars 808 forks source link

Binding a list to a ? parameter doesn't work and doesn't error #373

Closed morungos closed 9 years ago

morungos commented 9 years ago

I'm trying to bind an array value into an IN expression, and nothing seems to be happening. As far as I can tell, a null is being injected, so it never matches. Nor is there any error. Here's my test:

I can't see any obvious reason why this shouldn't work. But looking at statement.cc I can't find any logic to handle this. Nor is it obvious what a workaround would consist of, without manually checking every single value and escaping. node-mysql is documented as turning arrays into list, e.g. ['a', 'b'] turns into 'a', 'b'.

var sqlite3 = require('..');
var assert = require('assert');

describe('bind a list', function() {
    var db;
    before(function(done) {
        db = new sqlite3.Database(':memory:', function(e) {
            if (e) return done(e);
            db.run("CREATE TABLE foo (num INT)", function (e) {
                if (e) return done(e);
                db.run("INSERT INTO foo VALUES(?)", [1], function (e) {
                    if (e) return done(e);
                    done();
                });
            });
        });
    });
    it('should correctly evaluate the case clause', function(done) {
        db.get("SELECT num, num IN (?) as bar FROM foo ORDER BY num", [[2, 3, 1]], function(err, row) {
            if (err) throw err;
            assert.equal(row.num, 1);
            assert.equal(row.bar, 1);
            done();
        });
    });
});
ColonelThirtyTwo commented 9 years ago

SQLite doesn't have an API for binding a list of items. Your best bet is to generate n variables to bind to and unpack the array in the parameters.

morungos commented 9 years ago

OK, same as this I guess: http://stackoverflow.com/questions/4788724/sqlite-bind-list-of-values-to-where-col-in-prm. I didn't know about this before. In any case, design decisions at the C level shouldn't really propagate to upper levels of query interface. In some DBs this actually works, so the effect is to create a non-portable SQL issue pushed from the C code.

I'm actually using knex.js, so there's a case for making this part of the knex.js layer, or (in my view) part of this layer. In any event, adding logic to one of these to reduce the lack of portability here would be a plus.

ColonelThirtyTwo commented 9 years ago

@morungos It's not that simple.

While "design decisions at the C level shouldn't really propagate to upper levels of query interface" is a nice ideal, bending over backwards to make a higher-level interface support something that the lower-level interface doesn't support is impractical.

morungos commented 9 years ago

OK, then it's a knex.js issue and I can push it there. After all, knex.js doesn't need to parse as it generates SQL. I didn't think parsing was an issue, as all would be needed would be tokenizing for SQLite, which is probably regexable, but it it's too much -- and I take the point about prepared statements.

Happy for you to close this anyways.

Mithgol commented 9 years ago

@morungos When you open a knex.js issue, please mention it here for future references.