Handling nullable responses in NSwag-generated clients, and modernizing controller definitions

[vvdb-architecture]

These modifications have the following objectives:

• to allow null responses to be sent from controllers
• to have a better strongly typed definition of controller methods.

The modifications apply for Yarp and all other services.

Host project modifications

Infrastructure

Add the following code in the Infrastructure folder. This is a model provider that will add ProducesResponseTypeAttribute instances deduced from the return value of the methods. In addition, it will also add a response type for AppException instances, as is done explicitly today.

Replace SolutionName with the name of your solution.

using Arc4u.ServiceModel;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.ApplicationModels;

namespace HappyFlow.Yarp.Host.Infrastructure;

public class ProducesResponseTypeModelProvider : IApplicationModelProvider
{
    private static readonly Type ErrorReturnType = typeof(IEnumerable<Message>);

    public int Order => 3;

    public void OnProvidersExecuted(ApplicationModelProviderContext context)
    {
        // no implementation needed
    }

    public void OnProvidersExecuting(ApplicationModelProviderContext context)
    {
        foreach (var controller in context.Result.Controllers)
            foreach (var action in controller.Actions)
            {
                // extract from the action method the return type T from Task<ActionReslt<T>> or ActionResult<T> or just T.
                Type? returnType = action.ActionMethod.ReturnType;
                if (returnType.IsGenericType && returnType.GetGenericTypeDefinition() == typeof(Task<>))
                    returnType = returnType.GetGenericArguments()[0];
                if (returnType.IsGenericType && returnType.GetGenericTypeDefinition() == typeof(ActionResult<>))
                    returnType = returnType.GetGenericArguments()[0];

                // "no return type" is either "void" (in the synchronous case) or Task (in the asynchronous case)
                if (returnType == typeof(void) || returnType == typeof(Task))
                    returnType = null;

                var has2xx = false;
                var has400 = false;

                foreach (var p in action.Filters)
                    if (p is ProducesResponseTypeAttribute producesResponseTypeAttribute)
                    {
                        var statusCode = producesResponseTypeAttribute.StatusCode;
                        if (!has2xx && 200 <= statusCode && statusCode <= 299)
                        {
                            has2xx = true;
                            if (producesResponseTypeAttribute.Type == null && returnType != null && IsUntyped(returnType))
                                producesResponseTypeAttribute.Type = returnType;
                            continue;
                        }

                        // throwing an AppException returns an ErrorReturnType for now if none was specified.
                        if (!has400 && statusCode == StatusCodes.Status400BadRequest)
                        {
                            has400 = true;
                            if (producesResponseTypeAttribute.Type == null)
                                producesResponseTypeAttribute.Type = ErrorReturnType;
                            continue;
                        }
                    }

                // add the response types if none was found in the existing action filters
                if (!has2xx)
                {
                    if (returnType != null && IsUntyped(returnType))
                        throw new ArgumentException($"Method {action.Controller.ControllerName}.{action.ActionName} doesn't have a strongly typed return value");
                    AddProducesResponseTypeAttribute(action, returnType, StatusCodes.Status200OK);
                }
                if (!has400)
                    AddProducesResponseTypeAttribute(action, ErrorReturnType, StatusCodes.Status400BadRequest);
            }
    }

    private static bool IsUntyped(Type? returnType)
    {
        return returnType != null && typeof(IActionResult).IsAssignableFrom(returnType);
    }

    private static void AddProducesResponseTypeAttribute(ActionModel action, Type? returnType, int statusCodeResult)
    {
        if (returnType != null)
            action.Filters.Add(new ProducesResponseTypeAttribute(returnType, statusCodeResult));
        else // returnType == null
            action.Filters.Add(new ProducesResponseTypeAttribute(statusCodeResult));
    }
}

Program.cs

In Program.cs, locate the statement services.AddControllers() and adjust it as follows:

    services.AddControllers(opt =>
    {
        opt.OutputFormatters.RemoveType<HttpNoContentOutputFormatter>();
    });

    services.TryAddEnumerable(ServiceDescriptor.Transient<IApplicationModelProvider, ProducesResponseTypeModelProvider>());

The OutputFormatters statement removes the implicit conversion from successful (200) null responses to 204 empty responses.
This is needed to avoid raising a RestException for a 204 if a null return value is an indication of a non-existing result.

The documentation is hidden in Special case formatters. The saga about the implicit conversions can be found dotnet/aspnetcore#8847. Note that the issue has been closed by Microsoft without satisfying resolution (except the workaround described here).

The other statements adds the application model provider you defined in the previous point. Don't worry about the "Transient" lifetime, it will be called only once anyway.

Sdk project modifications (optional)

In the facade.swag and interface.nswag, locate the generateNullableReferenceTypes property in the codeGenerators:openApiToCSharpClient section and change its value to true.

      "generateNullableReferenceTypes": true,

This adds #nullable true in the client code, and generates nullable reference types when needed.
If you do this, you need to change .netstandard2.0 to .netstandard2.1 in your facade's .csproj file, since nullable reference types are a language feature that does not exist in .netstandard2.0.

You can ommit this if you want.

Controllers specification

Using strongly typed return values and adding a cancellation token

When generating a controller you can return a strongly typed value T, ActionResult<T>, Task<T> or Task<ActionResult<T>>. In that case, no [ProducesResponeType()] attribute will be needed if you only report "200 Success" or raise an AppException.
You can still add [ProducesResponeType()] in case your method returns several status codes, and/or does not want to use a strongly typed result.

If the methods of your controller are asynchronous, it is a good practice to specify a CancellationToken as the last parameter. The source of the cancellationToken will be HttpContext.RequestAborted, which will allow you to stop any long running requests.
See Handling aborted requests in Asp.Net.

For example, where previously you specified a controller and method like this (irrelevant details omitted):

[Authorize]
[ApiController]
[Route("/Service/facade/[controller]")]
[ApiExplorerSettings(GroupName = "facade")]
[ProducesResponseType(typeof(IEnumerable<Message>), StatusCodes.Status400BadRequest)]
public class EnvironmentController : ControllerBase
{
    /// <summary>
    /// Return details information about the application.
    /// </summary>
    /// <response code="200">Return the environment information.</response>
    /// <response code="400">Error during the process of the request.</response>
    /// <returns>The Environment name.</returns>
    [ServiceAspect(Access.AccessApplication)]
    [ProducesResponseType(typeof(EnvironmentInfo), StatusCodes.Status200OK)]
    [HttpGet("")]
    public async Task<IActionResult> GetAsync()
    {
        return Ok(await EnvironmentInfoBL.GetEnvironmentInfoAsync());
    }
}

Now you write it like this:

[Authorize]
[ApiController]
[Route("/facade/[controller]")]
[ApiExplorerSettings(GroupName = "facade")]
public class EnvironmentController : ControllerBase
{
    /// <summary>
    /// Return details information about the application.
    /// </summary>
    /// <param name="cancellationToken"></param>
    /// <response code="200">Return the environment information.</response>
    /// <response code="400">Error during the process of the request.</response>
    /// <returns>The Environment name.</returns>
    [ServiceAspect(Access.AccessApplication)]
    [HttpGet("")]
    public async Task<ActionResult<EnvironmentInfo>> GetAsync(CancellationToken cancellationToken)
    {
           return Ok(await EnvironmentInfoBL.GetEnvironmentInfoAsync());
    }
}

The cancellation token should be passed to the business layer, but it wasn't done here.

The generated Sdk code should be exactly the same.

Returning null from actions, and specifying mandatory parameters

If you need to specify a method returning a nullable reference, you need to:

• Specify the NRT in the return value.
• Add a nullable="true" in the response comment tag.

If you need to express the mandatory presence of a parameter, use the attribute [BindRequired] when specifying it.

This is an example of a method returning a possible null depending on a mandatory returnNull parameter:

    /// <summary>
    /// Return details information about the application.
    /// </summary>
    /// <param name="returnNull"></param>
    /// <param name="cancellationToken"></param>
    /// <response code="200" nullable="true">Return the environment information.</response>
    /// <returns>The Environment name.</returns>
    [ServiceAspect(Access.AccessApplication)]
    [HttpGet("")]
    public async Task<ActionResult<EnvironmentInfo?>> GetAsync([BindRequired]bool returnNull, CancellationToken cancellationToken)
    {
        if (returnNull)
            return Ok(null);
        return Ok(await EnvironmentInfoBL.GetEnvironmentInfoAsync());
    }

The most important indication for nullability is not the NRT itself, but the attribute nullable="true" in response comment !

In the generated Sdk, the return value will be an NRT (as expected), and the OpenAPI .json description will indicate that the return value is nullable.

By default, parameter binding will be optional, which will translate in a nullable data type in the generated SDK, and a default value in the called method (false in the bool case). If you want to make the parameter mandatory, [BindRequired] needs to be specified.

[rdarko]

This need to be tested on the version 1.10.

[janschmidmaier50]

We've implemented and tested that and have some suggestions.

Define null return

First of all we would suggest to not use the xml comment to define that a return value can be null, but use the CanBeNullAttribute instead.
The disadvantage is that the nullable is then defined for all status codes, but asp.net 6 is not capable of having a different method signature anyway, so this would be fine.
The advantage of doing so is, that it can be checked via an unit test

[Theory]
    [MemberData(nameof(NullableActions))]
    public void CanBeNullAttribute_IsSet_Everywhere(NamedWrapper<MethodInfo> infoWrap)
    {
        MethodInfo info = infoWrap;
        if (!info.ReturnParameter.CustomAttributes.Any(data => data.AttributeType.IsAssignableFrom(typeof(CanBeNullAttribute))))
        {
            Assert.Fail($"{infoWrap} should have the [return: CanBeNull] attribute)");
        }
    }

where NullableAction is

public static TheoryData<NamedWrapper<MethodInfo>> NullableActions
{
    get
    {
        var ret = new TheoryData<NamedWrapper<MethodInfo>>();

        foreach (var namedWrapper in ActionMethods
                     .Where(info =>
                         info.ReturnParameter?.CustomAttributes.Any(o => o.AttributeType.FullName == "System.Runtime.CompilerServices.NullableAttribute") == true)
                     .Select(info =>
                         new NamedWrapper<MethodInfo>(info, methodInfo => $"{methodInfo.DeclaringType}.{methodInfo.Name}")
                     ))
        {
            ret.Add(namedWrapper);
        }

        return ret;
    }
}

private static IEnumerable<MethodInfo> ActionMethods
{
    get
    {
        var ret = typeof(SomeController)
            .Assembly
            .GetExportedTypes()
            .SelectMany(type => type
                .GetMethods(BindingFlags.Public | BindingFlags.Instance)
                .Where(info => info.GetCustomAttributes().Any(attribute => attribute.GetType().IsAssignableTo(typeof(HttpMethodAttribute))))
            );
        return ret;
    }
}

So every Action returning a nullable object, will be checked if it has the [return: NJsonSchema.Annotations.CanBeNull] set.

How to handle BindRequired

From our experience it is no harm to set the BindRequiredAttribute on every parameter This can then be also checked via unit test (it can off course be extended to include null checks as well!)

[Theory]
[MemberData(nameof(ControllerActionParameters))]
public void BindRequired_IsSet_Everywhere(NamedWrapper<ParameterInfo> wrappedInfo)
{
    ParameterInfo info = wrappedInfo;
    info.CustomAttributes.Any(data => data.AttributeType == typeof(BindRequiredAttribute)).ShouldBeTrue(
        $"The parameter {info.Name} of {info.Member.DeclaringType?.Name}.{info.Member.Name}  should have the [{nameof(BindRequiredAttribute)}] set");
}

public static TheoryData<NamedWrapper<ParameterInfo>> ControllerActionParameters
{
    get
    {
        var ret = new TheoryData<NamedWrapper<ParameterInfo>>();
        foreach (var wrapper in ActionMethods
                     .SelectMany(info => info
                         .GetParameters()
                         .Where(parameterInfo => parameterInfo.ParameterType != typeof(CancellationToken))
                         .Select(parameterInfo => new NamedWrapper<ParameterInfo>(parameterInfo, i =>
                         {
                             var capturedInfo = info;
                             return $"{capturedInfo.DeclaringType}.{capturedInfo.Name} -> {i.Name}";
                         }))))
        {
            ret.Add(wrapper);
        }

        return ret;
    }
}

Handling ResponseTypeAttribute

With your proposal, the attribute will be set at runtime only and therefore work somehow magical (or using vodoo).
We would rather suggest, defining the response type at the controller action itself. This way it is obvious what has been defined. Then again, this can be checked using a unit test

[Theory]
[MemberData(nameof(ReturningActions))]
public void ResponseType_istSet_Everywhere(NamedWrapper<MethodInfo> infoWrap)
{
    MethodInfo info = infoWrap;
    var attributes = info.GetCustomAttributes<ProducesResponseTypeAttribute>().SingleOrDefault(attribute => attribute.StatusCode == 200);

    var type = GetReturnType(info);
    switch (type)
    {
        case Type _ when type.IsAssignableTo(typeof(StatusCodeResult)):
            attributes.Type.Should().Be(typeof(void));
            break;
        default:
            attributes.Type.Should().Be(type, $"missing the {nameof(ProducesResponseTypeAttribute)} for type {type}");
            break;
    }
}

public static TheoryData<NamedWrapper<MethodInfo>> ReturningActions
{
    get
    {
        var ret = new TheoryData<NamedWrapper<MethodInfo>>();

        foreach (var namedWrapper in ActionMethods
                     .Where(info => IsMethodReturningAValue(info) || info.GetCustomAttributes<ProducesResponseTypeAttribute>().Any(attribute => attribute.StatusCode == 200))
                     .Select(info =>
                         new NamedWrapper<MethodInfo>(info, methodInfo => $"{methodInfo.DeclaringType}.{methodInfo.Name}")
                     ))
        {
            ret.Add(namedWrapper);
        }

        return ret;
    }
}

This also illustrates a general problem with the proposed idea. What if the return type of the action is not being returned on ok, but on a different return code. Imagine the following Action (might be theoretical, but ...)

public ActionResult<string> EnableFeature(string featureToBeEnabled)
{
    var feature = _featureService.Get(featureToBeEnabled);
    if (feature is null)
    {
        return BadRequest($"{featureToBeEnabled} is unknown");
    }

    return Ok();
}

Using the automatic setting of the the response type will set the string return to 200, instead of 400. (This feature will be available with .net 7 :D).
Sure this will not be covered by the test right now as well, but one can check manually the correctness of the code.

This way the runtime is not impacted at all and the ProducesResponseTypeModelProvider is not needed at all.

Success response

We would further suggest having a unit test checking, that every Action has a success response (200-299) defined, without duplication.

[Theory]
[MemberData(nameof(Actions))]
public void SuccessfulResponse_istSet_Everywhere(NamedWrapper<MethodInfo> infoWrap)
{
    MethodInfo info = infoWrap;
    var attributes = info.GetCustomAttributes<ProducesResponseTypeAttribute>().Where(attribute => attribute.StatusCode is >= 200 and <= 299).ToList();
    attributes.Should().NotBeEmpty();
    attributes.Select(attribute => attribute.StatusCode).Should().OnlyHaveUniqueItems();
}

BadRequest

Within the ProducesResponseTypeModelProvider all action are decorated with [ProducesResponseType(StatusCodes.Status400BadRequest)], although the action itself does not create the BadRequest. The BadRequest then can only be generated by the framework itself. So adding it to every single action would just be meaningful to force NSwag to generate a BadResponse Handling everywhere.
So actually we can add it to every action, but this seems more a work around to get a dependent tool to work correctly.

General thoughts

We are putting real effort into the backend service in order to get the code generation tool (NSwag) working correctly. So actually we are adjusting the backend, in order to make the frontend working. This seems the wrong way around. Right now the backend and frontend are tightly coupled (since there is for example no versioning), so maybe using a shared library wouldn't be harmful, but ease things up.

[janschmidmaier50]

Why was this closed ?
I think there is a to do - at least adding the tests in the framework