Add ExceptionHandled to ExceptionContext #4825
Conversation
hint: I'd suggest first investigating MVC5's behavior. We found that we were missing this from the API surface of previous versions. |
| @@ -69,6 +69,11 @@ public virtual ExceptionDispatchInfo ExceptionDispatchInfo | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Gets or sets an indication that the <see cref="Exception"/> has been handled. | |||
| /// </summary> | |||
| public virtual bool ExceptionHandled { get; set; } = false; | |||
There was a problem hiding this comment.
Why virtual? I don't think there's an easy way to replace the exception context with your own, and being get set I don't see how making it virtual would improve unit testing which is the other scenario I would consider. @rynowak am I missing something?
There was a problem hiding this comment.
All of the properties on all of these classes are virtual for mocking support
|
@rynowak after reviewing how we did this in MVC5 I've made an update. I believe this would be a breaking change for any exception filters that currently handle exceptions as they'd need to start marking |
|
Do you want to explain what the delta in behavior is and why this is terrifying you? I need some deets. |
ryanbrandenburg
force-pushed
the
rybrande/ExceptionHandled
branch
from
90fa7c6 to
d57f2d0
Compare
|
🆙📅 The major change from RC2 here (other than just adding the property) is that we set ExceptionHandled to true when handling an exception, rather than nulling the exception. This is consistent with the behavior I observed in MVC5. One thing that has me a bit confused is that yesterday in person I thought I remembered you saying that user code wouldn't be handling this interaction (previously nulling |
| @@ -422,7 +422,7 @@ private Task InvokeAllExceptionFiltersAsync() | |||
|
|
|||
| _diagnosticSource.AfterOnExceptionAsync(_exceptionContext, current.FilterAsync); | |||
|
|
|||
| if (_exceptionContext.Exception == null) | |||
| if (_exceptionContext.ExceptionHandled) | |||
There was a problem hiding this comment.
Why is this condition different from the other ones? Think what would happen if you set the .Exception to null. Would other exception filters run?
|
⌚ |
|
🆙📅 |
| .Setup(f => f.OnException(It.IsAny<ExceptionContext>())) | ||
| .Callback<ExceptionContext>(context => | ||
| { | ||
| filter2.ToString(); |
There was a problem hiding this comment.
These calls to ToString() aren't needed. This is an ancient workaround for an issue in Moq. Just don't put them in new code
|
|
after removing those
ryanbrandenburg
force-pushed
the
rybrande/ExceptionHandled
branch
from
7262434 to
913a7eb
Compare
ryanbrandenburg
force-pushed
the
rybrande/ExceptionHandled
branch
from
913a7eb to
27bde12
Compare
Fixes #4700.
@rynowak please check me extra well since I'm not sure I correctly interpreted the behavior/functionality of this area.