Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Database errors need to be more matchable [needs upstream rework] #86

Open
aantron opened this issue Jul 1, 2021 · 4 comments
Open

Comments

@aantron
Copy link
Owner

aantron commented Jul 1, 2021

@thangngoc89 The Dream program at the bottom of this message shows the closest I was able to get to actually matching on a precise SQL error (a UNIQUE constraint violation) using Caqti. The relevant snippet is:

        try%lwt
          Dream.sql request (add_comment 1)
        with Caqti_error.Exn (`Response_failed _ as error) as exn ->
          Dream.log "CAUGHT %s" (Caqti_error.show error);
          raise exn

This program at least confirms that it caught the right exception. In the log output, there is

01.07.21 14:09:52.666                       REQ 1 CAUGHT Response from <sqlite3:db.sqlite> failed: CONSTRAINT. Query: "INSERT INTO comment (foo) VALUES (?)".

It doesn't appear that you can get any more precise error pattern than in this at the moment:

  • `Response_failed has a Caqti_error.query_error as a payload. The uri and query fields are not interesting here. They are just the DB connection string and the query we sent, of course. That leaves the msg field.
  • The msg field is an extensible variant, with only one constructor defined by the base Caqti, which just contains an opaque string.
  • Caqti_driver_sqlite3 defines an additional constructor Caqti_driver_sqlite3.Rc which carries an ocaml-sqlite3 return code, which at least has a Sqlite3.CONSTRAINT variand. However, Rc is not exposed in the interface of Caqti_driver_sqlite3, so, to my knowledge, there is currently no way to match on it in a pattern.
  • That leaves only some slightly awkward string conversion functions.

So, in summary, it seems that one has to, at the moment, convert the error to a string and parse the string to get any more details.

There is another problem, that even if we did match for Sqlite3.CONSTRAINT, the code would not be portable to other DB drivers, so this would not be a satisfactory solution anyway.

We really need some kind of interpretation of driver errors at the Caqti level. Also, it should all be nicely documented, so that most users don't have to read .mlis like I just did :)

I'm going to do a bit more digging and report this upstream to Caqti, to see if this is something that was considered and just not addressed yet. I'll post a link to that issue once it exists.


module type DB = Caqti_lwt.CONNECTION
module R = Caqti_request
module T = Caqti_type

let create_table =
  let query =
    R.exec T.unit
      {|CREATE TABLE comment (
          id INTEGER PRIMARY KEY AUTOINCREMENT,
          foo INTEGER UNIQUE NOT NULL)
      |} in
  fun (module Db : DB) ->
    let%lwt unit_or_error = Db.exec query () in
    Caqti_lwt.or_fail unit_or_error

let add_comment =
  let query =
    R.exec T.int
      "INSERT INTO comment (foo) VALUES ($1)" in
  fun text (module Db : DB) ->
    let%lwt unit_or_error = Db.exec query text in
    Caqti_lwt.or_fail unit_or_error

let () =
  Dream.run
  @@ Dream.logger
  @@ Dream.sql_pool "sqlite3:db.sqlite"
  @@ Dream.router [

    Dream.get "/" (fun request ->
      let%lwt () =
        try%lwt
          Dream.sql request (add_comment 1)
        with Caqti_error.Exn (`Response_failed _ as error) as exn ->
          Dream.log "CAUGHT %s" (Caqti_error.show error);
          raise exn
      in
      Dream.html "ok");

    Dream.get "/init" (fun request ->
      let%lwt () = Dream.sql request create_table in
      Dream.html "ok");

  ]
  @@ Dream.not_found
@aantron
Copy link
Owner Author

aantron commented Jul 1, 2021

(Also leaving myself a note that it might be good to create an actual example in this repo out of the program, or at least paste the code for catching errors into the existing SQL example).

@thangngoc89
Copy link
Contributor

thangngoc89 commented Jul 1, 2021

I think this needs to be solved upstream. The output error is kinda bad.

failed: CONSTRAINT

This doesn't give you any context about what fails. There could be many constraint in a complex query.

sqlite3 cli and other GUI client gives much more details error:

Error: UNIQUE constraint failed: user.email

@aantron aantron added this to the alpha3 milestone Jul 1, 2021
@aantron aantron changed the title Database errors need to be more matchable Database errors need to be more matchable [needs upstream rework] Jul 3, 2021
@aantron
Copy link
Owner Author

aantron commented Jul 4, 2021

Opened paurkedal/ocaml-caqti#72 about this in Caqti.

@aantron aantron removed this from the alpha3 milestone Jul 4, 2021
@aantron
Copy link
Owner Author

aantron commented Mar 30, 2022

It looks like Caqti 1.8.0 has some support for this. I haven't had a chance to try it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants