khellang / Middleware

Various ASP.NET Core middleware
MIT License
807 stars 111 forks source link

ProblemDetails middleware when Response has started #4

Closed lurumad closed 6 years ago

lurumad commented 6 years ago

Hi @khellang

I have one scenario that I would like to support with ProblemDetails. I usually create a unit of work middleware to make request transactional and consistent:

public class UnitOfWorkMiddleware
{
    private readonly RequestDelegate next;

    public UnitOfWorkMiddleware(RequestDelegate next)
    {
        this.next = next;
    }

    public async Task Invoke(HttpContext context)
    {
        await next(context);
        var requestMethod = context.Request.Method;
        var isSafeMethod = requestMethod == HttpMethods.Get || requestMethod == HttpMethods.Head;
        var dbContext = context.RequestServices.GetService(typeof(EFDbContext)) as EFDbContext;
        if (IsSuccessStatusCode(context.Response) && !isSafeMethod)
        {
            await dbContext.SaveChangesAsync();
        }
    }

    private bool IsSuccessStatusCode(HttpResponse response)
    {
        return response.StatusCode >= 200 && response.StatusCode <= 299;
    }
}

In case that SaveChanges() fail, problems details detect the exception but the response has started and rethrow the exception, but in this case I would like to continue using Problem Details.

One question related with this line https://github.com/khellang/Middleware/blob/4ff1f88d86573be0545ee40f60f01362568742b6/src/ProblemDetails/ProblemDetailsMiddleware.cs#L84

Why check if response has been started and not catch all exceptions? Can you tell me some scenario when you have to re throw the exception and not send a problems details to the client?

Actually I don't know to manage my scenario in the right way.

Regards!

khellang commented 6 years ago

Why check if response has been started and not catch all exceptions?

Because the whole point of the middleware is to send an RFC7807 response to the client. If the response has already started, it's too late for the middleware to do anything. That's why it logs and rethrows the exception, in case anyone else is interested in it, like some other error handler or logging middleware.

khellang commented 6 years ago

If you want to make sure the response hasn't started before saving, you should register a callback using context.Response.OnStarting and save the changes there.

lurumad commented 6 years ago

Hi @khellang

Make sense, I've change my UoW middleware:

context.Response.OnStarting(async state =>
{
    var httpContext = (HttpContext)state;
    var requestMethod = httpContext.Request.Method;
    var isSafeMethod = requestMethod == HttpMethods.Get || requestMethod == HttpMethods.Head;
    var dbContext = httpContext.RequestServices.GetService(typeof(EFDbContext)) as EFDbContext;
    if (IsSuccessStatusCode(httpContext.Response) && !isSafeMethod)
    {
        await dbContext.SaveChangesAsync();
    }
}, context);   

await next(context);

image

I've debug ProblemsDetails source code because I've received that error:

System.ObjectDisposedException: The response has been aborted due to an unhandled application exception. ---> System.InvalidOperationException: The entity of type 'Channel' is sharing the table 'Configuration.Channels' with entities of type 'Channel.AudienceChannelId#NaturalNumber', but there is no entity of this type with the same key value that has been marked as 'Added'. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the key values. at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.Validate(Dictionary2 sharedTablesCommandsMap) at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.CreateModificationCommands(IReadOnlyList1 entries, Func1 generateParameterName) at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.BatchCommands(IReadOnlyList1 entries)+MoveNext() at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.ExecuteAsync(DbContext _, ValueTuple2 parameters, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func4 operation, Func4 verifySucceeded, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(IReadOnlyList1 entriesToSave, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken) at Microsoft.EntityFrameworkCore.DbContext.SaveChangesAsync(Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken) at Schedule.Api.Infrastructure.UnitOfWorkMiddleware.b__2_0(Object state) in C:\Dev\SistemaParrillaPlain\src\Schedule\Schedule.Api\Infrastructure\Middlewares\UnitOfWorkMiddleware.cs:line 26 at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.FireOnStartingAwaited(Task currentTask, Stack1 onStarting) --- End of inner exception stack trace --- at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ThrowResponseAbortedException() at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.InitializeResponseAwaited(Task startingTask, Int32 firstWriteByteCount) at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.WriteAsyncAwaited(Task initializeTask, ReadOnlyMemory1 data, CancellationToken cancellationToken) at Microsoft.AspNetCore.WebUtilities.HttpResponseStreamWriter.FlushInternalAsync(Boolean flushEncoder) at Microsoft.AspNetCore.Mvc.Formatters.JsonOutputFormatter.WriteResponseBodyAsync(OutputFormatterWriteContext context, Encoding selectedEncoding) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeResultAsync(IActionResult result) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResultFilterAsync[TFilter,TFilterAsync]() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResultExecutedContext context) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeResultFilters() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync() at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext) at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext) at Microsoft.AspNetCore.Builder.Extensions.MapWhenMiddleware.Invoke(HttpContext context) at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext) at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context) at Schedule.Api.Infrastructure.UnitOfWorkMiddleware.Invoke(HttpContext context) in C:\Dev\SistemaParrillaPlain\src\Schedule\Schedule.Api\Infrastructure\Middlewares\UnitOfWorkMiddleware.cs:line 30 at Hellang.Middleware.ProblemDetails.ProblemDetailsMiddleware.Invoke(HttpContext context) in C:\Dev\ClassLibrary1\ProblemDetailsMiddleware.cs:line 63

Any ideas?

Regards!

khellang commented 6 years ago

Hmm, yeah, the problem seems to be that the DbContext has been disposed by the time the OnStarting callback is invoked.

Do you really need this to be custom middleware? I think it would be much easier if this was part of the MVC filter pipeline instead. That would let you save your changes before the result is written to the response body, without callback gymnastics.

lurumad commented 6 years ago

@khellang definitively you save my day. In next MVP Summit I'll pay you some beers!

I've changed my approach to a ActionFilter and works like a charm:

public class UnitOfWorkFilterAttribute : ActionFilterAttribute
{
    private readonly ScheduleDbContext dbContext;

    public UnitOfWorkFilterAttribute(ScheduleDbContext dbContext)
    {
        this.dbContext = dbContext ?? throw new System.ArgumentNullException(nameof(dbContext));
    }

    public override async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
    {
        var result = await next.Invoke();
        var requestMethod = context.HttpContext.Request.Method;
        var isSafeMethod = requestMethod == HttpMethods.Get || requestMethod == HttpMethods.Head;
        if (result.Exception == null && !isSafeMethod)
        {
            await dbContext.SaveChangesAsync();
        }
    }
}

Regards!

khellang commented 6 years ago

Awesome. Glad to hear! 😁