bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Jonas Bernoulli]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
>> More importantly though, please also include the actual error
>> message from SQLite in the error data.  Currently only a string
>> is included, which represents the _kind_ of error that occurred,
>> something like "SQL logic error".
>
> Ah, I missed that it was possible to get more data out of sqlite about
> what's wrong.
>
> I've now added the extra error string to the value.

Thanks!

You should probably do the same for sqlite-execute.

The functions that return error codes and messages are documented
at https://www.sqlite.org/c3ref/errcode.html and the error codes
at https://www.sqlite.org/rescode.html.

- sqlite3_errcode()
- sqlite3_extended_errcode()
  return the numeric result code or extended result code for that
  API call.
- sqlite3_errstr()
  returns the English-language text that describes the result code
- sqlite3_errmsg()
- sqlite3_errmsg16()
  return English-language text that describes error
- sqlite3_error_offset()

>> Thanks, but what about my suggestion to use a dedicated signal?
>> (Suggesting that different signals be used for different error
>> codes, may have been a bit excessive though.)
>
> I'm not sure adding a separate signal would be valuable here.

Currently sqlite.c intentionally withhold information from elisp,
needlessly making sophisticated error handling harder and less reliable.
Not using a separate signal is part of that.

I can see no benefit in withholding information.  Maybe you feel that no
user of sqlite.c would ever need/should implement more than rudimentary
error handling.  Currently my end-user packages only use rudimentary
error handling; basically they simply bail on any sql error.  (They use
sqlite.c via emacsql-sqlite-builtin.el.)

However as the new maintainer of EmacSQL, I would like to give users of
the new builtin backend the same feature sets as for the other backends,
not least because maybe some current or future users make use of that.

This is possible to an extend because EmacSQL wraps directly around the
call to sqlite-select and similar functions from different backends, so
it knows that every error it encounters there comes from the respective
backend and can then (in the case of sqlite-select) make an attempt to
decrypt the provided error data.

With the most recent change to that function it can, for example,
resignal (error "SQL logic error (no such table)") as (emacsql-error "no
such table: nono" 1).  To do this it has to extract the two pieces of
information from the one string and because we include the errcode in
the error data instead of the equivalent errstr, we have to maintain an
alist to translate from errstr to errcode.  Including the human readable
errstr is probably better than using the errcode, but changing that
would be a breaking change, so going forward I will provide both, which
actually is better than providing just either one: (emacsql-error "no
such table: nono" 1 "SQL logic error").  I think it would be a good idea
for sqlite.c to do the same.

But I am not just thinking of the needs of EmacSQL here.  In the future
I (and others) likely will use sqlite.c directly.  In order to implement
anything but rudimentary error handling, all those callers would have to
wrap sqlite.c's functions to resignal its errors.  Without doing that it
becomes impossible to reliably tell errors that originate from SQL (or
sqlite.c itself) apart from other errors.

So I would encourage you to always (i.e., not only in sqlite-select)
signal

  (sqlite-error sqlite_errstr() sqlite_errmsg() sqlite_errcode())

for any error originating from sqlite3_prepare_v2() or similar,
and e.g., 

  (sqlite-error "Invalid set object" nil nil)
or
  (sqlite-error "Invalid set object")

for errors originating from check_sqlite() and other places, where the
error doesn't originate from a call to sqlite3_prepare_v2() or similar.

By the way, for one particular error sqlite-execute already uses a
separate signal: Qsqlite_locked_error.  But only that function does it
and only for that one error.  That seems highly inconsistent.  I would
recommend removing this signal and replacing it with Qsqlite_error and
using that for every error and to always include all available data,
filling in nil when a particular piece is not available.

Actually, in the spirit of forward thinking, you might just as well
include the sqlite3_extended_errcode() and sqlite3_error_offset():

  (sqlite-error sqlite_errstr()
                sqlite_errmsg()
                sqlite_errcode()
                sqlite3_extended_errcode()
                sqlite3_error_offset())

     Thanks for considering,
     Jonas