Results / TypedResults ignore null values

[Dryvnt]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

When using Minimal APIs with nullable annotations enabled, it is possible to make a delegate that returns a null value for a nullable-annotated type, e.g.

// Responds 200: "null"
app.MapGet("/nullable", T? () => null);

If the same value is instead wrapped in a Results or TypedResults container, the response body will be empty, e.g.

// Responds 200: ""
app.MapGet("/iresult", IResult () => Results.Ok<T?>(null));
app.MapGet("/typed_result", Ok<T?> () => TypedResults.Ok<T?>(null));

This means changing an endpoint delegate to use these result wrappers causes a breakage in the API.

T can be both a value or reference type, the problem still occurs.

Expected Behavior

Wrapping a return value in Results / TypedResults of the same status should respond with the exact same body as the value by itself.

Explicitly setting the status code in an idiomatic way should not cause a breaking change in the API.

// Responds 200: "null"
app.MapGet("/nullable", T? () => null);
app.MapGet("/iresult", IResult () => Results.Ok<T?>(null));
app.MapGet("/typed_result", Ok<T?> () => TypedResults.Ok<T?>(null));

Current behavior is acceptable in case null is an invalid value for the type.

// Programmer error, T is not nullable. Nasal demons may occur.
app.MapGet("/", Ok<T> () => TypedResults.Ok<T>(null!));

Steps To Reproduce

using Microsoft.AspNetCore.Http.HttpResults;

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();
app.MapGet("/nullable", Data? () => Data.NullInstance);
app.MapGet("/iresult", IResult () => Results.Ok<Data?>(Data.NullInstance));
app.MapGet("/typed_result", Ok<Data?> () => TypedResults.Ok<Data?>(Data.NullInstance));
// Semantically adjacent endpoints, for the sake of discussion
app.MapGet("/void", void () => {});
app.MapGet("/string", string () => "null");
await app.StartAsync();

var client = new HttpClient { BaseAddress = new Uri(app.Urls.First()), };
var endpoints = new[] { "/nullable", "/iresult", "/typed_result", "/void", "/string", };
foreach (var e in endpoints)
{
    var r = await client.GetAsync(e);
    var s = r.StatusCode;
    var c = await r.Content.ReadAsStringAsync();
    var contentType = r.Content.Headers.ContentType;
    Console.WriteLine($"{e} -> {s} ({contentType}): `{c}`");
}

await app.StopAsync();

public record Data
{
    public static Data? NullInstance => null;
}

Prints:

/nullable -> OK (application/json; charset=utf-8): `null`
/iresult -> OK (): ``
/typed_result -> OK (): ``
/void -> OK (): ``
/string -> OK (text/plain; charset=utf-8): `null`

Exceptions (if any)

No response

.NET Version

7.0.203

Anything else?

.NET SDK:
 Version:   7.0.203
 Commit:    5b005c19f5

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.22621
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\7.0.203\

Host:
  Version:      8.0.0-preview.5.23280.8
  Architecture: x64
  Commit:       bc78804f5d

.NET SDKs installed:
  6.0.410 [C:\Program Files\dotnet\sdk]
  7.0.107 [C:\Program Files\dotnet\sdk]
  7.0.203 [C:\Program Files\dotnet\sdk]
  8.0.100-preview.5.23303.2 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 6.0.18 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.0-preview.5.23302.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.14 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.16 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.18 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.0-preview.5.23280.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.18 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 7.0.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 7.0.7 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 8.0.0-preview.5.23302.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found:
  None

Environment variables:
  Not set

global.json file:
  C:\Users\ThomasGravgaardHanse\RiderProjects\ProofOfConcept\global.json

[mitchdenny]

Thanks for reporting this @Dryvnt

I've been able to reproduce this based on your repro. This is where the behavior occurs:

https://github.com/dotnet/aspnetcore/blob/9da617793b3b387fd16bbc3e0ec06337569ca6ac/src/Http/Http.Results/src/HttpResultsHelper.cs#L33

[captainsafia]

I'm inclined to think that the buggy behavior is in the handling of the non-Results case here. My inclination is that the right modification to make is to update it so that returning null produces a 204 response, as it does for a controller action.

Returning the string literal "null" for a null return type feels like incorrect behavior here.

[Dryvnt]

May be. I'm not personally against a 204 response for a null T?, so long as it's consistent. I just want to be able to write my APIs in a simple way and then trust they won't mysteriously break when I extend them a bit.

My only objection is that it means I can't use HttpClient.GetFromJson<T?> et. al. by default, since the default json deserializer throws an exception when given an empty body. You can configure your way out of it, but it's not trivial.

"It returns a T? so why is this Get<T?> failing?" is very bad dev-ex in my opinion. Assuming it was my fault, I nearly ripped my hair out trying to answer that question, which lead me to make this issue.

If the decision is made, I would suggest extending the default configuration to interpret an empty body as a null value for a nullable type. I'm happy to create a feature suggestion at the time, should it be necessary.

[Dryvnt]

Upon further research I am now personally strongly opposed to returning 204 for null values in the base case as it, by my reading, would violate the HTTP semantics spec. I'm not part of the .NET team so the decision is out of my hands, but I will at least try to make my case as best I can:

200 and 204 are different in the same way that string? Foo() => null and void Foo() {} are different.

When making a GET request for a resource, the appropriate response is a 200 with the body containing the requested resource in an appropriate representation. For a common HTTP API case (and especially if I set an Accept: application/json header in my request), that would be application/json. The single json token null is perfectly valid json representation for a resource that exists whose value is null. The empty string is not valid json. And "204: your request has been fulfilled" is nonsense.

I consider the fact that controller actions do things in an incorrect way an example of the kind of legacy cruft that makes me want to stay away from using them. I was hoping Minimal APIs would be better.

I have revised the example in the issue description to include the Content-Type headers of the response, as well as some more examples for adjacent cases.

The HTTP Semantics spec, for reference: https://www.rfc-editor.org/rfc/rfc9110#section-15.3

[captainsafia]

@Dryvnt Thanks for sharing your thoughts here! Even though your not a member of the team, you're still a user who has run into this issue so your perspective is important. Raising this concern is totally valid too since we shouldn't assume MVC's behavior is necessarily semantically correct. for everything.

Given the research you've done in the area, would you be interested in submitting a PR for the desired behavior here?

[Dryvnt]

Sure, I can take a look over the weekend.

[jded76]

Any news on this? We have the same problem trying to convert an owin web api to minimal api, and we are facing this behavior difference.

P.S. Thank you @Dryvnt for your time.

[Dryvnt]

Thanks for the reminder and motivation to look at this again, @jded76 :)

I've tried to implement some of the required changes myself, as seen in the draft PR (#49588). It was quite easy to make TypedResults respect null values. I did some additional work today in an attempt to make it ready for non-draft, but I've hit a problem.

Unfortunately, a few of the Results methods have their optional value parameter as the last of a few other optional parameters. From my understanding, there's no way to make those work without breaking the API heavily. e.g. putting the value parameter first, or making all the other parameters non-optional.

Assuming I did not miss any, the problematic methods in question are

• Results.CreatedAtRoute
• Results.Accepted
• Results.AcceptedAtRoute

I'm not sure how to proceed, so I have left them alone in my draft PR. The PR can still serve as a starting point for a discussion. If some C# wizard can figure out a way to make it work that I missed, that would be fantastic.

We could break the API for those methods, whatever that ends up looking like. but I imagine that is a hard sell.

A compromise solution is to accept that Results has a gotcha and thus define that null is always considered no-value for Results regardless of the underlying type, and then I remove all changes related to Results and we proceed with the PR. I can accept that "you did not care about your exact return type, so the framework did not either" is a lesser evil than "specifying your return code breaks your API." And the workaround is simple: If it matters in your use case, use TypedResults instead of Results.

Finally, we could accept current behavior, WONTFIX this issue, and move on. Obviously, I would not personally consider this option acceptable.

Whatever the decision, making it is above my pay grade. I officially throw the ball back to the development team.