204 response enhancements

[Banner-Keith]

Purpose of change

Partially addresses issues from #1259

204 is a valid response type and it is valid to potentially return a 200 or a 204 from the same route. Currently nswag generates a server error for 204 results. In addition, this covers the situation where one of the success response types may return a null, but not all.

Change Summary

• New document generator setting ResponseStatusCodesToTreatAsNullable to allow for certain response codes to be treated as a nullable response, even if the response type is void. This means that we don't have to decorate everything that can return a 204 with the CanBeNull attribute.

• On the client generation side, we treat all 2xx responses with the same response type as the 200 response as success responses. Expanded that to include responses with the same type but nullable. e.g. a 200 with MyClass and a 204 with MyClass? would both return true when IsSuccessStatusCode is checked. A void type on the 204 would still not return true. That could be another enhancement to make later.

• During client generation, if there are multiple success responses (as defined by the result of IsSuccessStatusCode) where some are nullable, we set the whole method return type as nullable.

Usage Example

In Program.cs we tell the document generator that we want all 204 methods that do not have a NotNull attribute to be treated as nullable response types. We also give the response type the same class as the 200 result to pass checks in the generator library that only treat results as successful if they have the same return type as the 200. That part is existing behavior.

services.AddOpenApiDocument(document => 
{ 
    document.ResponseStatusCodesToTreatAsNullable = [HttpStatusCode.NoContent];
});

We have an example method in a .net web api.

/// <summary>Example Summary</summary>
[HttpGet]
[ProducesResponseType(typeof(MyClass), StatusCodes.Status200OK)]
[ProducesResponseType(typeof(MyClass), StatusCodes.Status204NoContent)]
public async Task<IActionResult> Get()
{
    MyClass? result = await _myservice.GetResult();
    
    if (result == null)
    {
	    return NoContent();
    }
    
    return Ok(result);
}

Note that the 200 is not a nullable response and the 204 is nullable due to the setting in Program.cs. They both use the same return type.
Using this example open api spec and generateNullableReferenceTypes set to true.

{
  "/api/Example": {
    "get": {
      "summary": "Example Summary",
      "operationId": "Example_Get",
      "responses": {
        "200": {
          "description": "",
          "content": {
            "application/json": {
              "schema": {
                "$ref": "#/components/schemas/MyClass"
              }
            }
          }
        },
        "204": {
          "description": "",
          "content": {
            "application/json": {
              "schema": {
                "nullable": true,
                "oneOf": [
                  {
                    "$ref": "#/components/schemas/MyClass"
                  }
                ]
              }
            }
          }
        }
      }
    }
  }
}

We would previously have generated something like this:

public virtual async System.Threading.Tasks.Task<MyClass> Get(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) 
{
    if (status_ == 200)
    {
        var objectResponse_ = await ReadObjectResponseAsync<InlPerson>(response_, headers_, cancellationToken).ConfigureAwait(false);
        if (objectResponse_.Object == null)
        {
            throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);
        }
        return objectResponse_.Object;
    }
    else
    if (status_ == 204)
    {
        // Throws error on 204 which makes this client unusable
        var objectResponse_ = await ReadObjectResponseAsync<InlPerson?>(response_, headers_, cancellationToken).ConfigureAwait(false);
        throw new ApiException<InlPerson?>("A server side error occurred.", status_, objectResponse_.Text, headers_, objectResponse_.Object, null); 
    }
}

Now we get this:

// Correctly marks whole method return type as nullable because one of the success status codes returns a nullable type.
public virtual async System.Threading.Tasks.Task<MyClass?> Get(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) 
{
    if (status_ == 200)
    {
        var objectResponse_ = await ReadObjectResponseAsync<InlPerson>(response_, headers_, cancellationToken).ConfigureAwait(false);
        // Note that there is still a null check for a 200 result, which is correct because I did not mark my action result as nullable.
        if (objectResponse_.Object == null)
        {
            throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);
        }
        return objectResponse_.Object;
    }
    else
    if (status_ == 204)
    {
        var objectResponse_ = await ReadObjectResponseAsync<InlPerson?>(response_, headers_, cancellationToken).ConfigureAwait(false);
        // Correctly returns the value, which will be null for 204.
        return objectResponse_.Object;
    }
}

[Banner-Keith]

@RicoSuter & @lahma I took a stab at addressing part of the problem in the linked issue. Please review and let me know your thoughts. Happy to make any changes needed.

[Banner-Keith]

This is calling a new method. Other call sites still use the original unchanged GetSuccessResponse method.

[Banner-Keith]

The new method call gives us access to the response type to see if we have additional success responses with the same underlying type that are null. Which means we would want to mark the operation as a whole nullable.

[Cpcrook]

This change would solve the issues I've been running into and I'd love to see this integrated sooner rather than later.

[markusahrweileramcon]

IMHO, this check is factually incorrect.
A response is a success, when the status code is between 200 and 300 (400), no?
A 204 has by design no content, which means Data? is wrong as response type.
It should be void.

Also, if someone would want to send a string on 201, but an object on 200, this check would still mark the response as failure

[Banner-Keith]

I agree that more changes should be made here. I am trying to make this particular PR be minimally impactful so we could potentially do this as a minor version change with no behavior changes for existing consumers of NSwag unless they opt in by using the new functionality. Changing this check in general would be more of a major version change, which may take much longer to get published. I would suggest we tackle that part separately unless you can find a way to keep it backwards compatible without a behavior change for existing users.

[markusahrweileramcon]

I guess, we could introduce an option which says to treat only the same response types as success which is true by default. Not sure, if this class can have easy access to the options