sql().toParams() generating wrong parameter bindings

[joelmukuthu]

First off, thanks for a great solution! I just adopted this module for an ORM I've been working on and so far everything is great. I found a small bug though when writing tests (with version 2.0.3):

const sql = require('sql-bricks');
const query = sql
  .select()
  .from('foo')
  .where(
    sql.and(
      sql('a like $1',1), 
      sql('b <> $2', 1)
    )
  );

console.log(query.toString());
// => SELECT * FROM foo WHERE a like 1 AND b <> 1 // good

console.log(query.toParams());
// { text: 'SELECT * FROM foo WHERE a like $1 AND b <> $3', values: [ 1, 1 ] }
// here, `text` has wrong bindings

Is this a known issue? It's not a biggie because I can work around it by using ? as the placeholder (EDIT: I spoke too soon, it doesn't seem to work either), but I thought it would be nice to report this. Also more than happy to submit a PR if I can help in any way.

Thanks!

PS: Might be related to #96

[prust]

@joelmukuthu: Thanks for reporting, I'll see if I can dive in today & see what's going sideways here.

[prust]

@joelmukuthu: Sorry I didn't get a chance to look into this yesterday. I dove in today & was able to easily reproduce the behavior you're seeing.

I had to look at the tests to remember what it is trying to do and what kind of input it is expecting -- check out this test:

    it('should properly merge parameterized sub-expressions with $%d placeholders', function() {
      checkParams(select().from('tbl').where(or(sql('a = $1', 444), sql('b = $1', 555), sql('c = $1', 666))),
        'SELECT * FROM tbl WHERE a = $1 OR b = $2 OR c = $3',
        [444, 555, 666]);
    });

So the idea here is that the numbering of each subquery is independent and each subquery can come from different places and can be dynamically added and removed, and the numbering will always automatically update appropriately.

But I think your expectations (that it won't mess with user-supplied numbers) are probably more typical and most people will see this behavior as a bug.

One way to workaround the current behavior (to get it to be "hands-off" with user-supplied numbering is to configure it with an empty string placeholder: query.toParams({placeholder: ''}). That will give you the behavior you're expecting.

In general, this placeholder-replacing code -- while somewhat cool if you expect it -- contributes to the complexity of SqlBricks, so it's on my list for removal in 3.0. In the meantime, I suppose we could throw an error or log a warning to the console to alert users of the intended use if they pass a subquery without independent numbering (subqueries with a $2 but not a $1), though I'm not 100% sure if that would be worth the effort.

[joelmukuthu]

@prust thanks a lot for looking into this and for the explanation, it make sense. I'm getting more used to the "statelessness" of sql-bricks but probably should have figure out that the calls to .sql are independent.

I was also curious regarding what changes to expect in v3 and after your explanation here and re-reading the notes in #92, I agree 100% (with all changes). In that regard, let me know if there's anything I can help with.

Closing this :)

[prust]

@joelmukuthu: I created separate checkboxes for each task in #92 and checked off the one that's complete (implemented in #100, landed in master and tagged as v3.0.0-beta.1). You're welcome to try your hand at any of them, or (perhaps a better first step) to pick one and create an Issue for it and start fleshing out the details by analyzing the current implementation and outlining in more detail how it could be implemented or asking questions about different possible implementation choices.

That said, you're also welcome to wait until I've taken a first pass and figured things out in more detail, or -- if I get part way into implementing something & document how far I've gotten, you may want to carry the rock forward on that particular task. I'm planning on pecking away at this over the coming months, but my time on this project is limited, so any assistance would be much appreciated.

[joelmukuthu]

@prust thanks. I'm okay to proceed however you see fit and btw I hope it didn't come across as me urging you to get things done - was not at all my intention. I also don't have as much time as is ideal but I'll try to pick something up, most likely the 5th todo: "reduce the variety of ways things can be called".

Going back to this issue though, I've found another small bug:

const sql = require('sql-bricks');

const query = sql
  .select()
  .where(
    sql.not(sql('id = $1', 1)), 
    sql.not(sql('name like $1', 'user%'))
  );

console.log(query.toParams());
// { text: 'SELECT * WHERE NOT id = $1 AND NOT name like $2', values: [ 1, 'user%' ] } // good

console.log(query.toString())
// 'SELECT * WHERE NOT id = 1 AND NOT name like 1' // bad

I'm using toParams so this is not really a problem, but I thought I'd also point it out. Will the fixes to the templating code also address this?