Throw an error if # of bind params doesn't the query (#2)

[nurpax]

Fail with an error rather than let sqlite convert missing parameters
to NULLs.

Add tests for the above.

[joeyadams]

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]
  

[nurpax]

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.

[joeyadams]

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:

• Allow bind to be given a short list, but have it call sqlite3_clear_bindings first.
• Require 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.

[IreneKnapp]

Yes, seems simple enough and clearly correct. Good! :)