Carter icon indicating copy to clipboard operation
Carter copied to clipboard

IResponseNegotiator not terminating response and falling through to ExecuteReturnAsync.WriteJsonResponseAsync

Open toddsmith-adsk opened this issue 1 year ago • 8 comments

I found that the default middleware in .NET 9 Minimal API is executing the following code below. When using IResponseNegotiator if I don't return an IResult object from my method it falls through to the WriteJsonResponseAsync call and then throws an exception about headers being read-only.

https://github.com/dotnet/aspnetcore/blob/1770dcf4e81872395cc4d3b3b3efbaef91f8020a/src/Shared/RouteHandlers/ExecuteHandlerHelper.cs#L27

   public static Task ExecuteReturnAsync(object obj, HttpContext httpContext, JsonTypeInfo<object> jsonTypeInfo)
    {
        // Terminal built ins
        if (obj is IResult result)
        {
            return result.ExecuteAsync(httpContext);
        }
        else if (obj is string stringValue)
        {
            SetPlaintextContentType(httpContext);
            return httpContext.Response.WriteAsync(stringValue);
        }
        else
        {
            // Otherwise, we JSON serialize when we reach the terminal state
            return WriteJsonResponseAsync(httpContext.Response, obj, jsonTypeInfo);
        }
    }

So instead of returning

return response.Negotiate(data);

I have to wrap the response in an IResult wrapper so that it hits the 1st conditional block in ExecuteReturnAsync.

return Results.Extensions.Negotiated(response.Negotiate(data));
public static class NegotiatedResultExtensions
{
    public static IResult Negotiated(this IResultExtensions _, Task obj)
    {
        return new NegotiatedResult(obj);
    }

    private class NegotiatedResult : IResult
    {
        private readonly Task _item;
    
        public NegotiatedResult(Task item)
        {
            _item = item;
        }

        public Task ExecuteAsync(HttpContext httpContext)
        {
            return _item;
        }
    }

toddsmith-adsk avatar Nov 23 '24 19:11 toddsmith-adsk

I assume this is an issue only for IAsyncEnumerable

On Sat, 23 Nov 2024 at 19:45, toddsmith-adsk @.***> wrote:

I found that the default middleware in .NET 9 Minimal API is executing the following code below. When using IResponseNegotiator if I don't return an IResult object from my method it falls through to the WriteJsonResponseAsync call and then throws an exception about headers being read-only.

https://github.com/dotnet/aspnetcore/blob/1770dcf4e81872395cc4d3b3b3efbaef91f8020a/src/Shared/RouteHandlers/ExecuteHandlerHelper.cs#L27

public static Task ExecuteReturnAsync(object obj, HttpContext httpContext, JsonTypeInfo jsonTypeInfo) { // Terminal built ins if (obj is IResult result) { return result.ExecuteAsync(httpContext); } else if (obj is string stringValue) { SetPlaintextContentType(httpContext); return httpContext.Response.WriteAsync(stringValue); } else { // Otherwise, we JSON serialize when we reach the terminal state return WriteJsonResponseAsync(httpContext.Response, obj, jsonTypeInfo); } }

So instead of returning

return response.Negotiate(data);

I have to wrap the response in an IResult wrapper so that it hits the 1st conditional block in ExecuteReturnAsync.

return Results.Extensions.Negotiated(response.Negotiate(data));

public static class NegotiatedResultExtensions { public static IResult Negotiated(this IResultExtensions _, Task obj) { return new NegotiatedResult(obj); }

private class NegotiatedResult : IResult
{
    private readonly Task _item;

    public NegotiatedResult(Task item)
    {
        _item = item;
    }

    public Task ExecuteAsync(HttpContext httpContext)
    {
        return _item;
    }
}

— Reply to this email directly, view it on GitHub https://github.com/CarterCommunity/Carter/issues/370, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZVJSVBQGMV3HDODGY7XD2CDLMPAVCNFSM6AAAAABSLMSAZSVHI2DSMVQWIX3LMV43ASLTON2WKOZSGY4DMNJRGI2TIMA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

jchannon avatar Nov 23 '24 21:11 jchannon

I assume this is an issue only for IAsyncEnumerable

This is for a regular non-IAsyncEnumerable response.

toddsmith-adsk avatar Nov 23 '24 22:11 toddsmith-adsk

As far as I can see non IAsyncEnumerable is working fine.

For IAsyncEnumerable, Negotiate will just work with the default json negotiator, for example:

app.MapGet("/asyncenumerable", (HttpResponse res) =>
            {
                var enumerable = RangeAsync(10, 3);
                
                return res.Negotiate(enumerable);
            });

 static async IAsyncEnumerable<int> RangeAsync(int start, int count)
        {
            for (var i = 0; i < count; i++)
            {
                await Task.Delay(i);
                yield return start + i;
            }
        }

The above will return "[10,11,12]" as the body, if you want a negotiator to do something else then you can choose to do that yourself, for example,

public class AsyncEnumerableResponseNegotiator : IResponseNegotiator
{
    public bool CanHandle(MediaTypeHeaderValue accept)
    {
        return true;
    }

    public async Task Handle<T>(HttpRequest req, HttpResponse res, T model, CancellationToken cancellationToken)
    {
        var enumerable = (IAsyncEnumerable<int>)model;

        await foreach (var item in enumerable)
        {
            await res.WriteAsync(item.ToString(), cancellationToken);
        }
    }
}

jchannon avatar Nov 25 '24 11:11 jchannon

I'm using Negotiate to return MemoryPack serialization and not json. I'll try and make a small demo.

toddsmith-adsk avatar Nov 26 '24 15:11 toddsmith-adsk

Here is a demo of the issue

carter-memorypack-api

toddsmith-adsk avatar Nov 26 '24 17:11 toddsmith-adsk

Interestingly this doesn't cause an exception for me on macOS .NET9 and I get binary data in the response that I can view in a file

I did find this which maybe of interest but it produces the same data as your example https://github.com/Cysharp/MemoryPack/blob/main/src/MemoryPack.AspNetCoreMvcFormatter/MemoryPackOutputFormatter.cs

jchannon avatar Nov 26 '24 20:11 jchannon

You tested both the / and /result endpoints and got the same result? The / endpoint is the one that should generate an error on my windows box running Visual Studio 2022.

toddsmith-adsk avatar Nov 26 '24 23:11 toddsmith-adsk

Yup. Both worked

On Wed, 27 Nov 2024 at 00:21, toddsmith-adsk @.***> wrote:

You tested both the / and /result endpoints and got the same result? The / endpoint is the one that should generate an error on my windows box running Visual Studio 2022.

— Reply to this email directly, view it on GitHub https://github.com/CarterCommunity/Carter/issues/370#issuecomment-2502167315, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZVJS5HUEMVVBDJI7WUGL2CT67VAVCNFSM6AAAAABSLMSAZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBSGE3DOMZRGU . You are receiving this because you commented.Message ID: @.***>

jchannon avatar Nov 27 '24 06:11 jchannon