Add InternalServerError and InternalServerError<T> to TypedResults

[onurmicoogullari]

Summary

As a .NET developer who develops a lot of Web API:s, I often use the new TypedResults with return type Result<T1, T2, ..>. In every project that I use this, I find myself implementing a custom return type of InternalServerError and InternalServerError<TValue> since it's missing in the standard implementation.

Motivation and goals

If I'm using TypedResults, I'm doing that for two major reasons:

• Compile time errors for return types that doesn't match the intended design (specified in the return type of the method)
• Autogeneration of metadata for OpenAPI specification

In scope

• You want to be able to have a 500 Internal Server error containing a complex type as one of your possible return types for your endpoint and get compile time errors if it doesn't match what you're actually returning.
• You want the return type of 500 Internal Server Error containing the complex type to be described in your autogenerated OpenAPI specification, so that your clients knows what to expect.
• Same as above, but a 500 Internal Server Error with no content.

Risks / unknowns

This can be misused as a general return type for client errors, but I feel like that is more of a violation of REST rather than the framework.

Examples

Simplified example:

app.MapGet("/books/{id}", async Task<Results<Ok<Book>, NotFound<string>, InternalServerError<string>>> (int id) => {
  
    try
    {
        // Retrieve the book
        var book = await ... 

        if (book is null)
            return TypedResults.NotFound($"Could not find book with id: {id}");

        // Found book
        return TypedResults.Ok(book);
    }
    catch (Exception ex)
    {
        Log.Error($"Something unexpected happened: {ex}");
        return TypedResults.InternalServerError("Something unexpected happened while processing your request, please contact the API provider");
    }

});

Detailed design

Looking at the other TypedResults, I would implement this as follows:

/// <summary>
/// An <see cref="IResult"/> that on execution will write an object to the response
/// with Internal Server Error (500) status code.
/// </summary>
public sealed class InternalServerError : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult
{
    /// <summary>
    /// Initializes a new instance of the <see cref="InternalServerError"/> class with the values
    /// provided.
    /// </summary>
    internal InternalServerError()
    {
    }

    /// <summary>
    /// Gets the HTTP status code: <see cref="StatusCodes.Status500InternalServerError"/>
    /// </summary>
    public int StatusCode => StatusCodes.Status500InternalServerError;

    int? IStatusCodeHttpResult.StatusCode => StatusCode;

    /// <inheritdoc/>
    public Task ExecuteAsync(HttpContext httpContext)
    {
        ArgumentNullException.ThrowIfNull(httpContext);

        // Creating the logger with a string to preserve the category after the refactoring.
        var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
        var logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Http.Result.InternalServerErrorObjectResult");

        HttpResultsHelper.Log.WritingResultAsStatusCode(logger, StatusCode);
        httpContext.Response.StatusCode = StatusCode;

        return Task.CompletedTask;
    }

    /// <inheritdoc/>
    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
    {
        ArgumentNullException.ThrowIfNull(method);
        ArgumentNullException.ThrowIfNull(builder);

        builder.Metadata.Add(new ProducesResponseTypeMetadata(StatusCodes.Status500InternalServerError, typeof(void)));
    }
}

.. and

/// <summary>
/// An <see cref="IResult"/> that on execution will write an object to the response
/// with Internal Server Error (500) status code.
/// </summary>
/// <typeparam name="TValue">The type of error object that will be JSON serialized to the response body.</typeparam>
public sealed class InternalServerError<TValue> : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult, IValueHttpResult, IValueHttpResult<TValue>
{
    /// <summary>
    /// Initializes a new instance of the <see cref="InternalServerError"/> class with the values
    /// provided.
    /// </summary>
    /// <param name="error">The error content to format in the entity body.</param>
    internal InternalServerError(TValue? error)
    {
        Value = error;
        HttpResultsHelper.ApplyProblemDetailsDefaultsIfNeeded(Value, StatusCode);
    }

    /// <summary>
    /// Gets the object result.
    /// </summary>
    public TValue? Value { get; }

    object? IValueHttpResult.Value => Value;

    /// <summary>
    /// Gets the HTTP status code: <see cref="StatusCodes.Status500InternalServerError"/>
    /// </summary>
    public int StatusCode => StatusCodes.Status500InternalServerError;

    int? IStatusCodeHttpResult.StatusCode => StatusCode;

    /// <inheritdoc/>
    public Task ExecuteAsync(HttpContext httpContext)
    {
        ArgumentNullException.ThrowIfNull(httpContext);

        // Creating the logger with a string to preserve the category after the refactoring.
        var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
        var logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Http.Result.InternalServerErrorObjectResult");

        HttpResultsHelper.Log.WritingResultAsStatusCode(logger, StatusCode);
        httpContext.Response.StatusCode = StatusCode;

        return HttpResultsHelper.WriteResultAsJsonAsync(
                httpContext,
                logger: logger,
                Value);
    }

    /// <inheritdoc/>
    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
    {
        ArgumentNullException.ThrowIfNull(method);
        ArgumentNullException.ThrowIfNull(builder);

        builder.Metadata.Add(new ProducesResponseTypeMetadata(StatusCodes.Status500InternalServerError, typeof(TValue), new[] { "application/json" }));
    }
}

I would open a PR, but the code of conduct said to post a design proposition first.

[Ziwin]

Will this feature be added?

[onurmicoogullari]

I've added a PR for this issue, hoping it will be picked up by someone at the ASP.NET team. Glad to see that others find this useful as well!

[davidfowl]

Looks reasonable to me.

cc @captainsafia @JeremyLikness

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[captainsafia]

I marked this as API ready for review. Proposed API is as follows (copied from @onurmicoogullari's message above with method implementations removed)

namespace Microsoft.AspNetCore.Http.HttpResults;

public sealed class InternalServerError : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult
{
    public int StatusCode => StatusCodes.Status500InternalServerError;
    int? IStatusCodeHttpResult.StatusCode => StatusCode;
    public Task ExecuteAsync(HttpContext httpContext)
    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)'
}

public sealed class InternalServerError<TValue> : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult, IValueHttpResult, IValueHttpResult<TValue>
{
    internal InternalServerError(TValue? error)
    public TValue? Value { get; }
    object? IValueHttpResult.Value => Value;
    public int StatusCode => StatusCodes.Status500InternalServerError;
    int? IStatusCodeHttpResult.StatusCode => StatusCode;
    public Task ExecuteAsync(HttpContext httpContext)
    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
}

[MackinnonBuck]

API Review notes:

• We'll want to add a new InternalServerError method to TypedResults as well
• Should we have another InternalServerError variant that includes an exception?
  • Conclusion: Yes, so that we can hook into exception handling and the problem details middleware.

The API that's been proposed so far has been approved!

Updated API:

namespace Microsoft.AspNetCore.Http.HttpResults;

+ public sealed class InternalServerError : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult
+ {
+    public int StatusCode => StatusCodes.Status500InternalServerError;
+    int? IStatusCodeHttpResult.StatusCode => StatusCode;
+    public Task ExecuteAsync(HttpContext httpContext)
+    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
+}

+public sealed class InternalServerError<TValue> : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult, IValueHttpResult, IValueHttpResult<TValue>
+{
+    public TValue? Value { get; }
+    object? IValueHttpResult.Value => Value;
+    public int StatusCode => StatusCodes.Status500InternalServerError;
+    int? IStatusCodeHttpResult.StatusCode => StatusCode;
+    public Task ExecuteAsync(HttpContext httpContext)
+    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
+}

[onurmicoogullari]

@MackinnonBuck Awesome! I must've missed adding it to TypedResults. Should I update my PR to adhere to the change requests suggested by your review?

[captainsafia]

Awesome! I must've missed adding it to TypedResults. Should I update my PR to adhere to the change requests suggested by your review?

Yes, please!

Another thing we discussed was adding an new overload of InternalServerError that took an Exception type that would be re-thrown for handling by the exception handling and problem details middlewares but allowed an endpoint to participate in the OpenAPI metadata generation.

Let me know if you need any help modifying the API/PR with these changes.

[onurmicoogullari]

@captainsafia Actually, I would love to get some help if you have the time. Do you prefer to jump on a call or should I post my questions here/dm them to you somewhere?

[captainsafia]

@onurmicoogullari Sure! Feel free to ask any questions here or in the PR so we have some code context to work with.

[onurmicoogullari]

@captainsafia Okey, so I went through my code, and I could see that I had already added InternalServerError to TypedResults. Can you please clarfiy what you guys meant when you said:

We'll want to add a new InternalServerError method to TypedResults as well

I also found that I had only added unit tests for the classes InternalServerError.cs and InternalServerErrorOfT.cs in InternalServerErrorResultTests.cs and InternalServerErrorOfTResultTests.cs, but I had missed adding unit tests for TypedResults.InternalServerError() and TypedResults.InternalServerError<TValue>() in TypedResultsTests.cs. I updated the PR with the missing tests.

Another thing I noticed was that all the other types that are available thorugh TypedResults were also exposed through Results, which internally used the TypedResults-method. Here is an example of Results.BadRequest:

   /// <summary>
    /// Produces a <see cref="StatusCodes.Status400BadRequest"/> response.
    /// </summary>
    /// <param name="error">An error object to be included in the HTTP response body.</param>
    /// <returns>The created <see cref="IResult"/> for the response.</returns>
    public static IResult BadRequest(object? error = null)
        => BadRequest<object>(error);

    /// <summary>
    /// Produces a <see cref="StatusCodes.Status400BadRequest"/> response.
    /// </summary>
    /// <param name="error">An error object to be included in the HTTP response body.</param>
    /// <returns>The created <see cref="IResult"/> for the response.</returns>
    public static IResult BadRequest<TValue>(TValue? error)
        => error is null ? TypedResults.BadRequest() : TypedResults.BadRequest(error);

So I decided to update the PR with the corresponding methods for InternalServerError in Results.cs accordingly:

    /// <summary>
    /// Produces a <see cref="StatusCodes.Status500InternalServerError"/> response.
    /// </summary>
    /// <param name="error">An error object to be included in the HTTP response body.</param>
    /// <returns>The created <see cref="IResult"/> for the response.</returns>
    public static IResult InternalServerError(object? error = null)
        => InternalServerError<object>(error);

    /// <summary>
    /// Produces a <see cref="StatusCodes.Status500InternalServerError"/> response.
    /// </summary>
    /// <param name="error">An error object to be included in the HTTP response body.</param>
    /// <returns>The created <see cref="IResult"/> for the response.</returns>
    public static IResult InternalServerError<TValue>(TValue? error)
        => error is null ? TypedResults.InternalServerError() : TypedResults.InternalServerError(error);

.. and added unit tests for these in ResultsTests.cs:

    [Fact]
    public void InternalServerError_WithValue_ResultHasCorrectValues()
    {
        // Arrange
        object value = new { };

        // Act
        var result = Results.InternalServerError(value) as InternalServerError<object>;

        // Assert
        Assert.Equal(StatusCodes.Status500InternalServerError, result.StatusCode);
        Assert.Equal(value, result.Value);
    }

    [Fact]
    public void InternalServerErrorOfT_WithValue_ResultHasCorrectValues()
    {
        // Arrange
        var value = new Todo(1);

        // Act
        var result = Results.InternalServerError(value) as InternalServerError<Todo>;

        // Assert
        Assert.Equal(StatusCodes.Status500InternalServerError, result.StatusCode);
        Assert.Equal(value, result.Value);
    }

    [Fact]
    public void InternalServerError_WithNoArgs_ResultHasCorrectValues()
    {
        // Act
        var result = Results.InternalServerError() as InternalServerError;

        // Assert
        Assert.Equal(StatusCodes.Status500InternalServerError, result.StatusCode);
    }

    (...)

    private static IEnumerable<(Expression<Func<IResult>>, Type)> FactoryMethodsTuples { get; } = new List<(Expression<Func<IResult>>, Type)>
    {
        (...)
        (() => Results.InternalServerError(null), typeof(InternalServerError)),
        (() => Results.InternalServerError(new()), typeof(InternalServerError<object>)),
        (...)
    };

Problem is, I get the following error from the Roslyn Analyzer:

/home/onmi/src/aspnetcore/src/Http/Http.Results/src/Results.cs(698,27): error RS0027: Symbol 'InternalServerError' violates the backcompat requirement: 'Public API with optional parameter(s) should have the most parameters amongst its public overloads'. See 'https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details.

I read through the linked Roslyn docs from the error, but still couldn't figure out the difference between my new methods in Results.cs and the others. Why do Roslyn complain about the new methods and not the current ones?

Last but not least, I wonder if you could give me some guidance on the suggested InternalServerError implementation that takes an exception?

[captainsafia]

Okey, so I went through my code, and I could see that I had already added InternalServerError to TypedResults. Can you please clarfiy what you guys meant when you said:

Ah, you're right. The TypedResults.InteralServerError was not included in the AP proposal comment that I created here which was used for review. I went off the proposed design in the original issue and not the PR content. No action needed here. Thanks for adding the additional test coverage.

I read through the linked Roslyn docs from the error, but still couldn't figure out the difference between my new methods in Results.cs and the others. Why do Roslyn complain about the new methods and not the current ones?

Yes, this is strange indeed 🤔 . I wonder if this might actually be an issue in the Public API analyzer given this is the first time we've added an API to results since we created the type. I think it should be sufficient to suppress the warning in this case.

Last but not least, I wonder if you could give me some guidance on the suggested InternalServerError implementation that takes an exception?

Yes, the idea here is to include a new InternalServerError implementation with the following API:

public sealed class InternalServerError(ExceptionDispatchInfo exception) : IEndpointMetadataProvider
{
  public Task ExecuteAsync(HttpContext httpContext) => exception.Throw();
  static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
}

When the exception is thrown from the ExecuteAsync method, it'll be caught by the ExceptionHandling middleware and treated as exceptions typically would be in the application. However, the IEndpointMetaProvider will allow us to provide metadata that the endpoint produces a 500 error.

As a first step here, let's update the API proposal for the issue to include this new type and confirm that the API shape there is what we would like.

[captainsafia]

Yes, this is strange indeed 🤔 . I wonder if this might actually be an issue in the Public API analyzer given this is the first time we've added an API to results since we created the type. I think it should be sufficient to suppress the warning in this case.

@halter73 suspects that it might be an analyzer issue as well. We think we should break the pattern used for the other endpoints and just provide two overloads, one that takes no arguments and another that takes the generic overload.

public static IResult InternalServerError()
	=> InternalServerError<object>(null);

public static IResult InternalServerError<TValue>(TValue? error)
	=> error is null ? TypedResults.InternalServerError() : TypedResults.InternalServerError(error);

[onurmicoogullari]

@captainsafia Thank you for your answers.

Yes, this is strange indeed 🤔 . I wonder if this might actually be an issue in the Public API analyzer given this is the first time we've added an API to results since we created the type. I think it should be sufficient to suppress the warning in this case.

@halter73 suspects that it might be an analyzer issue as well. We think we should break the pattern used for the other endpoints and just provide two overloads, one that takes no arguments and another that takes the generic overload.

public static IResult InternalServerError()
	=> InternalServerError<object>(null);

public static IResult InternalServerError<TValue>(TValue? error)
	=> error is null ? TypedResults.InternalServerError() : TypedResults.InternalServerError(error);

Yeah, that's what I suspected as well. I have updated the endpoints so that it matches your suggestion, which of course made the Roslyn Analyzer complaints go away. I haven't pushed these changes to the PR yet.

Last but not least, I wonder if you could give me some guidance on the suggested InternalServerError implementation that takes an exception?

Yes, the idea here is to include a new InternalServerError implementation with the following API:

public sealed class InternalServerError(ExceptionDispatchInfo exception) : IEndpointMetadataProvider
{
  public Task ExecuteAsync(HttpContext httpContext) => exception.Throw();
  static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
}

When the exception is thrown from the ExecuteAsync method, it'll be caught by the ExceptionHandling middleware and treated as exceptions typically would be in the application. However, the IEndpointMetaProvider will allow us to provide metadata that the endpoint produces a 500 error.

I wonder if the ExceptionDispatchInfo should be an overload of the current InternalServerError.cs implementations constructor instead? This would prevent the names of the classes InternalServerError to clash. Also the ExecuteAsync method has a return type of Task, which makes the compiler complain about the suggested implementation public Task ExecuteAsync(HttpContext httpContext) => exception.Throw();, since it doesn't return anything.

What do you think about the below implementation? Would it cause confusion that ExecuteAsync doesn't actually return anything in case you initialized the class with an Exception, but that it rather throws the Exception?

namespace Microsoft.AspNetCore.Http.HttpResults;

/// <summary>
/// An <see cref="IResult"/> that on execution will write an object to the response
/// with Internal Server Error (500) status code.
/// </summary>
public sealed class InternalServerError : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult
{
    /// <summary>
    /// Initializes a new instance of the <see cref="InternalServerError"/> class with the values
    /// provided.
    /// </summary>
    internal InternalServerError()
    {
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="InternalServerError"/> class with the values
    /// </summary>
    /// <param name="exception">The exception to be thrown.</param>
    internal InternalServerError(ExceptionDispatchInfo exception)
    {
        Exception = exception;
    }

    /// <summary>
    /// Gets the exception to be thrown.
    /// </summary>
    public ExceptionDispatchInfo? Exception { get; }

    /// <summary>
    /// Gets the HTTP status code: <see cref="StatusCodes.Status500InternalServerError"/>
    /// </summary>
    public int StatusCode => StatusCodes.Status500InternalServerError;

    int? IStatusCodeHttpResult.StatusCode => StatusCode;

    /// <inheritdoc/>
    public Task ExecuteAsync(HttpContext httpContext)
    {
        Exception?.Throw();
        ArgumentNullException.ThrowIfNull(httpContext);

        // Creating the logger with a string to preserve the category after the refactoring.
        var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
        var logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Http.Result.InternalServerErrorObjectResult");

        HttpResultsHelper.Log.WritingResultAsStatusCode(logger, StatusCode);
        httpContext.Response.StatusCode = StatusCode;

        return Task.CompletedTask;
    }

    /// <inheritdoc/>
    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
    {
        ArgumentNullException.ThrowIfNull(method);
        ArgumentNullException.ThrowIfNull(builder);

        builder.Metadata.Add(new ProducesResponseTypeMetadata(StatusCodes.Status500InternalServerError, typeof(void)));
    }
}

As a first step here, let's update the API proposal for the issue to include this new type and confirm that the API shape there is what we would like.

How do I do this? Should I just paste in the shapes of the added APIs here in the issue?

[captainsafia]

What do you think about the below implementation? Would it cause confusion that ExecuteAsync doesn't actually return anything in case you initialized the class with an Exception, but that it rather throws the Exception?

Hmmm....I'm not opposed to adding another constructor to the existing InternalServerError type. We'll have to see how that fairs in API review but I'm comfortable championing that shape.

How do I do this? Should I just paste in the shapes of the added APIs here in the issue?

Yep! Then we'll review the API shape in the issue for the follow-up.

[onurmicoogullari]

OK, so here comes the API proposal:

InternalServerError.cs

namespace Microsoft.AspNetCore.Http.HttpResults;

public sealed class InternalServerError : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult
{
    internal InternalServerError()
    internal InternalServerError(ExceptionDispatchInfo exception)
    public ExceptionDispatchInfo? Exception { get; }
    public int StatusCode => StatusCodes.Status500InternalServerError;
    int? IStatusCodeHttpResult.StatusCode => StatusCode;
    public Task ExecuteAsync(HttpContext httpContext)
    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
}

InternalServerErrorOfT.cs

namespace Microsoft.AspNetCore.Http.HttpResults;

public sealed class InternalServerError<TValue> : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult, IValueHttpResult, IValueHttpResult<TValue>
{
    internal InternalServerError(TValue? error)
    public TValue? Value { get; }
    object? IValueHttpResult.Value => Value;
    public int StatusCode => StatusCodes.Status500InternalServerError;
    int? IStatusCodeHttpResult.StatusCode => StatusCode;
    public Task ExecuteAsync(HttpContext httpContext)
    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
}

TypedResults.cs

namespace Microsoft.AspNetCore.Http;

public static class TypedResults
{
   ...
   public static InternalServerError InternalServerError() => ResultsCache.InternalServerError;
   public static InternalServerError<TValue> InternalServerError<TValue>(TValue? error) => new(error);
   public static InternalServerError InternalServerError(ExceptionDispatchInfo exception) => new(exception);
   ...
}

Results.cs

namespace Microsoft.AspNetCore.Http;

public static partial class Results
{
   ...
   public static IResult InternalServerError()
        => InternalServerError<object>(null);

    public static IResult InternalServerError<TValue>(TValue? error)
        => error is null ? TypedResults.InternalServerError() : TypedResults.InternalServerError(error);

   public static IResult InternalServerError(ExceptionDispatchInfo exception)
        => TypedResults.InternalServerError(exception);
   ...
}

I have updated the PR with these changes, and test coverage for them. Please let me know if there is something that I have missed or that you need me to change and I will be happy to do it.

[dotnet-policy-service[bot]]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[captainsafia]

@onurmicoogullari Thanks! I'll send this in for API review now.

[halter73]

Thanks for waiting. After some internal discussion, our API review feedback is to take back what we said earlier when we suggested we should have another InternalServerError variant that includes an exception, so that we could hook into exception handling and the problem details middleware."

API Review Notes:

• @BrennanConroy pointed out "What’s the reason for accepting ExceptionDispatchInfo instead of Exception? I’m not aware of any real public API that takes an ExceptionDispatchInfo. I am aware of API that takes an Exception though and uses ExceptionDispatchInfo internally to preserve stacktrace info etc.: System.IO.Pipelines"
  • We agreed that if we did want to store the exception, the public API would take an Exception, but it would capture an ExceptionDispatchInfo
• It's weird how the non-TValue takes an exception, but the TValue one doesn’t.
  • We could take the exception in both versions, and simply throw the exception after writing the TValue to the response body. In this case, the exception should be logged, but it should be too late for the exception to influence what’s written to the response by the developer or exception handling middleware.
  • we would prefer to simply not take an exception for either version keeping it as similar to BadRequest/NotFound as possible. Rethrowing in IResult.ExcecuteAsync feels wrong. You could get the same effective results by simply declaring InternalServerError as a possible Result but throwing the exception directly from the route handler.
• We're a bit skeptical of the two motivations for the API proposal being compile-time errors and OpenAPI specs.
  • How the API will help catch any new errors at compile time? If you declare an InternalServerError as one of your Results types, nothing forces you to actually use it. And if you don’t, you could still throw and get a 500.
  • Ideally, a well-designed app never returns a 500, so I don’t see why you would want to include it in the OpenAPI specification. If the reason for a 500 is something other than an unintentional bug, it should probably be a 503 or 400 instead.
  • We would like to know where we are going to draw the line with built-in IResults for specific status codes. Personally, I feel that a 503 ServiceUnavailable IResult is more useful than a 500 InternalServerError one, but it doesn’t look like anyone has asked for it yet.
• We do understand it from a completeness perspective though. We have similar APIs for BadRequest, NotFound, etc… And we’re not forcing anyone to use the API just by adding it. At least it’s very clear what it does.

Ultimately, we decided to approve the API without the Exception stuff for now. If we decide later that we were wrong to exclude Exception overloads, we can still add it then.

namespace Microsoft.AspNetCore.Http.HttpResults;

+ public sealed class InternalServerError : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult
+ {
+    public int StatusCode => StatusCodes.Status500InternalServerError;
+    int? IStatusCodeHttpResult.StatusCode => StatusCode;
+    public Task ExecuteAsync(HttpContext httpContext)
+    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
+}

+public sealed class InternalServerError<TValue> : IResult, IEndpointMetadataProvider, IStatusCodeHttpResult, IValueHttpResult, IValueHttpResult<TValue>
+{
+    public TValue? Value { get; }
+    object? IValueHttpResult.Value => Value;
+    public int StatusCode => StatusCodes.Status500InternalServerError;
+    int? IStatusCodeHttpResult.StatusCode => StatusCode;
+    public Task ExecuteAsync(HttpContext httpContext)
+    static void IEndpointMetadataProvider.PopulateMetadata(MethodInfo method, EndpointBuilder builder)
+}

namespace Microsoft.AspNetCore.Http;
 
public static partial class Results
{
+   public static IResult InternalServerError() => TypedResults.InternalServerError();
+    public static IResult InternalServerError<TValue>(TValue? error)
+        => error is null ? TypedResults.InternalServerError() : TypedResults.InternalServerError(error);
}

public static class TypedResults
{
+   public static InternalServerError InternalServerError() => ResultsCache.InternalServerError;
+   public static InternalServerError<TValue> InternalServerError<TValue>(TValue? error) => new(error);
}

[onurmicoogullari]

I'm happy to see that you have approved my API proposal. Thank you guys for being kind and helpful throughout the process! I've updated the PR and removed the overload that includes an exception. Also, I learned a lot by looking at the code in this repo. 😊

Here are some of my thoughts on your notes from the review:

Ideally, a well-designed app never returns a 500, so I don’t see why you would want to include it in the OpenAPI specification. If the reason for a 500 is something other than an unintentional bug, it should probably be a 503 or 400 instead.

500 Internal Server Error is a response type that indicates to the consumer of an API that an unexpected error happened, preventing the API from fufulfilling the request. Of course this is not desirable, but I would argue that it is indeed a scenario that can and (given enough time) probably will happen. And when that happens, it's nice to have prepared the consumer for how such a response will look. Will it contain a ProblemDetails? Or maybe just a string with some error text? Whichever one it is, the consumer now has a better chance to understand how to handle it. Maybe I'm wrong, but I have always considered the 503 Service Unavailable to be caused by some expected reason, like planned maintenance or something. And of course, 400 Bad Request would assume that the consumer did something wrong.

How the API will help catch any new errors at compile time? If you declare an InternalServerError as one of your Results types, nothing forces you to actually use it. And if you don’t, you could still throw and get a 500.

So, lets take a look at this code:

app.MapGet("/", Results<Ok, NoContent> () =>
{
    return TypedResults.BadRequest();
});

It will give the following compile-time error:

Cannot implicitly convert type 'Microsoft.AspNetCore.Http.HttpResults.BadRequest' to 'Microsoft.AspNetCore.Http.HttpResults.Results<Microsoft.AspNetCore.Http.HttpResults.Ok, Microsoft.AspNetCore.Http.HttpResults.NoContent>'CS0029

This is because BadRequest is not specified as a valid response type in Results<T1, T2, ...>, which in turn makes sure that I cannot return a response type that is not represented in my OpenAPI specification. This is the same exact thing I want to achieve with InternalServerError.

With all that said, these are just my opinions and thoughts. That does not necessarily mean that they are correct.

Btw, is it possible to say when this change will be made available?