search

rockdai / sql-bricks

Transparent, Schemaless SQL Generation
http://csnw.github.io/sql-bricks
MIT License
213 stars 25 forks source link

Better sql() params support #76

Closed Suor closed 8 years ago

Suor commented 9 years ago

As far as I understand the correct way to use params in sql() now is using placeholders without numbers:

stmt.where(sql('some_func($, $)', [a, b]))

This is fine, however, under current implementation this also works:

stmt.where(sql('some_func($1, $2)', [a, b]))

But if stmt had more params either via parametrized sql() or higher level methods it can both work erroneously or throw. Also, given that above works one could expect that these should also work:

stmt.where(sql('some_func($2, $1)', [b, a]))
stmt.where(sql('some_func($1, $1)', [a]))

But they will also either fail or work erroneously.

All in all current state of affairs will eventually lead some users to some spectaculary confusing bugs.

Suor commented 9 years ago

The best solution I can see is supporting numbered placeholders and accurately shift their numbers:

'some_func($2, $1, $1)' -> 'some_func($5, $4, $4)'

Alternatively we can actively forbid $\d+ in sql, but that seems less intuitive.

prust commented 9 years ago

@Suor: Yeah, I agree the current state is confusing -- previously I wasn't sure that number-shifting makes sense in all situations (I thought that maybe the user would expect the numbering to start at $4 if they had already supplied 3 parameters), but now that I think about it a little more, you're probably right that number-shifting is more intuitive.

I'd like to write out a few examples/scenarios and make sure number-shifting is intuitive for them.

prust commented 8 years ago

Fixed in #85.