From 882f31aa9936b9ff5501aa8cb12e89d2c3e6c7e4 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 15 Feb 2023 10:10:07 -0800 Subject: [PATCH 1/5] Introducing IProblemDetailsService.TryWriteAsync --- .../ProblemDetails/IProblemDetailsService.cs | 21 +++++- .../src/PublicAPI.Unshipped.txt | 1 + .../src/ProblemDetailsService.cs | 45 +++++------- .../test/ProblemDetailsServiceTest.cs | 70 ++++++++++++++----- 4 files changed, 90 insertions(+), 47 deletions(-) diff --git a/src/Http/Http.Abstractions/src/ProblemDetails/IProblemDetailsService.cs b/src/Http/Http.Abstractions/src/ProblemDetails/IProblemDetailsService.cs index 038049680bde..aabc7f9490cb 100644 --- a/src/Http/Http.Abstractions/src/ProblemDetails/IProblemDetailsService.cs +++ b/src/Http/Http.Abstractions/src/ProblemDetails/IProblemDetailsService.cs @@ -10,7 +10,7 @@ namespace Microsoft.AspNetCore.Http; public interface IProblemDetailsService { /// - /// Try to write a response to the current context, + /// Write a response to the current context, /// using the registered services. /// /// The associated with the current request/response. @@ -21,5 +21,24 @@ public interface IProblemDetailsService /// All were executed and none of them was able to write the response successfully. /// /// + /// If no can write to the given context. ValueTask WriteAsync(ProblemDetailsContext context); + + /// + /// Try to write a response to the current context, + /// using the registered services. + /// + /// The associated with the current request/response. + /// The registered services + /// are processed in sequence and the processing is completed when: + /// + /// One of them reports that the response was written successfully, or. + /// All were executed and none of them was able to write the response successfully. + /// + /// + async ValueTask TryWriteAsync(ProblemDetailsContext context) + { + await WriteAsync(context); + return true; + } } diff --git a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt index 148bf26feed8..221dab728364 100644 --- a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt @@ -1,4 +1,5 @@ #nullable enable Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.ExecuteAsync(Microsoft.AspNetCore.Http.HttpContext! httpContext) -> System.Threading.Tasks.Task! +Microsoft.AspNetCore.Http.IProblemDetailsService.TryWriteAsync(Microsoft.AspNetCore.Http.ProblemDetailsContext! context) -> System.Threading.Tasks.ValueTask static Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.Instance.get -> Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult! \ No newline at end of file diff --git a/src/Http/Http.Extensions/src/ProblemDetailsService.cs b/src/Http/Http.Extensions/src/ProblemDetailsService.cs index 68002af77b96..11d06376da0f 100644 --- a/src/Http/Http.Extensions/src/ProblemDetailsService.cs +++ b/src/Http/Http.Extensions/src/ProblemDetailsService.cs @@ -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."); } + } - IProblemDetailsWriter? selectedWriter = null; - - if (_writers.Length == 1) - { - selectedWriter = _writers[0]; - - return selectedWriter.CanWrite(context) ? - selectedWriter.WriteAsync(context) : - ValueTask.CompletedTask; - } + public async ValueTask 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; } } diff --git a/src/Http/Http.Extensions/test/ProblemDetailsServiceTest.cs b/src/Http/Http.Extensions/test/ProblemDetailsServiceTest.cs index 66abd0de36a3..fcd54762a086 100644 --- a/src/Http/Http.Extensions/test/ProblemDetailsServiceTest.cs +++ b/src/Http/Http.Extensions/test/ProblemDetailsServiceTest.cs @@ -34,10 +34,18 @@ 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 + { + 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() { @@ -45,14 +53,30 @@ public async Task WriteAsync_Skip_WhenNoWriterRegistered() }; // 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(async () => await service.WriteAsync(new() { HttpContext = context })); } [Fact] - public async Task WriteAsync_Skip_WhenNoWriterCanWrite() + public async Task WriteAsync_Throws_WhenNoWriterCanWrite() { // Arrange var service = CreateService( @@ -63,19 +87,31 @@ public async Task WriteAsync_Skip_WhenNoWriterCanWrite() Response = { Body = stream, StatusCode = StatusCodes.Status400BadRequest }, }; + // Act & Assert + await Assert.ThrowsAsync(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( @@ -83,16 +119,14 @@ public async Task WriteAsync_Skip_WhenSuccessStatusCode(int statusCode) 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( From a7e79f1fd2bf53a999dc07fe180cf8a5e1a41513 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 15 Feb 2023 10:47:28 -0800 Subject: [PATCH 2/5] Change everywhere to use TryWriteAsync --- .../DeveloperExceptionPageMiddlewareImpl.cs | 15 ++------------- .../ExceptionHandlerMiddlewareImpl.cs | 5 +++-- .../src/StatusCodePage/StatusCodePagesOptions.cs | 15 ++++----------- .../src/Formatters/TextOutputFormatter.cs | 2 +- .../src/Infrastructure/ObjectResultExecutor.cs | 2 +- 5 files changed, 11 insertions(+), 28 deletions(-) diff --git a/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs b/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs index bb34c6d64597..a35883334826 100644 --- a/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs +++ b/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs @@ -172,19 +172,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"; diff --git a/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs b/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs index c6413f9f5c4c..62e06651f59d 100644 --- a/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs +++ b/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs @@ -142,13 +142,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, @@ -157,7 +158,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)) diff --git a/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesOptions.cs b/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesOptions.cs index 496e02d763e3..d3d946ab1763 100644 --- a/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesOptions.cs +++ b/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesOptions.cs @@ -23,19 +23,12 @@ public StatusCodePagesOptions() HandleAsync = async context => { var statusCode = context.HttpContext.Response.StatusCode; + var problemDetailsService = context.HttpContext.RequestServices.GetService(); - if (context.HttpContext.RequestServices.GetService() 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"; diff --git a/src/Mvc/Mvc.Core/src/Formatters/TextOutputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/TextOutputFormatter.cs index 10ec7bf26fed..3b82c08cbc46 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/TextOutputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/TextOutputFormatter.cs @@ -132,7 +132,7 @@ public override Task WriteAsync(OutputFormatterWriteContext context) if (context.HttpContext.RequestServices.GetService() is { } problemDetailsService) { - return problemDetailsService.WriteAsync(new () + return problemDetailsService.TryWriteAsync(new () { HttpContext = context.HttpContext, ProblemDetails = { Status = statusCode } diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/ObjectResultExecutor.cs b/src/Mvc/Mvc.Core/src/Infrastructure/ObjectResultExecutor.cs index 1bca41a29da4..cb13e3f93b62 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/ObjectResultExecutor.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/ObjectResultExecutor.cs @@ -103,7 +103,7 @@ private Task ExecuteAsyncCore(ActionContext context, ObjectResult result, Type? if (context.HttpContext.RequestServices.GetService() is { } problemDetailsService) { - return problemDetailsService.WriteAsync(new() + return problemDetailsService.TryWriteAsync(new() { HttpContext = context.HttpContext, ProblemDetails = { Status = statusCode } From 65e94a75d212c44db79da604a99332b72e376932 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 16 Feb 2023 15:39:18 -0800 Subject: [PATCH 3/5] API review updates --- .../src/ProblemDetails/IProblemDetailsService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Http.Abstractions/src/ProblemDetails/IProblemDetailsService.cs b/src/Http/Http.Abstractions/src/ProblemDetails/IProblemDetailsService.cs index aabc7f9490cb..1fe4a5433005 100644 --- a/src/Http/Http.Abstractions/src/ProblemDetails/IProblemDetailsService.cs +++ b/src/Http/Http.Abstractions/src/ProblemDetails/IProblemDetailsService.cs @@ -39,6 +39,6 @@ public interface IProblemDetailsService async ValueTask TryWriteAsync(ProblemDetailsContext context) { await WriteAsync(context); - return true; + return context.HttpContext.Response.HasStarted; } } From b91a1a1d3ded647d0f94f0303233b6aec27e4701 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 16 Feb 2023 15:42:10 -0800 Subject: [PATCH 4/5] Update PublicAPI.Unshipped.txt --- src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt index e1ad66f0c429..914e99a40703 100644 --- a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt @@ -1,6 +1,5 @@ #nullable enable const Microsoft.AspNetCore.Http.StatusCodes.Status499ClientClosedRequest = 499 -> int -static Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.Instance.get -> Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult! *REMOVED*Microsoft.AspNetCore.Http.DefaultEndpointFilterInvocationContext.DefaultEndpointFilterInvocationContext(Microsoft.AspNetCore.Http.HttpContext! httpContext, params object![]! arguments) -> void Microsoft.AspNetCore.Http.DefaultEndpointFilterInvocationContext.DefaultEndpointFilterInvocationContext(Microsoft.AspNetCore.Http.HttpContext! httpContext, params object?[]! arguments) -> void Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult @@ -8,3 +7,4 @@ Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.ExecuteAsync(Microsoft.Asp Microsoft.AspNetCore.Http.IProblemDetailsService.TryWriteAsync(Microsoft.AspNetCore.Http.ProblemDetailsContext! context) -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Http.HttpValidationProblemDetails.Errors.set -> void Microsoft.AspNetCore.Mvc.ProblemDetails.Extensions.set -> void +static Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.Instance.get -> Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult! From 5b9ffc77cc4f9c9a186e28d0ae3116d9e8c13f1e Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 16 Feb 2023 15:43:13 -0800 Subject: [PATCH 5/5] Update PublicAPI.Unshipped.txt --- src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt index 914e99a40703..2522d459b85f 100644 --- a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt @@ -4,7 +4,7 @@ const Microsoft.AspNetCore.Http.StatusCodes.Status499ClientClosedRequest = 499 - Microsoft.AspNetCore.Http.DefaultEndpointFilterInvocationContext.DefaultEndpointFilterInvocationContext(Microsoft.AspNetCore.Http.HttpContext! httpContext, params object?[]! arguments) -> void Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.ExecuteAsync(Microsoft.AspNetCore.Http.HttpContext! httpContext) -> System.Threading.Tasks.Task! -Microsoft.AspNetCore.Http.IProblemDetailsService.TryWriteAsync(Microsoft.AspNetCore.Http.ProblemDetailsContext! context) -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Http.HttpValidationProblemDetails.Errors.set -> void +Microsoft.AspNetCore.Http.IProblemDetailsService.TryWriteAsync(Microsoft.AspNetCore.Http.ProblemDetailsContext! context) -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Mvc.ProblemDetails.Extensions.set -> void static Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.Instance.get -> Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult!