Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

204 response enhancements #5090

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Banner-Keith
Copy link

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;
    }
}

New document generator setting to allow for certain response codes to be treated as a nullable response, even if the response type is void.

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.

During client generation, if there are multiple success responses where some are nullable, we set the whole method return type as nullable.
@Banner-Keith
Copy link
Author

@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.

@@ -102,21 +102,28 @@ public string UnwrappedResultType
{
get
{
var response = GetSuccessResponse();
if (response.Value == null || response.Value.IsEmpty(_operation))
TResponseModel response = GetSuccessResponseModel();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!isNullable)
{
// If one of the success types is nullable, we set the method return type to nullable as well.
isNullable = Responses.Any(r => r.IsSuccess && r.Type == response.Type + "?" && r.IsNullable);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant