Better error system #1002

tyt2y3 · 2022-08-28T05:00:54Z

Continue #750

@Sculas

* feat: Introducing feature "sqlx-error" Purpose of this feature is to not convert errors given from sqlx into strings to ease further analysis of the error and react to it accordingly. This implementation uses a feature switch and an additional error kind to avoid interfering with existing implementations without this feature enabled. See discussion #709 * fix: Align feature "sqlx-error" with merged Migration error kind Due to the merge, an other error kind had been introduced and the DbErr became Eq and Clone, however Eq cannot easily be derived from, so I went back to PartialEq, and since the sqlx error does not implement clone, this was converted into an Arc, to allow cloning of the new kind too. * fix: Repairing failing jobs Several jobs had failed as I missed to correct the return values of a few methods in transaction.rs and had a wrong understanding of map_err at that point. * feat: realigning with latest changes in sea-orm, different approach Instead of the former approach to introduce a new error kind, now the existing error types get extended, for now only Exec and Query, because these are the most relevant for the requirement context. Afterwards it might still be possible to add some further detail information. See discussion #709 * Update src/driver/sqlx_mysql.rs Integrating fixes done by @Sculas Co-authored-by: Sculas <[email protected]> * Update src/driver/sqlx_postgres.rs Integrating fixes done by @Sculas Co-authored-by: Sculas <[email protected]> * Update src/driver/sqlx_sqlite.rs Integrating fixes done by @Sculas Co-authored-by: Sculas <[email protected]> * feat: reworking feature with thiserror Following the latest suggestions I changed the implementation to utilize `thiserror`. Now there are more error kinds to be able to see the different kinds directly in src/error.rs To ensure the behaviour is as expected, I also introduce a further test, which checks for a uniqueness failure. Co-authored-by: Sculas <[email protected]>

tyt2y3 · 2022-08-28T05:01:07Z

@mohs8421

billy1624 · 2022-09-01T08:37:55Z

sea-orm-macros/src/derives/column.rs

-    let entity_name = data_enum
-        .variants
-        .first()
-        .map(|column| {
-            let column_iden = column.ident.clone();
-            quote!(
-                #ident::#column_iden.entity_name().to_string()
-            )
-        })
-        .unwrap();


Hey @tyt2y3, yeah, this isn't possible considering we can DeriveColumn on Enum that is not necessarily a Entity::Column.

billy1624

Good to go!
Thanks!! @mohs8421 @tyt2y3

Sculas

I fixed some typos and consistency in the error messages.

src/error.rs

Co-authored-by: Sculas <[email protected]>

Sculas · 2022-09-07T00:46:38Z

Any updates on this?

billy1624 · 2022-09-07T08:42:22Z

Hey @Sculas, we'll do a final review then merge it :)

tyt2y3 · 2022-09-07T16:00:10Z

Actually I strongly appreciate another pair of eyes to look at this

Sculas · 2022-09-07T16:04:07Z

Would an error like UniqueConstraintError be out of scope for SeaORM? If not, maybe some other uncommon errors too? Just to make it easier for the user. If it's not out of scope I'm willing to make a PR for it.

Sculas

I couldn't find anything out of the ordinary, so LGTM.

ikrivosheev

Small comment

src/error.rs

ikrivosheev · 2022-09-07T16:11:14Z

Would an error like UniqueConstraintError be out of scope for SeaORM? If not, maybe some other uncommon errors too? Just to make it easier for the user. If it's not out of scope I'm willing to make a PR for it.

This is needed very often when writing applications.

Sculas · 2022-09-07T16:15:13Z

This is needed very often when writing applications.

Yeah. I currently have some function that matches the error and checks if it's a unique constraint violation. It would be so much better if you could just check if it's a DbErr::UniqueConstraintViolation, for example.

billy1624 · 2022-09-08T04:25:48Z

I think UniqueConstraintError can be a variant of RuntimeErr.

mohs8421 · 2022-09-08T06:09:27Z

@Sculas that was my original intention with this discussion and the pull request in general. This pull request is now merely laying the foundation for what you suggest. There will be more after this one is done. See the discussion and my original PR.
But yes effectively speaking it will help a lot to have all those typical database runtime errors be reflected by seaorm, like fk-related errors or deadlocks which might even be circumvented by a retry mechanism.

tyt2y3 · 2022-09-13T16:28:40Z

Thanks for the suggestion @ikrivosheev

tyt2y3 · 2022-09-13T16:31:26Z

I think UniqueConstraintError is only applicable to Exec, not Query ?

ikrivosheev

@tyt2y3 LGTM! I think we can create: UniqueConstraintError in other PR.

Sculas · 2022-09-13T21:03:20Z

I think UniqueConstraintError is only applicable to Exec, not Query ?

That sqlx error that contains the unique constraint error is currently mapped to Query, so that would need to be changed.

My review in #750 mapped the error to Query: #750 (review) They should be mapped to Exec instead.

Sculas

LGTM. I agree with ikrivosheev.

mohs8421 · 2022-09-14T05:14:13Z

shortly regarding the name of errors:
I think in UniqueConstraintError the Error suffix seems to be noisy in my opinion, as it is already within the DbErr enum. I would rather rename it to UniqueConstraintViolation as that is more speaking.

billy1624 · 2022-09-14T07:36:12Z

I think UniqueConstraintError is only applicable to Exec, not Query ?

It should be applicable to Exec only. But I suspect update / insert operation with RETURNING is executed with query_one. So, the error might end up in DbErr::Query.

Sculas · 2022-09-27T12:23:05Z

Hey, any updates on this PR?

tyt2y3 · 2022-10-06T16:24:50Z

Thank you for your patience

* What's new in SeaORM 0.10.1 * Edit * Error handling (SeaQL/sea-orm#1002) * Support array datatype for Postgres (SeaQL/sea-orm#1132) * generate index file as `lib.rs` instead of `mod.rs` (SeaQL/sea-orm#953) * Running migration on any Postgres schema (SeaQL/sea-orm#1056) * More connection options (SeaQL/sea-orm#897, SeaQL/sea-orm#1056) * Implements `TryFrom<ActiveModel>` for `Model` (SeaQL/sea-orm#990) * Delete 2022-10-28-whats-new-in-0.10.1.md Co-authored-by: Chris Tsang <[email protected]>

mohs8421 and others added 2 commits August 28, 2022 11:13

Refactor Type Errors

0b754ea

tyt2y3 added 3 commits August 28, 2022 13:30

Refactor ColumnFromStrErr

348e841

Refactor SqlxError;

0ce0f49

Give up and fix tests

85533a3

tyt2y3 force-pushed the better-errors branch from eb85b79 to 85533a3 Compare August 28, 2022 06:55

tyt2y3 requested a review from billy1624 August 28, 2022 07:48

Typo

0efdfc6

billy1624 reviewed Sep 1, 2022

View reviewed changes

billy1624 added 3 commits September 1, 2022 16:38

.

4e835f2

Testing [issues] [cli]

6564dda

Fix [issues]

5f3210c

billy1624 approved these changes Sep 1, 2022

View reviewed changes

Sculas suggested changes Sep 2, 2022

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

tyt2y3 and others added 4 commits September 3, 2022 20:44

Update src/error.rs

1c8ef33

Co-authored-by: Sculas <[email protected]>

Update src/error.rs

4d95218

Co-authored-by: Sculas <[email protected]>

Update src/error.rs

4993c6a

Co-authored-by: Sculas <[email protected]>

Update src/error.rs

36f09c5

Co-authored-by: Sculas <[email protected]>

Sculas approved these changes Sep 7, 2022

View reviewed changes

ikrivosheev reviewed Sep 7, 2022

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

src/error.rs Show resolved Hide resolved

Address comments

1225255

ikrivosheev approved these changes Sep 13, 2022

View reviewed changes

Sculas approved these changes Sep 13, 2022

View reviewed changes

billy1624 assigned mohs8421, tyt2y3 and billy1624 Sep 14, 2022

Reorder variants

a44017f

tyt2y3 merged commit 5d752e6 into master Oct 6, 2022

tyt2y3 deleted the better-errors branch October 6, 2022 16:24

tyt2y3 mentioned this pull request Oct 9, 2022

PostgreSQL insert many with returning #1021

Merged

1 task

billy1624 added a commit to SeaQL/seaql.github.io that referenced this pull request Oct 31, 2022

Error handling (SeaQL/sea-orm#1002)

0ceaccb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error system #1002

Better error system #1002

tyt2y3 commented Aug 28, 2022

tyt2y3 commented Aug 28, 2022

billy1624 Sep 1, 2022

billy1624 left a comment

Sculas left a comment

Sculas commented Sep 7, 2022

billy1624 commented Sep 7, 2022

tyt2y3 commented Sep 7, 2022 •

edited

Loading

Sculas commented Sep 7, 2022

Sculas left a comment

ikrivosheev left a comment

ikrivosheev commented Sep 7, 2022

Sculas commented Sep 7, 2022

billy1624 commented Sep 8, 2022

mohs8421 commented Sep 8, 2022

tyt2y3 commented Sep 13, 2022

tyt2y3 commented Sep 13, 2022 •

edited

Loading

ikrivosheev left a comment

Sculas commented Sep 13, 2022 •

edited

Loading

Sculas left a comment

mohs8421 commented Sep 14, 2022

billy1624 commented Sep 14, 2022 •

edited

Loading

Sculas commented Sep 27, 2022

tyt2y3 commented Oct 6, 2022

Better error system #1002

Better error system #1002

Conversation

tyt2y3 commented Aug 28, 2022

tyt2y3 commented Aug 28, 2022

billy1624 Sep 1, 2022

Choose a reason for hiding this comment

billy1624 left a comment

Choose a reason for hiding this comment

Sculas left a comment

Choose a reason for hiding this comment

Sculas commented Sep 7, 2022

billy1624 commented Sep 7, 2022

tyt2y3 commented Sep 7, 2022 • edited Loading

Sculas commented Sep 7, 2022

Sculas left a comment

Choose a reason for hiding this comment

ikrivosheev left a comment

Choose a reason for hiding this comment

ikrivosheev commented Sep 7, 2022

Sculas commented Sep 7, 2022

billy1624 commented Sep 8, 2022

mohs8421 commented Sep 8, 2022

tyt2y3 commented Sep 13, 2022

tyt2y3 commented Sep 13, 2022 • edited Loading

ikrivosheev left a comment

Choose a reason for hiding this comment

Sculas commented Sep 13, 2022 • edited Loading

Sculas left a comment

Choose a reason for hiding this comment

mohs8421 commented Sep 14, 2022

billy1624 commented Sep 14, 2022 • edited Loading

Sculas commented Sep 27, 2022

tyt2y3 commented Oct 6, 2022

tyt2y3 commented Sep 7, 2022 •

edited

Loading

tyt2y3 commented Sep 13, 2022 •

edited

Loading

Sculas commented Sep 13, 2022 •

edited

Loading

billy1624 commented Sep 14, 2022 •

edited

Loading