-
Notifications
You must be signed in to change notification settings - Fork 97
Add Roslyn analyzer to detect improper catching of DotvvmInterruptRequestExecutionException #1965
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…cutionException Co-authored-by: tomasherceg <5599524+tomasherceg@users.noreply.github.com>
Co-authored-by: tomasherceg <5599524+tomasherceg@users.noreply.github.com>
src/Analyzers/Analyzers/ApiUsage/DotvvmInterruptExceptionAnalyzer.cs
Outdated
Show resolved
Hide resolved
|
@copilot Please look at my review comments and make the requested changes. |
…es, and nested try blocks Co-authored-by: tomasherceg <5599524+tomasherceg@users.noreply.github.com>
| foreach (var catchClause in tryOperation.Catches) | ||
| { | ||
| var caughtType = catchClause.ExceptionType; | ||
| if (caughtType == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Does this mean that if there is just catch { ... }, we just ignore it and report that the exception is handled correctly? Can you verify this by adding a test for the catch clause without the specified exception type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct - the original code was skipping bare catch { } blocks entirely. I've now fixed this by handling bare catch blocks conservatively: they always trigger the analyzer warning, regardless of whether they contain a throw; statement. This is because we cannot reliably detect rethrows in bare catch blocks through Roslyn's operation tree. Users should use catch (DotvvmInterruptRequestExecutionException) { throw; } instead for proper handling. Added tests for bare catch blocks with and without rethrows - both now correctly trigger warnings. Commit: 94f1b1a
…atic Co-authored-by: tomasherceg <5599524+tomasherceg@users.noreply.github.com>
Users wrapping
ReturnFile*orRedirect*calls in try-catch blocks inadvertently swallowDotvvmInterruptRequestExecutionException, breaking request flow control.Changes
ReturnFile*orRedirectTo*methods onIDotvvmRequestContextare called within try-catch blocks that catchDotvvmInterruptRequestExecutionException(or base types likeException) without rethrowingthrow;anywhere in catch block, including inside if statements or other nested operationswhen (ex is not DotvvmInterruptRequestExecutionException))catch { }blocks (without exception type) are always flagged as problematic since we cannot reliably detect rethrows within them. Users should use explicit exception types instead.Example
The analyzer flags these patterns:
And accepts these patterns:
Warning
<issue_title>Roslyn analyzer to detect catching DotvvmInterruptRequestExecutionException</issue_title>
<issue_description>A couple of users were dealing with this unintuitive behavior - when they perform a redirect or returning a file, they wrap it in a try/catch block, and catch all exceptions. However, DotVVM throws
DotvvmInterruptRequestExecutionExceptionwhich needs to be rethrown.We already have Roslyn analyzers in the project. We should detect this pattern when
ReturnFile*orRedirect*is called onIDotvvmRequestContext, and if it is inside try/catch block, ensure there is a special case forDotvvmInterruptRequestExecutionExceptionwhere the exception is rethrown as such:<issue_title>Roslyn analyzer to detect catching DotvvmInterruptRequestExecutionException</issue_title>
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.