Skip to content

OpenApi generation creates NullableOf version of structs when null is allowed? #59056

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

Closed
Markz878 opened this issue Nov 19, 2024 · 19 comments
Closed
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved

Comments

@Markz878
Copy link

Markz878 commented Nov 19, 2024

Not sure if this is a question, bug report or API proposal, but I was pretty surprised when I tried to switch from Swagger to .NET 9 OpenApi generation, and saw that it generates these NullableOfXXX versions of structs if the API has any endpoints where the struct is nullable in input or output objects. What is the purpose of them?

This makes it harder to migrate from Swagger api generation to this, because we have auto-generated the Typescript schema objects and this would need a lot of changes in the app's TS signatures.

I'm not an OpenApi expert, but couldn't this be handled by using oneOf or anyOf?
Or can this be somehow disabled using the OpenApi configuration? This doesn't happen with classes, why are structs handled differently?

@Markz878 Markz878 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 19, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Nov 19, 2024
@gfoidl gfoidl added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 19, 2024
@Atulin
Copy link

Atulin commented Nov 21, 2024

I'm having the same issue. Switching from Swagger generated a bunch of NullableOf, IEnumerableOf, DictionaryOf and other types of this kind, instead of having them inlined. The worst offender are probably enums and collections of primitives. I really don't need NullableOfMyEnum and IEnumerableOfInt64 when just { "$ref": "#/components/schemas/MyEnum", "nullable": true } would suffice.

@ClementCHA
Copy link

I'm also having the same issue. Fun part is that, if a model is using nullable Propriety AND an other model using the same propriety but non nullable this time, it will add it twice in the json as NullableOfProperty & Property

@jankaltenecker
Copy link

I encountered the same issue. Here's another problem I noticed:
If you add additional metadata (e.g., a description), it results in duplicate schemas as well. For example, I end up with two schemas:

  • NullableOfMyEnum (without a description)
  • NullableOfMyEnum2 (with a description)

This happens because metadata like nullable, example, and description are applied to the reference itself rather than being inlined.

OpenAPI keywords like oneOf, anyOf, allOf, or not are not used at all. Is there a way to activate this?

I came across this blog post about schema reuse, but the enum inlining example doesn't work for me: https://devblogs.microsoft.com/dotnet/dotnet9-openapi/#customize-schema-reuse

Swashbuckle:

"MyModel": {
  "type": "object",
  "properties": {
    "myEnum": {
      "allOf": [
        {
          "$ref": "#/components/schemas/MyEnum"
        }
      ],
      "description": "Description.",
      "nullable": true,
      "example": "Value1"
    }
  }
},

...

"MyEnum": {
  "enum": [
    "Value1",
    "Value2",
    "Value3"
  ],
  "type": "string"
}

.NET 9:

"MyModel": {
  "type": "object",
  "properties": {
    "myEnum": {
      "$ref": "#/components/schemas/MyEnum"
    }
  }
},

...

"NullableOfMyEnum": {
  "enum": [
    "Value1",
    "Value2",
    "Value3",
    null
  ],
  "nullable": true,
  "example": "Value1"
},
"NullableOfMyEnum2": {
  "enum": [
    "Value1",
    "Value2",
    "Value3",
    null
  ],
  "description": "Description.",
  "nullable": true,
  "example": "Value1"
}

@mikekistler
Copy link
Contributor

tl;dr

Yes. A schema that allows null is different than a schema that does not, so two different schemas are generated. By necessity.

The detailed answer

Swashbuckle also generates different schemas. The difference is really that Swashbuckle attempts to reuse the "base" (non-nullable) schema in the schema for the nullable type. Unfortunately, the way this is done is not technically correct. To explain why, consider this schema:

"MyModel": {
  "type": "object",
  "properties": {
    "myEnum": {
      "allOf": [
        {
          "$ref": "#/components/schemas/MyEnum"
        }
      ],
      "description": "Description.",
      "nullable": true,
      "example": "Value1"
    }
  }
},

And now let's look at how nullable is defined in the OpenAPI specification. Here's the description in the 3.0.4 (most recent and likely the last) point release of the 3.0.x OpenAPI Specification [ref]:

This keyword only takes effect if type is explicitly defined within the same Schema Object. A true value indicates that both null values and values of the type specified by type are allowed. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of null as a value. A false value leaves the specified or default type unmodified. The default value is false.

By this description, the nullable keyword in the schema for the MyEnum property has no effect, because type is not explicitly defined within the same Schema Object. Further, because "Other Schema Object constraints retain their defined behavior" and because the MyEnum schema in the allOf does not allow null, this constraint applies to the schema for the MyEnum property.

Aside: the nullable keyword in OpenAPI 3.0.x is an extension of JSON Schema added by OpenAPI and has had to be clarified multiple times in the patch releases of the 3.0 spec.

To my knowledge, there is no technically correct way in OpenAPI 3.0.x to reuse the "base" (non-nullable) schema in the schema for the nullable type. But I believe this will be possible with OpenAPI v3.1, which uses "full JSON Schema". In OpenAPI v3.1 you could write:

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

There is an open issue for adding OpenAPI v3.1 support in ASP.NET - #58619. Please add a thumb's up reaction on that issue if this is important to you.

So what can be done in OpenAPI 3.0.x and .NET 9? One approach could be to avoid generating "nullable" schemas at all. In many cases the "nullability" of a property in a request or response body can be represented as being "optional" -- not in the required properties of the schema. This allows the property to be omitted from the body entirely and the Json deserializer will produce an object with a null value for that property.

This is something you can accomplish with a schema transformer that removes nullable: true from any properties that are not listed in the required keyword of the schema.

builder.Services.AddOpenApi(options =>
{
    options.AddSchemaTransformer((schema, context, cancellationToken) =>
    {
        if (schema.Properties is not null)
        {
            foreach (var property in schema.Properties)
            {
                if (schema.Required?.Contains(property.Key) != true)
                {
                    property.Value.Nullable = false;
                }
            }
        }
        return Task.CompletedTask;
    });
});

Unfortunately this does not handle the case where a nullable property is an enum. I made a couple attempts to fix the enum properties as well but none had the desired outcome. I'll continue to investigate that case but hopefully the above will be helpful to some.

@mikekistler
Copy link
Contributor

After a little more investigation and consultation with @captainsafia I now have an improved transformer that handles the NullableOf cases reported here. These arise when value types are used in nullable an non-nullable forms. In addition, nullable enums have null included in the enum values and this must be removed to allow a single schema for both nullable and non-nullable enums.

The code for this is a little more involved than the transformer I posted in the previous comment, so I will link to an implementation of it in GitHub:

https://github.com/mikekistler/aspnet-transformer-gallery/blob/main/TransformerGallery/Transformers/NullableTransformer.cs

I believe this should be a complete solution but pls post here if there are any remaining problems.

@mikekistler mikekistler added the ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. label Dec 11, 2024
@Atulin
Copy link

Atulin commented Dec 11, 2024

@mikekistler I tried this transformer, and it duplicates enum definitions it seems. One is nullable, another is not:
Image

@mikekistler
Copy link
Contributor

@Atulin This could happen if a class or struct used in a request or response body contains an ETagNamespace property that is "required". Is that possibly the case in your project?

@Atulin
Copy link

Atulin commented Dec 11, 2024

@mikekistler It's... actually not required anywhere: link to search results in the repo

@mikekistler mikekistler removed the ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. label Dec 11, 2024
@mikekistler
Copy link
Contributor

@Atulin Thank you for reporting this. I've been able to reproduce the problem so now I must figure out why this is happening.

@mikekistler
Copy link
Contributor

It looks like the problem comes from record definitions like this one:

public sealed record Command(string Name, string? Description, ETagNamespace? Namespace);

Despite being declared as nullable in this record definition, both Description and ETagNamespace are required in the corresponding schema. I would concede that this is not particularly intuitive but the behavior is documented here:

For a class or record class with a single public constructor, any property with the same type and name (case-insensitive match) as a parameter to the constructor is required in the corresponding schema.

If you search the generated OpenAPI you should find Namespace in the required list of some schema.

One solution is to declare Namespace as a property and not in the primary constructor, like this:

public sealed record Command(string Name, string? Description) {
    public ETagNamespace? Namespace { get; init; }
}

You might question the logic of making nullable parameters to the constructor required. I don't have a good answer for that, but it might be something that could be changed if there is strong demand for it from the community.

@mikekistler
Copy link
Contributor

I discovered a much simpler fix for the nullable properties in the constructor. If you simply assign them default values like this:

public sealed record Command(string Name, string? Description = null, ETagNamespace? Namespace = null);

they are no longer included in the required field of the generated schema.

But this will generate a default in the schema that needs to be handled in the NullableTransformer. I've already made that change and updated it on GitHub.

There is still the question of whether nullable parameters in the constructor should be required, but hopefully this workaround makes it easy to deal with that (along with the NullableTransformer).

But if there are any other problems still lurking on this please raise them here.

@mikekistler mikekistler added the ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. label Dec 12, 2024
@Markz878
Copy link
Author

Markz878 commented Dec 12, 2024

The code snippet you provided @mikekistler fixed this issue at least for me, thanks. Not sure if I should close this or keep it open for a while to see if others have any issues still.

I still hope this behavior would be backported to the default implementation, hard to imagine that anyone wants these NullableOf or IEnumerableOfInt64 in their OpenApi documentations..

@jornhd
Copy link

jornhd commented Dec 18, 2024

@mikekistler Thanks for the NullableTransformer, but I would keep nullable for nullable non-reference types by adding this:

        if (Nullable.GetUnderlyingType(context.JsonTypeInfo.Type) != null)
        {
            schema.Nullable = true;
        }

Edit: And still, even when the "NullableOf" prefix is gone, it insists of creating two identical enum schemas, but now with postfix "2" for the "nullable". This is very frustrating.

@mikekistler
Copy link
Contributor

@jornhd I don't quite understand your comment. Can you give an example of a property where the above logic is needed?

@jornhd
Copy link

jornhd commented Dec 19, 2024

@mikekistler Sorry, I guess I was overthinking. It's not needed.

But it there anyway to get rid of creation of the second type?

Consider these request objects/commands:
record UpdateStatus(int MyId, StatusEnum Status);
record UpdateEntity(int MyId, string Description, StatusEnum? Status = null);

Now this will result in a schema with identical StatusEnum and StatusEnum2! How do I get rid of StatusEnum2 (previously StatusEnum and NullableOfStatusEnum)?

When we use nullables in api request objects, we really mean it to be "optional" for the consuming client, so your NullableTransformer is doing the right thing. But as long as it still produces two versions of the same type, it's not ideal.

@mikekistler
Copy link
Contributor

@jornhd I think you might be getting two schemas because one has a default keyword and the other does not. If that is the case, then pull a new copy of the NullableTransformer.cs from my aspnet-transformer-gallery repo -- it removes the default: null and in my tests with the code you pasted above I only get one StatusEnum schema.

Regarding:

When we use nullables in api request objects, we really mean it to be "optional" for the consuming client,

Only properties with a required modifier or [Required] keyword are considered to be required by System.Text.Json. As a result, only these properties will be in the required list of the schema, and all others will be optional. So at least for the purposes of generating the right OpenAPI document, you would not need to make optional properties nullable. Is that an option?

@lvde0
Copy link

lvde0 commented Jan 9, 2025

I don't think this issue is resolved and it should be reopened. Yes, the workaround provided by @mikekistler works, but it should not be needed. I don't see a single useful scenario for an extra NullableOfEnum type.

@Atulin
Copy link

Atulin commented Jan 9, 2025

Or NullableOfInt64or IEnumerableOfStringfor that matter. I agree that it should probably be a single setting to opt out of this. Even if for the sake of compatibility with Nswag and Swashbuckle alone.

@lvde0
Copy link

lvde0 commented Jan 9, 2025

In general the whole project needs a compatibility layer with Swashbuckle and Nswag. Migrating code from these frameworks causing a lot of friction at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved
Projects
None yet
Development

No branches or pull requests

8 participants