Skip to content

Set IExceptionHandlerFeature on DeveloperExceptionPageMiddleware #52688

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Kahbazi
Copy link
Member

@Kahbazi Kahbazi commented Dec 9, 2023

Fixes #49195

@ghost ghost added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member labels Dec 9, 2023
@ghost
Copy link

ghost commented Dec 9, 2023

Thanks for your PR, @Kahbazi. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@Kahbazi Kahbazi added feature-diagnostics Diagnostic middleware and pages (except EF diagnostics) and removed area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Dec 9, 2023
Comment on lines 321 to 356
// Arrange
using var host = new HostBuilder()
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseTestServer()
.Configure(app =>
{
app.Use(async (context, next) =>
{
await next();

var exceptionHandlerFeature = context.Features.GetRequiredFeature<IExceptionHandlerPathFeature>();

Assert.NotNull(exceptionHandlerFeature);
Assert.Equal("Test exception", exceptionHandlerFeature.Error.Message);
Assert.Equal(context.Request.Path, exceptionHandlerFeature.Path);
});
app.UseExceptionHandler(exceptionApp =>
{
exceptionApp.Run(context => Task.CompletedTask);
});
app.Run(context =>
{
throw new Exception("Test exception");
});

});
}).Build();

await host.StartAsync();

var server = host.GetTestServer();
var request = new HttpRequestMessage(HttpMethod.Get, "/path");

var response = await server.CreateClient().SendAsync(request);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test isn't forcing the asserts to run. Move asserts into the main path of the test:

Suggested change
// Arrange
using var host = new HostBuilder()
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseTestServer()
.Configure(app =>
{
app.Use(async (context, next) =>
{
await next();
var exceptionHandlerFeature = context.Features.GetRequiredFeature<IExceptionHandlerPathFeature>();
Assert.NotNull(exceptionHandlerFeature);
Assert.Equal("Test exception", exceptionHandlerFeature.Error.Message);
Assert.Equal(context.Request.Path, exceptionHandlerFeature.Path);
});
app.UseExceptionHandler(exceptionApp =>
{
exceptionApp.Run(context => Task.CompletedTask);
});
app.Run(context =>
{
throw new Exception("Test exception");
});
});
}).Build();
await host.StartAsync();
var server = host.GetTestServer();
var request = new HttpRequestMessage(HttpMethod.Get, "/path");
var response = await server.CreateClient().SendAsync(request);
// Arrange
var tcs = new TaskCompletionSource<IExceptionHandlerPathFeature>(TaskCreationOptions.RunContinuationsAsynchronously);
using var host = new HostBuilder()
.ConfigureWebHost(webHostBuilder =>
{
webHostBuilder
.UseTestServer()
.Configure(app =>
{
app.Use(async (context, next) =>
{
await next();
var exceptionHandlerFeature = context.Features.GetRequiredFeature<IExceptionHandlerPathFeature>();
tcs.SetResult(exceptionHandlerFeature);
});
app.UseExceptionHandler(exceptionApp =>
{
exceptionApp.Run(context => Task.CompletedTask);
});
app.Run(context =>
{
throw new Exception("Test exception");
});
});
}).Build();
await host.StartAsync();
var server = host.GetTestServer();
var request = new HttpRequestMessage(HttpMethod.Get, "/path");
var response = await server.CreateClient().SendAsync(request);
var feature = await tcs.Task;
Assert.NotNull(feature);
Assert.Equal("Test exception", feature.Error.Message);
Assert.Equal(context.Request.Path, feature.Path);

(note: changed in a text field, not might not compile, and/or have bugs)

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 26, 2023
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment Feb 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@wtgodbe wtgodbe removed the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
Copy link
Contributor

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 20, 2024
@adityamandaleeka
Copy link
Member

@Kahbazi Are you still interested in working on this?

@Kahbazi
Copy link
Member Author

Kahbazi commented Jun 24, 2024

@adityamandaleeka Sorry I missed this PR, I will work on it this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares community-contribution Indicates that the PR has been added by a community member feature-diagnostics Diagnostic middleware and pages (except EF diagnostics) pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Always set IExceptionHandlerFeature from UseDeveloperExceptionPage
5 participants