MarimerLLC / csla

A home for your business logic in any .NET application.
https://cslanet.com
MIT License
1.26k stars 403 forks source link

Add option for data portal to throw "real" exception #1308

Closed rockfordlhotka closed 4 years ago

rockfordlhotka commented 5 years ago

This is something to consider.

Right now the client-side data portal pretty much always returns Csla.DataPortalException, with the original exception as an inner exception (or deeper - see the BusinessException property).

In many ways this is really nice, because it means the caller always gets an exception object that has the most detailed information we can know about the exception and complete stack trace - often all the way back onto the server (at least via ErrorInfo if we can't serialize the server-side exceptions).

However, it has the drawback that the caller can't use strongly typed catch blocks to deal with various exceptions (in C# - in VB the When expression actually solves this problem nicely).

In particular, what I am concerned about is whether frameworks like Polly can react to more sophisticated things like DataPortalException? I've only used Polly to handle a top-level exception, and have never looked to see if it can be triggered with deeper exception metadata.

IF it ends up making sense to expose the original exception, it would only be possible sometimes. We can't always serialize or recreate an original server-side exception, for example. Also, in any case we'd be throwing a new instance of the original exception, so the "true original" exception would still be deeper in the exception flow/stack trace.

I'd think that, in the case that we can/do throw an instance of the original exception type, that it would have an inner exception containing the DataPortalException, so we're sure not to lose the full stack trace and ErrorInfo from the entire data portal process flow.

rockfordlhotka commented 5 years ago

@JasonBock and @jonnybee I'd be very interested to hear your thoughts on whether this is a crazy idea or not.

TheCakeMonster commented 5 years ago

My 2 cents, if I may. Yes, it would be great if we could do something to remove the DataPortal from the stack trace, and give a bit easier access to the original type.

The exception messages we write to our logging subsystem are very complex, and difficult for someone doing support to understand; there's quite a lot of noise in the stack trace - quite apart from the type always being the same. I've learned to filter out this noise, but other people struggle with that. At the moment we are rotating people through the support role, so it's repeatedly painful for them to try to learn the bits of the trace that are useful, and the bits to ignore.

In an attempt to simplify the exceptions, I recently tried to unwrap the exception from the BusinessException property, and use that as the source of information to log. However, as you say, we lost the majority of the stack trace that way; we ended up with an exception that was so simple it wasn't actually useful - it didn't pin down where it happened, from the point of view of the consumer.

We currently use a local dataportal, so this isn't specific to the remote dataportal scenario - that's what I expected, but it doesn't work that way.

In short, I absolutely love the idea, but I can believe it would be pretty hard to achieve!

TheCakeMonster commented 5 years ago

I suppose there are 2 different parts to this; removing the DataPortal from the stack trace - that might be achievable, as there are a known number of levels (but much easier if the number of levels is consistent across different dataportal implementations, which I can imagine it won't be!)

Then there is the nirvana - the ultimate experience - which is exposing the original exception type.

Even if it were only possible to achieve one of these to begin with, it would be a really good step forward.

Chicagoan2016 commented 5 years ago

First a disclaimer, this only applies if you are using Enterprise library (I know ancient : )) or something similar. I started working on a project a few years ago and decided to use Enterprise library exception handling block with Csla ( the Application is still live). I decided to create several exception handling policies mostly to cover logging and re throwing a different exception to hide 'sensitive' information. e.g. In one application I would log the data access layer exception somewhere secure but re throw a more sanitized exception for the business layer, this was based on a certain exception handling policy. When the exception would get to business layer, it would decide what to do based on another exception handling policy and the same was true for UI layer. I had to make sure to use the 'BusinessException' property although frankly speaking not a big deal but the real exception would have made the code simpler.

GillesBer commented 5 years ago

With .NET Core, I'm assuming that we will have even less class that are shared between the client and the server. For instance (and I'm not saying here that we should send/disclose that level of detail to the client) but even SqlException is part of a thirdparty package that won't be available on the client. With .NET Full framework, it was in the Base Class Library shared with the client.

rockfordlhotka commented 5 years ago

That is correct @GillesBer - there will be many cases where the only valid exception we can throw is DataPortalException because we can't rethrow most server-side exceptions.

My personal primary concern is that I want to be able to use things like Polly on the client, and that probably requires having access to the real client-side exceptions (like a timeout or other connection-related exception, etc.).

The other way we could go though, would be to create a richer set of CSLA exceptions to cover many common scenarios.

For example, a CSLA KeyNotFound exception that would be thrown in place of any SQL/Oracle/Postgres/etc. equivalent exception.

JasonBock commented 5 years ago

Note that C# from 6.0 has filtered exceptions as well.

ajj7060 commented 5 years ago

Yes, this might be nice, as it would simplify exception handling. Many times we have to look at the DPE.BusinessException to find something we threw on the server, like a BONotFoundException or things that we make up. If there's a way to keep the server side stack trace that'd be ideal.

Although as Jason says C# has exception filtering which has helped with this issue quite a bit.

jonnybee commented 5 years ago

The Polly framework can handle both conditions and inner exceptions. See: https://github.com/App-vNext/Polly f.ex:

// Multiple exception types with condition
Policy
    .Handle<SqlException>(ex => ex.Number == 1205)
    .Or<ArgumentException>(ex => ex.ParamName == "example")

// Inner exceptions of ordinary exceptions or AggregateException, with or without conditions
// (HandleInner matches exceptions at both the top-level and inner exceptions)
Policy
    .HandleInner<HttpRequestException>()
    .OrInner<OperationCanceledException>(ex => ex.CancellationToken != myToken)

The feature sounds nice but my concern is when you share code on multiple platforms would you then need different compiler directives to handle different types of Exeptions coming back from the DataPortal? How confusing would it be to get a correct type exception in one client and the DPE on another client?

JasonBock commented 5 years ago

Here are some random thoughts....

I've never really thought too hard about this before. The only time this would have mattered in CSLA-based applications before were with DataPortal operations, and if an exception occurred, the exception was logged server-side, and usually on the client-side we didn't do anything specific logic-wise based on the exception returned.

That said, if you wanted to do something specific (for Polly-based retry logic if it was a network issue, for example), I could see where you'd want the specific exception type. But as @rockfordlhotka said, serialization with exceptions can be an issue. I really don't think CSLA wants to get in the business of having custom exceptions that could map to "well-known" scenarios or something similar, that seems like a big maintenance headache.

I think the "best" approach is to try and deserialize the exception client-side, and if that's successful, throw it. Otherwise, if it's not, then just raise the common DataPortalException or something similar to what's done today.

One final note: when I was toying with Orleans, I noticed that if an exception was raised in a grain server-side, it did come through into the client caller. But those were always "standard" .NET-based exceptions, so the definitions were always the same on either side.

rockfordlhotka commented 4 years ago

I am going to close this. Recent updates to DataPortalException in version 3.2 made this whole scenario much better (imo) and I don't see the value of this change offsetting the massive break in backward compatibility.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.