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.

Sign up for GitHub

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

Jump to bottom

feat: better database errors #2109

Merged
merged 9 commits into from
Feb 8, 2023
Merged

feat: better database errors #2109

merged 9 commits into from
Feb 8, 2023

Conversation

saiintbrisson
Copy link
Contributor

@saiintbrisson saiintbrisson commented Sep 21, 2022 •
edited
Loading

In light of problems I faced when error handling a back-end agnostic code, I've came up with an interface that lets the user figure out what kind of error the database returned, which fixes #2102.

The new DatabaseError::kind(&self) function tries to match the error code with a known kind, taking into account the back-end specific ways it might be returned. Four error kinds are supported for now: unique, foreign key, not null, and check violations.

SQLite and Postgres are pretty straight forward, MySQL has multiple codes for each kind of violation, but MSSQL has a funny way of reporting checks and foreign key violations, it uses the same error code and differentiates the two in the message.

I've performed tests and all four kinds are supported for all back-ends currently supported, including check and foreign key for MSSQL.

@jacob-pro
Copy link

This looks brilliant to me.
Any idea on if this will be merged soon @abonander ?

@abonander
Copy link
Collaborator

@saiintbrisson current development is on the 0.7-dev branch which just had some major refactors merged. Do you mind rebasing?

@saiintbrisson
Copy link
Contributor Author

Sure! Was waiting until the refac was over, will do!

@saiintbrisson saiintbrisson force-pushed the feat/2102-better-database-errors branch from 46f343c to 0b94f50 Compare February 2, 2023 04:12
@saiintbrisson saiintbrisson changed the base branch from main to 0.7-dev February 2, 2023 04:14
@saiintbrisson saiintbrisson marked this pull request as draft February 2, 2023 04:14
@saiintbrisson saiintbrisson force-pushed the feat/2102-better-database-errors branch 2 times, most recently from 15d5683 to 2c9415c Compare February 2, 2023 04:18
@saiintbrisson saiintbrisson marked this pull request as ready for review February 2, 2023 04:24
@saiintbrisson
Copy link
Contributor Author

saiintbrisson commented Feb 2, 2023 •
edited
Loading

@abonander seems like we are good to go. I've deleted the MSSQL error implementation, but I'm wondering where I should put it, will the "pro" crate be available soon?

EDIT: Important note: this PR fixes the x.py test suite and removes MSSQL from the docker compose file. Should I leave it as is or move it to another PR first?

@abonander
Copy link
Collaborator

abonander commented Feb 3, 2023 •
edited
Loading

You can just delete the MSSQL stuff, we'll be redoing that from scratch anyway.

x.py I honestly never touch as I don't understand how to use it 🤣, it was written by @mehcode ages ago. I prefer to run tests directly in CLion so I can attach a debugger if I want to, and let Github Actions deal with running the full suite. If it's useful to you I'm fine leaving the fixes in this PR.

@saiintbrisson saiintbrisson force-pushed the feat/2102-better-database-errors branch from 2c9415c to 3df5653 Compare February 3, 2023 23:14
@saiintbrisson
Copy link
Contributor Author

saiintbrisson commented Feb 3, 2023 •
edited
Loading

Right! Dropped the MSSQL commit @abonander.

x.py is indeed confusing, there's probably a better way to write a test suite. And as to the remaining MSSQL tests, should I follow up with a PR deleting them?

@abonander
Copy link
Collaborator

I'll delete them, they're just dead code.

x.py is supposed to be a runner for common build/test commands, there's a similar script in rust-lang/rust that inspired it. If people are actually using it I'm fine keeping it but I don't see much value in it myself.

@abonander
Copy link
Collaborator

@saiintbrisson sadly Transaction cannot implement Executor anymore due to coherence issues: https://github.com/launchbadge/sqlx/actions/runs/4088645789/jobs/7050682422#step:7:665

You should be able to just insert a dereference, e.g. .execute(&mut *tx)

@saiintbrisson saiintbrisson force-pushed the feat/2102-better-database-errors branch 2 times, most recently from 4da070b to 749a5b2 Compare February 3, 2023 23:55
@saiintbrisson
Copy link
Contributor Author

@abonander Makes sense, I was expecting this to happen, and it did with sqlite, but x.py really does not work correctly hahaha

@saiintbrisson
Copy link
Contributor Author

@abonander MySQL has this weird functionality where CHECK constraints didn't exist before version 8.0, and the 5.7 server just "ignored it", we can't run tests for this version or any MariaDB. I'm not sure how to proceed, as the test isn't aware of the server version it's connecting to, any ideas?
SQLite also requires a PRAGMA foreign_keys = ON on every connection that wants to ensure foreign key consistency.

@abonander
Copy link
Collaborator

I believe we do send PRAGMA foreign_keys = ON by default for SQLite, cause having it off is kinda pointless for new applications.

As for MySQL, we've used #[cfg] flags to only enable tests when running against newer database versions in CI, you can duplicate what we did for Postgres to MySQL:

It looks like 5.7 is slowly getting EOL'd as it's not supported on newer operating systems: https://www.mysql.com/support/supportedplatforms/database.html

I'll be glad when we can drop it entirely cause it's kinda been a pain.

@saiintbrisson saiintbrisson force-pushed the feat/2102-better-database-errors branch from 749a5b2 to 8f64a53 Compare February 4, 2023 03:20
@saiintbrisson saiintbrisson force-pushed the feat/2102-better-database-errors branch from 8f64a53 to 8ac8749 Compare February 4, 2023 03:20
@saiintbrisson
Copy link
Contributor Author

@abonander I've re-ran all the tests locally and they seem to be working fine

abonander
abonander reviewed Feb 8, 2023
View reviewed changes
sqlx-core/src/error.rs Outdated Show resolved Hide resolved
@abonander abonander merged commit 5e8fff1 into launchbadge:0.7-dev Feb 8, 2023
@saiintbrisson saiintbrisson mentioned this pull request Feb 9, 2023
Closed
@saiintbrisson saiintbrisson deleted the feat/2102-better-database-errors branch February 9, 2023 12:59
abonander pushed a commit that referenced this pull request Feb 18, 2023
@saiintbrisson @abonander
feat: better database errors (#2109)
b21f4d2 
* feat(core): create error kind enum

* feat(core): add error kind for postgres

* feat(core): add error kind for sqlite

* feat(core): add error kind for mysql

* test(postgres): add error tests

* test(sqlite): add error tests

* test(mysql): add error tests

* fix(tests): fix tests rebasing

* refac(errors): add `ErrorKind::Other` variant
abonander pushed a commit that referenced this pull request Feb 21, 2023
@saiintbrisson @abonander
feat: better database errors (#2109)
c095328 
* feat(core): create error kind enum

* feat(core): add error kind for postgres

* feat(core): add error kind for sqlite

* feat(core): add error kind for mysql

* test(postgres): add error tests

* test(sqlite): add error tests

* test(mysql): add error tests

* fix(tests): fix tests rebasing

* refac(errors): add `ErrorKind::Other` variant
Aandreba pushed a commit to Aandreba/sqlx that referenced this pull request Mar 31, 2023
@saiintbrisson @Aandreba
feat: better database errors (launchbadge#2109)
91a5110 
* feat(core): create error kind enum

* feat(core): add error kind for postgres

* feat(core): add error kind for sqlite

* feat(core): add error kind for mysql

* test(postgres): add error tests

* test(sqlite): add error tests

* test(mysql): add error tests

* fix(tests): fix tests rebasing

* refac(errors): add `ErrorKind::Other` variant
@saiintbrisson saiintbrisson mentioned this pull request Jul 14, 2023
Merged
@raviqqe raviqqe mentioned this pull request Aug 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abonander abonander abonander approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feat: better database error support
3 participants
@saiintbrisson @jacob-pro @abonander