Skip to content

[Open API] Nullability set on component schema level and not operation. Causing multiple component schemas being generated. #58617

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

Open
1 task done
marinasundstrom opened this issue Oct 24, 2024 · 19 comments
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi

Comments

@marinasundstrom
Copy link

marinasundstrom commented Oct 24, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Deal-breaker for migration. I discovered this while porting my API from using NSwag.

One component schema per type, not distinct schemas based om nullability. And I think this is a part of a bigger story about dealing with nullability.

I think this also affects Minimal APIs.

In this particular case, it seems like nullability is set on a component schema-level and not the schema on on parameter and response-level

This happens because of the action (Test) with return type TestDto regardless of whether it is nullable or not. So no concern for explicit nullability either.

Following is for OpenAPI 3.

Consider this controller:

[ApiController]
[Route([controller]")]
public class StatisticsController : ControllerBase
{

    // TestDto corresponds to component schema "TestDto2" with nullable: true

    [HttpGet("Test")]
    public TestDto Test()
    {
        return default!;
    }

/*
    // Not relevant
    [HttpGet("Test2")]
    public TestDto? Test2()
    {
        return default!;
    }
*/

    // TestDto corresponds to component schema "TestDto" (with nullable: false)

    [HttpGet("Test3")]
    public KeyValuePair<int, TestDto> Test3()
    {
        return default!;
    }
}

Instead of one TestDto schema, it results in two distinct schemas TestDto and TestDto2 where one is nullable.

The response on the operation should be nullable at an operation-level, not at component schema level:

components:
  schemas:
    TestDto:
      required:
        - x
      type: object
      properties:
        x:
          type: string
    TestDto2:
      required:
        - x
      type: object
      properties:
        x:
          type: string
      nullable: true <—- THIS IS WHY THE TYPE HAS TWO SCHEMAS:

With the operation being tis:

paths:
  /Statistics/Test2:
    get:
      tags:
        - Statistics
      responses:
        '200':
          description: OK
          content:
            text/plain:
              schema:
                $ref: '#/components/schemas/TestDto2’
            application/json:
              schema:
                $ref: '#/components/schemas/TestDto2’
            text/json:
              schema:
                $ref: '#/components/schemas/TestDto2’

Expected Behavior

I would expect this operation with the shared component schema, and the nullability set on the operation:

paths:
  /Statistics/Test2:
    get:
      tags:
        - Statistics
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                allOf:
                  - $ref: '#/components/schemas/TestDto'
                nullable: true
components:
  schemas:
    TestDto:
      required:
        - x
      type: object
      properties:
        x:
          type: string

Meaning the use of ”allOf” and ”nullable: true” on the response.

This allows for maximal re-use of DTOs. Not having two otherwise identical schemas that are different in nullability.

Steps To Reproduce

var builder = WebApplication.CreateBuilder();

builder.Services.AddControllers();

builder.Services.AddOpenApi();

var app = builder.Build();

app.MapControllers();

app.MapOpenApi();

app.Run();

Exceptions (if any)

No response

.NET Version

9.0.100-rc.2.24474.11

Anything else?

The bigger story would be to write a transformer that deals with nullability for parameters and responses.

I'm on my way prototyping for my own project.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Oct 24, 2024
@marinasundstrom marinasundstrom added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Oct 24, 2024
@mikekistler
Copy link
Contributor

Thank you for this bug report! Can you please provide a full repro so that we can investigate further? There are a few inconsistencies in the bug report, like Test2 being commented out in the controller but appearing in the generated OpenApi, and we want to get a clear complete picture.

@marinasundstrom
Copy link
Author

marinasundstrom commented Oct 25, 2024

@mikekistler Ah. I think I copied the wrong operation when composing the sample.

Anyway, I was working on this when getting the notification:

https://github.com/marinasundstrom/NullabilityTransformersPrototype/tree/main/WebApi

Just comment out all transformers, run the app, and you will see TestDto and TestDto2 schemas in Scalar.

builder.Services.AddOpenApi(options =>
            {
                /*
                options.ApplyServersTransformer();
                options.ApplyOperationId();
                options.ApplyNullabilityTransformer();
                options.ApplyDescriptionTransformer();
                options.ApplySchemaNameTransforms(TypeExtensions.GetSchemaName);
                options.ApplySchemasNotDistinctByNullability();
                options.ApplyInheritanceTransformer(); */
            });

There are other issues as well, but they are subject to other issues.

@mikekistler
Copy link
Contributor

Thanks for the full repro. This is very helpful and I believes a few things we observed from our side.

  • When the action method has a return type of TestDto or TestDto?, the schema in the response is not marked as "nullable: true". This is accurate because of a quirk in the way Controller-based apps work: a "null" response is converted to a 204 No Content response. Now there is a problem in that the 204 response is not shown in the generated OpenApi, but the definition of the 200 response is correct.

  • When the action method accepts a TestDto?, as is the case for T2 in your project, the request body is defined as nullable and this is also correct, because sending "null" in the request body will set the body parameter to null.

Regarding generating a single schema for TestDto and using allOf to set nullable on or off, this is an interesting approach and we will take this under consideration.

@marinasundstrom
Copy link
Author

marinasundstrom commented Oct 25, 2024

@mikekistler Thanks. Yes, I understand there are reasons that might differ from other Open API spec generators.

The point is that nullable: true should be set at the operation level., rather than on the component schema. And the solution adds that extra inline schema and allOf.

I have also found other things that are related to polymorphism. Those can also be resolved with the use of allOf. Also in the sample.

I will say that I have learned a lot about OpenAPI during the last two days regarding the conventions used by NSwag/NJsonSchema and Swashbuckle.

@mikekistler
Copy link
Contributor

Regarding:

The point is that nullable: true should be set at the operation level., rather than on the component schema.

I'd like to dig into this a bit further. Do you think this is the case only for request and response bodies or wherever a nullable type is rendered? As a specific case, the type KeyValuePair<TestDto, int> in your project is generated as

      "KeyValuePairOfTestDtoAndint": {
        "required": [
          "key",
          "value"
        ],
        "type": "object",
        "properties": {
          "key": {
            "$ref": "#/components/schemas/TestDto2"
          },
          "value": {
            "type": "integer",
            "format": "int32"
          }
        }
      },

where TestDto2 has nullable: true. Should this in your opinion instead be:

      "KeyValuePairOfTestDtoAndint": {
        "required": [
          "key",
          "value"
        ],
        "type": "object",
        "properties": {
          "key": {
            "allOf": {
              "$ref": "#/components/schemas/TestDto"
            },
            "nullable": true
          },
          "value": {
            "type": "integer",
            "format": "int32"
          }
        }
      },

@marinasundstrom
Copy link
Author

I'm not sure that allOf is needed in that case. Since there is just one schema. Outside AllOff is shared by all subschemas.

But if the key is of nullable type ToodoDto then the property itself should be nullable.

I need to check. My reference is NSwag's output.

@marinasundstrom
Copy link
Author

marinasundstrom commented Oct 26, 2024

@mikekistler I have prepared the project so it also has NSwag installed for comparison of output.

https://github.com/marinasundstrom/NullabilityTransformersPrototype/tree/main/WebApi


I have stored the Open API outputs here: https://github.com/marinasundstrom/NullabilityTransformersPrototype/tree/main/Client/OpenAPIs


It defaulted to 2.0, so had to force it into 3.0 (which I believe is the default in the framework stack). Then the nullability worked.

I did switch the types of key and value in the example: KeyValuePair<TestDto, int> to in KeyValuePair<int, TestDto>.

It doesn't seem that the resulting schema for the KeyValuePair marks Value for nullability.

When I think about it, what if you have two types of the same name. Which wasn't generated by the way, even when having a separate endpoint returning the same but with the value TestDto being nullable.

In other cases you could have distinct types with the same name being named TestDto, TestDto2 etc.

This will be for both KeyValuePair<int, TestDto> and KeyValuePair<int, TestDto?>:

    KeyValuePairOfIntegerAndTestDto:
      type: object
      additionalProperties: false
      properties:
        key:
          type: integer
          format: int32
        value:
          $ref: '#/components/schemas/TestDto'

I understand the tradeoff they, especially Rico Suter, had to make here.

I haven't checked the behavior with Swashbuckle.


One thing that I notice is that DescriptionAttribute on a type automatically translates into Description on the corresponding Schema. I use that in my shim as an alternative to XML docs. But NSwag just supports that out of box.

There sure are more cases not covered in this upcoming release.

@marinasundstrom
Copy link
Author

marinasundstrom commented Oct 26, 2024

It does seem like nullable isn't even set on the properties of schemas based on source property, for example int?.

I try to force it with a transformer but the changes seem to be overridden later.

Then I wonder if there is an option to enable that.

Nullability with nullable is supported from Open API 3.0 and up.

@mikekistler
Copy link
Contributor

According to the ASP.NET Core docs on Learn:

Properties defined as a nullable value or reference type have nullable: true in the generated schema. This is consistent with the default behavior of the System.Text.Json deserializer, which accepts null as a valid value for a nullable property.

So int? should appear in the OpenAPI doc as:

    "nullableValue": {
      "type": "integer",
      "format": "int32",
      "nullable": true
    },

My tests show this to be accurate. Are you seeing something different?

@marinasundstrom
Copy link
Author

marinasundstrom commented Oct 26, 2024 via email

@mikekistler
Copy link
Contributor

@marinasundstrom I opened a couple new issues (#58680, #58681) to track the problems I uncovered from researching this issue, but is there anything else here that needs to be addressed? If not can we close this issue?

@Chrille79
Copy link

I believe its still an issue with nullable

If you return this Person class

public class Person
{
    public string Name { get; set; } = string.Empty;
    public Address MyAddress { get; set; } = new();
    public Address? MyAltAddress { get; set; }
}
public class Address
{
    public string Street { get; set; } = string.Empty;
    public int Number { get; set; }
}```

the resulting schema is
```json
"components": {
    "schemas": {
      "Address": {
        "type": "object",
        "properties": {
          "street": {
            "type": "string"
          },
          "number": {
            "type": "integer",
            "format": "int32"
          }
        }
      },
      "Address2": {
        "type": "object",
        "properties": {
          "street": {
            "type": "string"
          },
          "number": {
            "type": "integer",
            "format": "int32"
          }
        },
        "nullable": true
      },
      "Person": {
        "type": "object",
        "properties": {
          "name": {
            "type": "string"
          },
          "myAddress": {
            "$ref": "#/components/schemas/Address"
          },
          "myAltAddress": {
            "$ref": "#/components/schemas/Address2"
          }
        }
      }
    }
  }

But you want the schema as

"components": {
    "schemas": {
      "Address": {
        "type": "object",
        "properties": {
          "street": {
            "type": "string"
          },
          "number": {
            "type": "integer",
            "format": "int32"
          }
        }
      },
      "Person": {
        "type": "object",
        "properties": {
          "name": {
            "type": "string"
          },
          "myAddress": {
            "$ref": "#/components/schemas/Address"
          },
          "myAltAddress": {
            "$ref": "#/components/schemas/Address",
            "nullable": true
          }
        }
      }
    }
  }

You dont want the new type Address2, you want the reference to be nullable

@mikekistler
Copy link
Contributor

@Chrille79 I can understand why you'd prefer not to have Address2, just so that it is nullable. But I think it does accurately describe the class from a JSON schema perspective. Further, I don't think there are any good alternatives.

The solution you propose -- putting nullable: true next to the "$ref" -- isn't allowed as JSON Schema prohibits sibling properties of $ref.

Another approach -- discussed above -- is to use "allOf" like this:

          "myAltAddress": {
            "allOf": [{
              "$ref": "#/components/schemas/Address"
            }],
            "nullable": true
          }

which doesn't work either because Address specifies type: object and not nullable: true, which does not allow a "null" value and thus does not satisfy the "allOf".

@marinasundstrom
Copy link
Author

marinasundstrom commented Nov 11, 2024

https://github.com/marinasundstrom/NullabilityTransformersPrototype/blob/main/Client/OpenAPIs/v1-nswag.yaml

For schemas, NSwag uses oneOf with nullable: true to indicate that the complex property is nullable:

    Bag:
        properties:
          count:
            type: integer
            format: int32
          assignedParticipant:
            nullable: true
            oneOf:
            - $ref: '#/components/schemas/Item''

NSwag uses "required" for operation parameters of object types - and that is interpreted as nullable by their client generator.

And you see how you can use oneOf``` to make the schema of a responseType`` be nullable.

openapi: 3.0.0
info:
  title: My Title
  version: 1.0.0
servers:
  - url: http://localhost:5295
paths:
  /Test/T5:
    post:
      tags:
        - Test
      operationId: Test_T5
      parameters:
        - name: x
          in: query
          schema:
            type: integer
            format: int32
            nullable: true
          x-position: 2
      requestBody:
        x-name: testDto
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/TestDto"
        required: true
        x-position: 1
      responses:
        200:
          description: ""
          content:
            application/json:
              schema:
                oneOf:
                - nullable: true
                  oneOf:
                  - $ref: '#/components/schemas/Item''
components:
  schemas:
    TestDto:
      type: object
      description: This is a test type
      additionalProperties: false
      properties:
        x:
          type: string
        z:
          type: integer
          format: int32
          nullable: true
        y:
          type: string
          nullable: true

@mikekistler
Copy link
Contributor

For schemas, NSwag uses oneOf with nullable: true to indicate that the complex property is nullable:

I believe this suffers from the same flaw as the "allOf" approach I described. The oneOf will not match "null" so it fails, and when any assertion within a schema fails then the schema fails.

Again, I understand the desire to avoid duplication between Address and Address2, but I still don't see an alternative that follows the JSON Schema specification.

I believe this will be possible with OpenAPI v3.1, which uses "full JSON Schema". In OpenAPI v3.1 you could write:

    "myAltAddress": {
      "oneOf": [
        {
          "$ref": "#/definitions/Address"
        },
        {
          "type": "null"
        }
      ]
    }

So a solution is in sight, but I don't see one for OpenAPI v3.0.x.

@mikekistler
Copy link
Contributor

I should have mentioned ... there is an issue for adding support for OpenAPI v3.1.

#58619

Feel free to put a thumb's up reaction there if this is important to you.

@JohnGalt1717
Copy link

This is a deal breaker. nswag and swashbuckle do this correctly. Microsoft's version is broken and creates garbage clients.

Only the trivial case works until this is fixed. At the very least you need to fix it to use oneOf like NSwag and Swashbuckle for now.

@StefanCuypersQua
Copy link

I have the same issue. I use Kiota afterwards to generate a C# client, but it is not very handy that 2 different classes are generated for what is essentially the same thing.

@marinasundstrom
Copy link
Author

This is why I will stick with NSWag until the out-of-box spec generator is compatible with the current ecosystem. I tried to make it work. I wouldn't market this as an replacement to the existing libraries.

wuzzeb added a commit to SeedTactics/fms-insight that referenced this issue May 5, 2025
Current Issues:

Duplicates based on nullability
- dotnet/aspnetcore#58617
- There is a bug in generating NewJobs: the array type for SimStationsCurrentStatus is wrong
- Some of the query parameters aren't marked as required but also don't have a default value, so maybe it is OK?  Need to check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi
Projects
None yet
Development

No branches or pull requests

5 participants