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

Feat: better database error support #2102

Closed
saiintbrisson opened this issue Sep 19, 2022 · 7 comments · Fixed by #2109
Closed

Feat: better database error support #2102

saiintbrisson opened this issue Sep 19, 2022 · 7 comments · Fixed by #2109
Labels
enhancement New feature or request

Comments

@saiintbrisson
Copy link
Contributor

saiintbrisson commented Sep 19, 2022

Is your feature request related to a problem? Please describe.
I've been adding support for SQLx to an error handling crate and I got stuck when processing database errors because the crate aims to be agnostic, and I'd have to manually compare SQLSTATEs to check if the error is a unique violation, check violation, etc. It'd be nice if we had some solution akin to Diesel's DatabaseErrorKind.

While it's possible to do this via DatabaseError::kind, this handling is something that I've had to do multiple times in the past, and removing the burden off of the user would be nice.

Describe the solution you'd like
The solution boils down to mapping the SQLSTATE returned by the DB to an enum variant. We'd need constants with the SQLSTATEs for the drivers already supported (PostgreSQL, MySQL, SQLite, and MSSQL). The function that would perform this mapping would be implemented via DatabaseError and returns an Option<Enum> (drivers may not support the same SQLSTATEs):

enum ErrorKind {
  UniqueViolation,
}

trait DatabaseError {
  // ...
  fn kind(&self) -> Option<ErrorKind>;
}

impl DatabaseError for PgDatabaseError {
  // ...
  fn kind(&self) -> Option<ErrorKind> {
    match self.code()? {
      "23505" => Some(ErrorKind::UniqueViolation),
      _ => None,
    }
  }
}

Describe alternatives you've considered
Create a feature for each driver SQLx provides support for and manually comparing the error codes for each database on the crate I'm working with.

I'm keen on working on this, guidance is welcome!

@saiintbrisson saiintbrisson added the enhancement New feature or request label Sep 19, 2022
@saiintbrisson
Copy link
Contributor Author

There is also the table function which is local to the PostgreSQL back-end, and it'd be great to be able to access it in a generic way, but I'm asking for too much now 🥷

@abonander
Copy link
Collaborator

As context for how we handle this in our applications (as SQLx is a core component of many Launchbadge projects), we usually have a little extension trait over Result like this: https://github.com/launchbadge/realworld-axum-sqlx/blob/main/src/http/error.rs#L173

It's much easier to make this ergonomic in a non-generic context like an application where the final error type is known ahead of time. And it only handles constraint violations because that's really the only kind of error we care to recover from.

That interface wouldn't adapt to MySQL very well, however, as the error response doesn't include the constraint name as a separate field. I suppose it wouldn't be too difficult to parse it from the error message, although in this case I would reverse it and do a substring search for the constraint name in the error message.

@saiintbrisson
Copy link
Contributor Author

saiintbrisson commented Sep 20, 2022

That's how I do it as well, and it works fine when I know which driver is being used. I've parsed the error message in the past, but I can't guarantee it will have the same message features across all back-ends. Also, the constraint name isn't required (in my case at least), only the SQLSTATE kind.

The problem I'm aiming to tackle is when we add support for SQLx in agnostic code (for instance, this is the crate I'm working on). In my case, I'll have to add some logic that accounts for the possible drivers the code may be using.

@abonander
Copy link
Collaborator

I could see possibly introducing a kind() method and enum, though I think there should also be some convenience methods like:

fn is_constraint_violation(&self) -> bool;

fn is_unique_violation(&self) -> bool;

fn is_check_violation(&self) -> bool;

added to the DatabaseError trait.

@saiintbrisson
Copy link
Contributor Author

I'll sketch out a rough idea in a PR and mention you for ideas. Are you fine with this?

@abonander
Copy link
Collaborator

Yeah, feel free.

@saiintbrisson
Copy link
Contributor Author

#2109 was merged and will be available in the next minor release (0.7).

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

Successfully merging a pull request may close this issue.

2 participants