Removed enum for ExceptionType #2803
Conversation
Reverted change related to LastKnownGood.
| } | ||
|
|
||
| if (TokenUtilities.IsRecoverableErrorType(result.UnwrapError().Type)) | ||
| if (TokenUtilities.IsRecoverableException(result.UnwrapError().GetException())) |
There was a problem hiding this comment.
Why are we allocating the exception when we could simply check the type and save the allocation?
| validationParameters.RefreshBeforeValidation = false; | ||
| validationParameters.ValidateWithLKG = true; | ||
| ExceptionType recoverableExceptionType = result.UnwrapError().Type; | ||
| Exception recoverableExceptionType = result.UnwrapError().GetException(); |
There was a problem hiding this comment.
Same question around allocating the exception.
| /// </summary> | ||
| /// <returns>An instantance of an Exception.</returns> | ||
| public Exception GetException() | ||
| public virtual Exception GetException() |
There was a problem hiding this comment.
The Activator used in this PR: #2800, is more extensible than this approach. Why are we resorting to still using a bunch of ifs checking for the different exceptions?
One more thing to consider is that the ArgumentNullException not inheriting from SecurityTokenException and therefore doesn't have a stack trace.
|
This PR is re-doing the work done in a previous PR, #2800. I am not seeing the point on merging these changes over the linked PR. I am happy to take the approach to initialise exceptions with the ExceptionDetail presented here onto that one. |
|
Added changes to #2801. Will pull in additional work around adding ExceptionDetails to SecurityTokenException. |
The enums that map to an Exception to create have been removed. This creates a dependency in the code, benchmarks show very little change.
Reverted change related to LastKnownGood in that modified the release call graph.
Introduced a model for a derived ExceptionDetail coordinated with an additional ctor for Exceptions.