Closed nurpax closed 12 years ago
This check makes sense. If too few parameters are given to bind
, the remaining parameters will default to null. If too many parameters are given to bind
it will produce an ErrorRange error.
However, I'm not sure if we should be doing it at this level of abstraction. Thoughts?
A quick summary of SQLite3's parameters and bindings:
sqlite3_bind_parameter_count
returns the index of the highest-numbered parameter. For example:
> import Database.SQLite3
> conn <- open ":memory:"
> bindParameterCount =<< prepare conn "SELECT ?1, ?5, ?3, ?"
6
Note that ?
takes the highest-numbered parameter plus one. This applies to named parameters like ?a
, too.
If a parameter is not bound, its default value is null.
> stmt <- prepare conn "SELECT ?1, ?5, ?3, ?"
> bindDouble stmt 3 2.71828
> step stmt
Row
> columns stmt
[SQLNull,SQLNull,SQLFloat 2.71828,SQLNull]
If a bind references a parameter whose index is > bindParameterCount, it produces an SQLITE_RANGE
error. Otherwise, it does not complain, even if the parameter is unused:
> stmt <- prepare conn "SELECT ?1, ?3, ?5"
> bindDouble stmt 6 2.71828
*** Exception: user error (SQLite3 returned ErrorRange while attempting to perform bind double.)
> bindDouble stmt 2 2.71828 -- unused parameter that is in range
> step stmt
Row
> columns stmt
[SQLNull,SQLNull,SQLNull]
I figured that it'd make sense to check at this level since the 'bind' function is a convenience function added in direct-sqlite. My sqlite-simple will do a similar check on top of direct-sqlite, so this error check is certainly not something I rely on. I filed the bug when I noticed that this error case was not handled but don't feel strongly about this check.
The pull request adds a couple of (trivial) tests for parameter binding, so even if the error check would be dropped, the tests would still be useful to merge.
I found a somewhat compelling argument in favor of checking the number of items passed to bind
:
> import Database.SQLite3
> conn <- open ":memory:"
> stmt <- prepare conn "SELECT ?, ?, ?"
> bind stmt $ map SQLInteger [1,2,3]
> step stmt >> columns stmt
[SQLInteger 1,SQLInteger 2,SQLInteger 3]
> reset stmt
> bind stmt $ map SQLInteger [4,5]
> step stmt >> columns stmt
[SQLInteger 4,SQLInteger 5,SQLInteger 3] -- the 3 is a stale value from the last bind
Since sqlite3_reset does not clear the parameters, if a subsequent bind has a short count, then there will be stale parameters from the last bind.
This is pretty bad. It could lead to security holes. Here are a couple solutions:
bind
to be given a short list, but have it call sqlite3_clear_bindings first.bind
to have the correct number of arguments.I'm leaning in favor of the second, which this pull request implements. This catches potential mistakes sooner.
I vote for this pull request to be accepted.
Yes, seems simple enough and clearly correct. Good! :)
Fail with an error rather than let sqlite convert missing parameters to NULLs.
Add tests for the above.