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

Microsoft.AspNetCore.OpenApi specifies non-nullable get-only properties as nullable #58192

Open
1 task done
keenjus opened this issue Oct 1, 2024 · 3 comments
Open
1 task done
Assignees
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

Comments

@keenjus
Copy link

keenjus commented Oct 1, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

If nullable reference types are enabled and a class has a get only non-nullable property, Microsoft.AspNetCore.OpenApi will incorrectly specify the property as nullable.

I looked at the source code and it seems like JsonNodeSchemaExtensions.ApplyNullabilityContextInfo(JsonNode, JsonPropertyInfo) is the culprit. Specifically this part propertyInfo.IsGetNullable || propertyInfo.IsSetNullable.

Should it even check for IsSetNullable when the property does not have a setter?

A temporary workaround for the library consumer would be to introduce an unused setter, eg

public IEnumerable<string> Values
{
  get => [];
  private set {}
}

Expected Behavior

Microsoft.AspNetCore.OpenApi generates correct nullability information for schema properties.

Steps To Reproduce

https://github.com/keenjus/OpenApiStuff/tree/2e83b32a316500018eb216be015c677933cfa73b

Launch the project and inspect the generated OpenApi document at /openapi/v1.json. The property values is specified as nullable for EntityCompany/EntityPerson schemas.

Exceptions (if any)

No response

.NET Version

9.0.100-rc.1.24452.12

Anything else?

Microsoft.AspNetCore.OpenApi 9.0.0-rc.1.24452.1

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Oct 1, 2024
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Oct 1, 2024
@captainsafia
Copy link
Member

@keenjus Thanks for filing this issue!

The ApplyNullabilityContextInfo line that you've identified attempts to match the behavior of the underlying JsonSchemaExporter (ref).

The reason we do this is because OpenAPI's schemas are supersets of the official JSON schema specification. While JSON schema will permit you to use { type: ["array", "null"] } to convey that a type is nullable array, OpenAPI schema requires that you represent that as { type: "array", nullable: true }.

In this particular case, IsSetNullable and IsGetNullable are derived based on whether the property type is nullable (ref).

As to why it gets represent this way, assuming you have the following type:

Assuming the following type:

public class Test
{
    public IEnumerable<string> Values { get; }
    public IEnumerable<string> Values2 => [];
    public IEnumerable<string> Values3 { get; private set; }
}

You'll get the following result when you try to deserialize "{}":

{"values":null,"values2":[],"values3":null}

values and values3 do indeed serialize to null values. values2 doesn't because of the default getter. This is an exception though because the schema generator doesn't factor this in when generating the schema.

There is a RespectsNullableAnnotation (ref) option that allows you to modify this behavior, but it has a notable constraint that impacts your scenario:

Due to restrictions in how nullable reference types are represented at run time, this setting only governs nullability annotations of non-generic properties, fields, and constructor parameters. It cannot be used to enforce nullability annotations of root-level types, collection elements, or generic parameters.

Any general fix I can think of comes with a catch-22 WRT to breaking other things, the best things I can think of for your case are using schema transformers.

Sidenote: I'm a little surprised that your workaround works. My own debugging reveals that IsGetNullable = False and IsSetNullable = True for that scenario so there might be something more going on here.

@captainsafia captainsafia added the ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. label Oct 3, 2024
@keenjus
Copy link
Author

keenjus commented Oct 4, 2024

I did try a custom schema transformer already with some very crude overrides

foreach (var property in schema.Properties)
{
    var jsonProperty = context.JsonTypeInfo.Properties.FirstOrDefault(x => x.Name == property.Key);
    if (jsonProperty == null) continue;
    
    // Only taking IsSetNullable into account if a setter actually exists
    // Should probably do something similar with IsGetNullable?
    property.Value.Nullable = jsonProperty.IsGetNullable ||
                              jsonProperty is { Set: not null, IsSetNullable: true };

    // Make the property readonly (as per OpenAPI v3 "readOnly properties are included in responses but not in requests")
    property.Value.ReadOnly = jsonProperty is { Get: not null, Set: null };

    // Make the property required if it is not nullable and can only be used for responses.
    // Otherwise the resulting TypeScript definition would make it optional eg. `readonly values?: string[]`
    if (property.Value.ReadOnly && !property.Value.Nullable)
    {
        schema.Required.Add(property.Key);
    }
}

C# class

public class Entity
{
  public IEnumerable<string> Values => [];
}

Generated TypeScript definition using the openapi-typescript npm package.

Entity: {
    readonly values: string[];
}

@captainsafia

Any general fix I can think of comes with a catch-22 WRT to breaking other things

What would this kind of behavior change break in Microsoft.AspNetCore.OpenApi? Why is not logical to only check for IsSetNullable if a setter actually exists?

Copy link
Contributor

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@captainsafia captainsafia reopened this Oct 5, 2024
@captainsafia captainsafia removed ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved labels Oct 5, 2024
@captainsafia captainsafia self-assigned this Oct 5, 2024
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
Projects
None yet
Development

No branches or pull requests

3 participants