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

More detailed error types #72

Closed
aantron opened this issue Jul 4, 2021 · 8 comments
Closed

More detailed error types #72

aantron opened this issue Jul 4, 2021 · 8 comments

Comments

@aantron
Copy link
Contributor

aantron commented Jul 4, 2021

For example, upon a UNIQUE constraint error, Caqti exposes a Caqti_error.t which looks like this after show:

Response from <sqlite3:db.sqlite> failed: CONSTRAINT. Query: <snip>

The actual value is

`Response_failed _

The msg field of _ could be either...

  • Caqti_error.Msg, which carries a string, and is therefore not useful for pattern matching. The string would have to be parsed instead.
  • Caqti_driver_sqlite3.Rc carrying, presumably, Sqlite3.CONSTRAINT. This would be useful, but:
    • Caqti_driver_sqlite3.Rc is not exposed AFAICT.
    • We actually want something portable to all the database drivers, not just SQLite3.
    • It doesn't specify that the constraint violation was of a UNIQUE constraint.

Is there a way to get more detail in the error types? Since at the moment, all we can reasonably match on is `Response_failed _, all we essentially can know is that "some error occurred."

On the subject of detail on CONSTRAINT, by contrast, @thangngoc89 reports in aantron/dream#86 that sqlite3 cli gives errors like

Error: UNIQUE constraint failed: user.email

Ideally, we would have a detailed database-agnostic error type, and translate database-specific errors in a thorough way into it.

@paurkedal
Copy link
Owner

Yes, the database specific errors are only exposed as strings today. It would be possible to reveal them, but the user would have to link against each specific driver. So, I agree a better solution would be to translate the errors into an agnostic type. I also think it would be useful to classify the errors based on the kind of issue, the recipient of the error, and whether something can be done about it (like retry in case of timeout or connection failure).

I only had a brief look this far and considering the number of errors for MariaDB and PostgreSQL it looks like a major task to classify and unite them. In comparison Sqlite3 seems to have few enumerated conditions, maybe even insufficient for what you suggest? But focusing on the former two, there is a common "SQLSTATE". It's not exposed in the PostgreSQL API, but the documentation suggests it present, so we can make a PR for that. In MariaDB the error is represented as int * string. I wonder if that string is the SQLSTATE, otherwise there is at least a translation. Many of the conditions, however, translates to the fall-back HY000. So, I am not sure if we can rely on the SQLSTATE classification.

On the side, I am a bit suspicious of using errors to detect unique constraint or other violations as a normal control flow. Though, maybe there isn't an easy alternative which works across database systems, except paying the overhead of querying up-front within the same transaction.

@thangngoc89
Copy link

If you look at SQLITE3 documentation, you can see that error codes are documented clearly with subclasses. I don’t think sqlite3 driver exposed the subclasses

https://www.sqlite.org/rescode.html#constraint_unique

@tcoopman
Copy link

tcoopman commented Jul 6, 2021

On the side, I am a bit suspicious of using errors to detect unique constraint or other violations as a normal control flow.

Isn't that actually the accepted way of doing this? You set a unique constraint and the database keeps it. Getting a unique violation tells you exactly what you need to know

@paurkedal
Copy link
Owner

paurkedal commented Jul 17, 2021

To outline the task:

  • Extend PostgreSQL bindings to expose the SQLSTATE, possibly supplemented by a variant. Update: This is already implemented in the error_field method.
  • Extend the MariaDB bindings to expose the SQLSTATE. We have the error code from the result. Expose sqlstate in the connection and stmt APIs. ocaml-community/ocaml-mariadb#42. Update: While it may be nice to have, the chosen approach does not rely on SQLSTATE.
  • Extend the Sqlite3 bindings to expose the refined error codes. Extended error codes mmottl/sqlite3-ocaml#49.
  • Decide on how the errors will be exposed in Caqti and whether and how to try to unify them.
  • Implement it in Caqti. Update: Missing extended error codes for sqlite.

@paurkedal
Copy link
Owner

My rough thoughts on the Caqti interface is to expose them as a field in query_error, or possibly accessible only by functions so that the representation can depend on the driver. What we can easily expose is an optional SQLSTATE in raw form and maybe an optional driver-dependent integer error code as a limited-use fall-back. As for the less easy but much nicer API, I consider making a grant variant containing all cases for all drivers and then creating variant aliases of groups of cases which one is interested in matching. What I don't like about this approach is that it exposes too much of the specifics of each database system, but on the other hand I am not sure how feasible it will be to map them into a really unified set of errors, given that many of the errors are really specific to the database system.

@paurkedal
Copy link
Owner

On the side, I am a bit suspicious of using errors to detect unique constraint or other violations as a normal control flow.

Isn't that actually the accepted way of doing this? You set a unique constraint and the database keeps it. Getting a unique > violation tells you exactly what you need to know

@tcoopman It probably is. I usually try to avoid triggering errors, which has been useful in cases when I want to run a while series of updates in a transaction, since an error would abort the transaction. But that sometimes involves additional queries, within the transaction, or the use of not universally available ON CONFLICT clauses. And in case of concurrent operation, the transaction could fail, in which case a more detailed error will also be needed to handle it gracefully.

@paurkedal
Copy link
Owner

I'm reconsidering my approach here. PostgreSQL provides SQLSTATE at a useful level, but MariaDB maps many errors to too generic classes, esp. to a fallback HY000, so I think we would need to redo the mapping to make it useful. Sqlite3 has no SQLSTATE support. But, creating a mapping for MariaDB and Sqlite3 turned out to be a tiring process with limited success. I pushed some limited mapping to https://github.com/paurkedal/ocaml-caqti/tree/error-sqlstate in case anyone wants to see a meagre start.

My thinking now is that if we're not going for anything close to the full SQLSTATE, we may as well make something which is more ergonomic for OCaml code. So, I made about the same mapping to a variant type, https://github.com/paurkedal/ocaml-caqti/tree/error-cause. Example:

(match Caqti_error.cause error with
 | `Foreign_key_violation -> handle_foreign_key_violation ()
 | #integrity_constraint_violation -> handle_other_constraint_violation ()
 | _ -> handle_other_error ())

We don't have the extended sqlite3 error codes yet, so any constraint violation ends up on the #integrity_constraint_violation branch. I am also unsure about some of the MariaDB error codes, since the manual only states the error messages and not the precise circumstances when the error occurs.

@paurkedal
Copy link
Owner

I closed this before having sufficient bindings for sqlite3 to discriminate between different types of constraint violations. The refinement of the error cause variant is now also available for sqlite3 in master (ed71ade) (using Sqlite3.extended_errcode_int).

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

No branches or pull requests

4 participants