Add support for exceptions thrown by ExecuteUpdate and ExecuteDelete

[MarkusG]

Update failures from ExecuteUpdate() and ExecuteDelete() and their async variants will not be intercepted by SaveChangesInterceptor. To add support for errors thrown in this way, I have added a DbCommandInterceptor. This required refactoring the DatabaseError enum out of the interceptor class. As I understand it, this is not a breaking change.

Addresses #81

[Giorgi]

Thanks for the PR @MarkusG! I agree, it's a good idea to process exception thrown by ExecuteUpdate and ExecuteDelete

As for the implementation, what if there is a single class that implements IDbCommandInterceptor and ISaveChangesInterceptor interfaces? This way the DatabaseError class can stay nested and there will be less duplicate code.

[MarkusG]

That would require the interceptor to implement all methods, which would be a lot of extraneous code since all we want are SaveChangesFailed and CommandFailed and their async variants. I think, because we don't expect the number of interceptors to grow, the duplicate approach will be fine. If EF introduces a third way to execute commands, it might be worth trying to consolidate.

As for DatabaseError being a member of the interceptor class, what's the motivation for this as opposed to having it be a public standalone enum? Is there a need to distinguish between DatabaseError variants of different providers?

[Giorgi]

I haven't tried it but these interfaces have Default interface methods so we would only implement the ones that we are interested in.

https://github.com/dotnet/efcore/blob/main/src/EFCore/Diagnostics/ISaveChangesInterceptor.cs

https://github.com/dotnet/efcore/blob/main/src/EFCore.Relational/Diagnostics/IDbCommandInterceptor.cs

[MarkusG]

Interesting. The documentation's suggestion to inherit from the classes if not implementing all methods made me think implementing the interfaces would require implementations for all methods. In that case, I think a single interceptor implementing both interfaces is a great idea. Right now I'm working on tests for all the exceptions for ExecuteUpdate and ExecuteDelete, but after that I'll try my hand at a single interceptor class.

[MarkusG]

I saw there's a skipped SQLite test referencing an EF issue that has since been closed. I also noticed that SQLite appeared to not enforce the max length on a string. Do you have any insight into this @Giorgi?

Oracle has a different error code for inserting vs. updating null into a not-null column, so I'll add support for that in another PR.

[Giorgi]

The max length test is skipped because that EF Core issue was fixed in version 9. I'll re-enable that test when I upgrade to EF Core 9

Is the PR ready for review?

[MarkusG]

Ready for review. One question I still have is: Why is the DatabaseError enum nested in the interceptor class in the first place? Is there a reason to have a different type for each interceptor?

[Giorgi]

Probably because I didn't want to make it public not to pollute the public API.

[MarkusG]

That makes sense. It does make the ExceptionFactory class quite wide in the editor, but I can see why it's desirable to have the public API consist of only the interceptor and extension methods.

[Giorgi]

I should have some time at the end of the week to review it.

[Giorgi]

Looks good. I see that in many places method definitions and some code is split across multiple lines, you probably use ReSharper or another formatting tool. Can you put that code on the same lines as it was before? It makes it harder to read and see what's changed actually.

[Giorgi]

Thanks!