Skip to content

Introducing IProblemDetailsService.TryWriteAsync #46680

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

Merged
merged 6 commits into from
Feb 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.AspNetCore.Http;
public interface IProblemDetailsService
{
/// <summary>
/// Try to write a <see cref="Mvc.ProblemDetails"/> response to the current context,
/// Write a <see cref="Mvc.ProblemDetails"/> response to the current context,
/// using the registered <see cref="IProblemDetailsWriter"/> services.
/// </summary>
/// <param name="context">The <see cref="ProblemDetailsContext"/> associated with the current request/response.</param>
Expand All @@ -21,5 +21,24 @@ public interface IProblemDetailsService
/// <item><description>All <see cref="IProblemDetailsWriter"/> were executed and none of them was able to write the response successfully.</description></item>
/// </list>
/// </remarks>
/// <exception cref="InvalidOperationException">If no <see cref="IProblemDetailsWriter"/> can write to the given context.</exception>
ValueTask WriteAsync(ProblemDetailsContext context);

/// <summary>
/// Try to write a <see cref="Mvc.ProblemDetails"/> response to the current context,
/// using the registered <see cref="IProblemDetailsWriter"/> services.
/// </summary>
/// <param name="context">The <see cref="ProblemDetailsContext"/> associated with the current request/response.</param>
/// <remarks>The <see cref="IProblemDetailsWriter"/> registered services
/// are processed in sequence and the processing is completed when:
/// <list type="bullet">
/// <item><description>One of them reports that the response was written successfully, or.</description></item>
/// <item><description>All <see cref="IProblemDetailsWriter"/> were executed and none of them was able to write the response successfully.</description></item>
/// </list>
/// </remarks>
async ValueTask<bool> TryWriteAsync(ProblemDetailsContext context)
{
await WriteAsync(context);
return context.HttpContext.Response.HasStarted;
}
}
1 change: 1 addition & 0 deletions src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@ Microsoft.AspNetCore.Http.DefaultEndpointFilterInvocationContext.DefaultEndpoint
Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult
Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.ExecuteAsync(Microsoft.AspNetCore.Http.HttpContext! httpContext) -> System.Threading.Tasks.Task!
Microsoft.AspNetCore.Http.HttpValidationProblemDetails.Errors.set -> void
Microsoft.AspNetCore.Http.IProblemDetailsService.TryWriteAsync(Microsoft.AspNetCore.Http.ProblemDetailsContext! context) -> System.Threading.Tasks.ValueTask<bool>
Microsoft.AspNetCore.Mvc.ProblemDetails.Extensions.set -> void
static Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.Instance.get -> Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult!
45 changes: 17 additions & 28 deletions src/Http/Http.Extensions/src/ProblemDetailsService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,44 +15,33 @@ public ProblemDetailsService(
_writers = writers.ToArray();
}

public ValueTask WriteAsync(ProblemDetailsContext context)
public async ValueTask WriteAsync(ProblemDetailsContext context)
{
ArgumentNullException.ThrowIfNull(context);
ArgumentNullException.ThrowIfNull(context.ProblemDetails);
ArgumentNullException.ThrowIfNull(context.HttpContext);

if (context.HttpContext.Response.HasStarted ||
context.HttpContext.Response.StatusCode < 400 ||
_writers.Length == 0)
if (!await TryWriteAsync(context))
{
return ValueTask.CompletedTask;
throw new InvalidOperationException("Unable to find a registered `IProblemDetailsWriter` that can write to the given context.");
Copy link
Member

Choose a reason for hiding this comment

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

Throwing is a breaking behavior change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is but I believe it is consistent with TryParse/Parse approach. However, I ok to not do that and avoid this breaking change

}
}

IProblemDetailsWriter? selectedWriter = null;

if (_writers.Length == 1)
{
selectedWriter = _writers[0];

return selectedWriter.CanWrite(context) ?
selectedWriter.WriteAsync(context) :
ValueTask.CompletedTask;
}
public async ValueTask<bool> TryWriteAsync(ProblemDetailsContext context)
{
ArgumentNullException.ThrowIfNull(context);
ArgumentNullException.ThrowIfNull(context.ProblemDetails);
ArgumentNullException.ThrowIfNull(context.HttpContext);

// Try to write using all registered writers
// sequentially and stop at the first one that
// `canWrite`.
for (var i = 0; i < _writers.Length; i++)
{
if (_writers[i].CanWrite(context))
var selectedWriter = _writers[i];
if (selectedWriter.CanWrite(context))
{
selectedWriter = _writers[i];
break;
await selectedWriter.WriteAsync(context);
return true;
}
}

if (selectedWriter != null)
{
return selectedWriter.WriteAsync(context);
}

return ValueTask.CompletedTask;
return false;
}
}
70 changes: 52 additions & 18 deletions src/Http/Http.Extensions/test/ProblemDetailsServiceTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,25 +34,49 @@ public async Task WriteAsync_Skip_NextWriters_WhenResponseAlreadyStarted()
}

[Fact]
public async Task WriteAsync_Skip_WhenNoWriterRegistered()
public async Task TryWriteAsync_ReturnsTrue_WhenAtLeastOneWriterCanWrite()
{
// Arrange
var service = CreateService();
var service = CreateService(
writers: new List<IProblemDetailsWriter>
{
new MetadataBasedWriter("FirstWriter", canWrite: false),
new MetadataBasedWriter("SecondWriter"),
new MetadataBasedWriter("FirstWriter"),
});

var metadata = new EndpointMetadataCollection(new SampleMetadata() { ContentType = "application/problem+json" });
var stream = new MemoryStream();
var context = new DefaultHttpContext()
{
Response = { Body = stream, StatusCode = StatusCodes.Status400BadRequest },
};

// Act
await service.WriteAsync(new() { HttpContext = context });
var result = await service.TryWriteAsync(new() { HttpContext = context, AdditionalMetadata = metadata });

// Assert
Assert.Equal(string.Empty, Encoding.UTF8.GetString(stream.ToArray()));
Assert.True(result);
Assert.Equal("\"SecondWriter\"", Encoding.UTF8.GetString(stream.ToArray()));
}

[Fact]
public async Task WriteAsync_Throws_WhenNoWriterRegistered()
{
// Arrange
var service = CreateService();
var stream = new MemoryStream();
var context = new DefaultHttpContext()
{
Response = { Body = stream, StatusCode = StatusCodes.Status400BadRequest },
};

// Act
await Assert.ThrowsAsync<InvalidOperationException>(async () => await service.WriteAsync(new() { HttpContext = context }));
}

[Fact]
public async Task WriteAsync_Skip_WhenNoWriterCanWrite()
public async Task WriteAsync_Throws_WhenNoWriterCanWrite()
{
// Arrange
var service = CreateService(
Expand All @@ -63,36 +87,46 @@ public async Task WriteAsync_Skip_WhenNoWriterCanWrite()
Response = { Body = stream, StatusCode = StatusCodes.Status400BadRequest },
};

// Act & Assert
await Assert.ThrowsAsync<InvalidOperationException>(async () => await service.WriteAsync(new() { HttpContext = context }));
}

[Fact]
public async Task TryWriteAsync_ReturnsFalse_WhenNoWriterRegistered()
{
// Arrange
var service = CreateService();
var stream = new MemoryStream();
var context = new DefaultHttpContext()
{
Response = { Body = stream, StatusCode = StatusCodes.Status400BadRequest },
};

// Act
await service.WriteAsync(new() { HttpContext = context });
var result = await service.TryWriteAsync(new() { HttpContext = context });

// Assert
Assert.False(result);
Assert.Equal(string.Empty, Encoding.UTF8.GetString(stream.ToArray()));
}

[Theory]
[InlineData(StatusCodes.Status100Continue)]
[InlineData(StatusCodes.Status200OK)]
[InlineData(StatusCodes.Status300MultipleChoices)]
[InlineData(399)]
public async Task WriteAsync_Skip_WhenSuccessStatusCode(int statusCode)
[Fact]
public async Task TryWriteAsync_ReturnsFalse_WhenNoWriterCanWrite()
{
// Arrange
var service = CreateService(
writers: new List<IProblemDetailsWriter> { new MetadataBasedWriter() });
var stream = new MemoryStream();
var context = new DefaultHttpContext()
{
Response = { Body = stream, StatusCode = statusCode },
Response = { Body = stream, StatusCode = StatusCodes.Status400BadRequest },
};
var metadata = new EndpointMetadataCollection(new SampleMetadata() { ContentType = "application/problem+json" });
context.SetEndpoint(new Endpoint(context => Task.CompletedTask, metadata, null));

// Act
await service.WriteAsync(new() { HttpContext = context });
// Act & Assert
var result = await service.TryWriteAsync(new() { HttpContext = context });

// Assert
Assert.Equal(string.Empty, Encoding.UTF8.GetString(stream.ToArray()));
Assert.False(result);
}

private static ProblemDetailsService CreateService(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,19 +184,8 @@ private async Task DisplayExceptionContent(ErrorContext errorContext)
{
var httpContext = errorContext.HttpContext;

if (_problemDetailsService != null)
{
var problemDetails = CreateProblemDetails(errorContext, httpContext);

await _problemDetailsService.WriteAsync(new()
{
HttpContext = httpContext,
ProblemDetails = problemDetails
});
}

// If the response has not started, assume the problem details was not written.
if (!httpContext.Response.HasStarted)
if (_problemDetailsService == null ||
!await _problemDetailsService.TryWriteAsync(new() { HttpContext = httpContext, ProblemDetails = CreateProblemDetails(errorContext, httpContext) }))
{
httpContext.Response.ContentType = "text/plain; charset=utf-8";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,14 @@ private async Task HandleException(HttpContext context, ExceptionDispatchInfo ed
context.Response.StatusCode = DefaultStatusCode;
context.Response.OnStarting(_clearCacheHeadersDelegate, context.Response);

var problemDetailsWritten = false;
if (_options.ExceptionHandler != null)
{
await _options.ExceptionHandler!(context);
}
else
{
await _problemDetailsService!.WriteAsync(new ()
problemDetailsWritten = await _problemDetailsService!.TryWriteAsync(new()
{
HttpContext = context,
AdditionalMetadata = exceptionHandlerFeature.Endpoint?.Metadata,
Expand All @@ -169,7 +170,7 @@ private async Task HandleException(HttpContext context, ExceptionDispatchInfo ed
}

// If the response has already started, assume exception handler was successful.
if (context.Response.HasStarted || context.Response.StatusCode != StatusCodes.Status404NotFound || _options.AllowStatusCode404Response)
if (context.Response.HasStarted || problemDetailsWritten || context.Response.StatusCode != StatusCodes.Status404NotFound || _options.AllowStatusCode404Response)
{
const string eventName = "Microsoft.AspNetCore.Diagnostics.HandledException";
if (_diagnosticListener.IsEnabled() && _diagnosticListener.IsEnabled(eventName))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,12 @@ public StatusCodePagesOptions()
HandleAsync = async context =>
{
var statusCode = context.HttpContext.Response.StatusCode;
var problemDetailsService = context.HttpContext.RequestServices.GetService<IProblemDetailsService>();

if (context.HttpContext.RequestServices.GetService<IProblemDetailsService>() is { } problemDetailsService)
{
await problemDetailsService.WriteAsync(new ()
{
HttpContext = context.HttpContext,
ProblemDetails = { Status = statusCode }
});
}

// TODO: Render with a pre-compiled html razor view.
if (!context.HttpContext.Response.HasStarted)
if (problemDetailsService == null ||
!await problemDetailsService.TryWriteAsync(new() { HttpContext = context.HttpContext, ProblemDetails = { Status = statusCode } }))
{
// TODO: Render with a pre-compiled html razor view
var body = BuildResponseBody(statusCode);

context.HttpContext.Response.ContentType = "text/plain";
Expand Down
2 changes: 1 addition & 1 deletion src/Mvc/Mvc.Core/src/Formatters/TextOutputFormatter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public override Task WriteAsync(OutputFormatterWriteContext context)

if (context.HttpContext.RequestServices.GetService<IProblemDetailsService>() is { } problemDetailsService)
{
return problemDetailsService.WriteAsync(new ()
return problemDetailsService.TryWriteAsync(new ()
{
HttpContext = context.HttpContext,
ProblemDetails = { Status = statusCode }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ private Task ExecuteAsyncCore(ActionContext context, ObjectResult result, Type?

if (context.HttpContext.RequestServices.GetService<IProblemDetailsService>() is { } problemDetailsService)
{
return problemDetailsService.WriteAsync(new()
return problemDetailsService.TryWriteAsync(new()
{
HttpContext = context.HttpContext,
ProblemDetails = { Status = statusCode }
Expand Down