[OpenApi] .NET 10, XML schema transformer applies property comments on referenced schema

[desjoerd]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

The generated schema transformer currently applies the comment on a property on a referenced schema instead of as part of the property. It also overwrites the description based on the last property transformed.

Expected Behavior

I would expect that the description of a referenced schema would be solely based on comments on that type. And for properties when it has a comment to generate something like:

"allOf": [
  { "$ref": "#/components/schemas/Address" }
],
"description": "Billing address"

But that removes some simplicity in the schemas, so it could also be that a description is not generated for "$ref" schemas.

Steps To Reproduce

Program.cs

WebApplicationBuilder builder = WebApplication.CreateBuilder(args);

builder.Services.AddOpenApi();

var app = builder.Build();

app.MapPost("/operation", (Company c) =>
{
    throw new NotImplementedException();
}).WithTags();

app.Run();

class Company
{
    /// <summary>
    /// Billing address
    /// </summary>
    public Address BillingAddress { get; set; }

    /// <summary>
    /// Visiting address
    /// </summary>
    public Address VisitingAddress { get; set; }
}

class Address
{
    public string Street { get; set; }
}

.csproj - all default, enable output on build and generate docs

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net10.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>

  <PropertyGroup>
    <GenerateDocumentationFile>true</GenerateDocumentationFile>
    <OpenApiDocumentsDirectory>.</OpenApiDocumentsDirectory>
    <OpenApiGenerateDocumentsOptions>--file-name openapi</OpenApiGenerateDocumentsOptions>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.OpenApi" Version="10.0.0-preview.4.25258.110" />
    <PackageReference Include="Microsoft.Extensions.ApiDescription.Server"
      Version="10.0.0-preview.4.25258.110">
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>
  </ItemGroup>

</Project>

Output:

{
  "openapi": "3.1.1",
  "info": {
    "title": "WebApplication5 | v1",
    "version": "1.0.0"
  },
  "paths": {
    "/operation": {
      "post": {
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "$ref": "#/components/schemas/Company"
              }
            }
          },
          "required": true
        },
        "responses": {
          "200": {
            "description": "OK"
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "Address": {
        "type": "object",
        "properties": {
          "street": {
            "type": "string"
          }
        },
        "description": "Visiting address"
      },
      "Company": {
        "type": "object",
        "properties": {
          "billingAddress": {
            "$ref": "#/components/schemas/Address"
          },
          "visitingAddress": {
            "$ref": "#/components/schemas/Address"
          }
        }
      }
    }
  }
}

Exceptions (if any)

No response

.NET Version

10.0.100-preview.4.25258.110

Anything else?

Related xml generated code

    [System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.AspNetCore.OpenApi.SourceGenerators, Version=10.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60", "10.0.0.0")]
    file class XmlCommentSchemaTransformer : IOpenApiSchemaTransformer
    {
        public Task TransformAsync(OpenApiSchema schema, OpenApiSchemaTransformerContext context, CancellationToken cancellationToken)
        {
            if (context.JsonPropertyInfo is { AttributeProvider: PropertyInfo propertyInfo })
            {
                if (XmlCommentCache.Cache.TryGetValue(propertyInfo.CreateDocumentationId(), out var propertyComment))
                {
                    schema.Description = propertyComment.Value ?? propertyComment.Returns ?? propertyComment.Summary;
                    if (propertyComment.Examples?.FirstOrDefault() is { } jsonString)
                    {
                        schema.Example = jsonString.Parse();
                    }
                }
            }
            if (XmlCommentCache.Cache.TryGetValue(context.JsonTypeInfo.Type.CreateDocumentationId(), out var typeComment))
            {
                schema.Description = typeComment.Summary;
                if (typeComment.Examples?.FirstOrDefault() is { } jsonString)
                {
                    schema.Example = jsonString.Parse();
                }
            }
            return Task.CompletedTask;
        }
    }

[desjoerd]

So thinking how to solve this, it would probably need to know whether it is a schema with a reference id, if it is, it should handle that differently.

There is also another reverse problem. When a property is not something with a schema ref (reference id == null), the description will always be the comment of the type.

So if I set, don't use reference id's (emulating that address is a struct or value type for example)

builder.Services.AddOpenApi(options =>
{
    options.CreateSchemaReferenceId = (x) => null;
});

I will get the following two results with comments:

Case: No comment on Address Type

This gives me an expected result, both have a different description.

WebApplicationBuilder builder = WebApplication.CreateBuilder(args);

builder.Services.AddOpenApi(options =>
{
    options.CreateSchemaReferenceId = (x) => null;
});

var app = builder.Build();

app.MapPost("/operation", (Company c) =>
{
    throw new NotImplementedException();
}).WithTags();

app.Run();

class Company
{
    /// <summary>
    /// Billing address
    /// </summary>
    public Address BillingAddress { get; set; }

    /// <summary>
    /// Visiting address
    /// </summary>
    public Address VisitingAddress { get; set; }
}

class Address
{
    public string Street { get; set; }
}

Output:

{
  "openapi": "3.1.1",
  "info": {
    "title": "WebApplication5 | v1",
    "version": "1.0.0"
  },
  "paths": {
    "/operation": {
      "post": {
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "type": "object",
                "properties": {
                  "billingAddress": {
                    "type": "object",
                    "properties": {
                      "street": {
                        "type": "string"
                      }
                    },
                    "description": "Billing address"
                  },
                  "visitingAddress": {
                    "type": "object",
                    "properties": {
                      "street": {
                        "type": "string"
                      }
                    },
                    "description": "Visiting address"
                  }
                }
              }
            }
          },
          "required": true
        },
        "responses": {
          "200": {
            "description": "OK"
          }
        }
      }
    }
  }
}

Case: Comment on Address Type

This gives both address properties the same description.

WebApplicationBuilder builder = WebApplication.CreateBuilder(args);

builder.Services.AddOpenApi(options =>
{
    options.CreateSchemaReferenceId = (x) => null;
});

var app = builder.Build();

app.MapPost("/operation", (Company c) =>
{
    throw new NotImplementedException();
}).WithTags();

app.Run();

class Company
{
    /// <summary>
    /// Billing address
    /// </summary>
    public Address BillingAddress { get; set; }

    /// <summary>
    /// Visiting address
    /// </summary>
    public Address VisitingAddress { get; set; }
}

/// <summary>
/// Comment on Type Address
/// </summary>
class Address
{
    public string Street { get; set; }
}

Output:

{
  "openapi": "3.1.1",
  "info": {
    "title": "WebApplication5 | v1",
    "version": "1.0.0"
  },
  "paths": {
    "/operation": {
      "post": {
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "type": "object",
                "properties": {
                  "billingAddress": {
                    "type": "object",
                    "properties": {
                      "street": {
                        "type": "string"
                      }
                    },
                    "description": "Comment on Type Address"
                  },
                  "visitingAddress": {
                    "type": "object",
                    "properties": {
                      "street": {
                        "type": "string"
                      }
                    },
                    "description": "Comment on Type Address"
                  }
                }
              }
            }
          },
          "required": true
        },
        "responses": {
          "200": {
            "description": "OK"
          }
        }
      }
    }
  }
}

Maybe the logic should be something like:

if schema has reference id:
    set description from type on the schema in components
    set description from property on the property schema, with an "allOf" block
else:
    set description from property (and fallback to type)

The big question is whether rewriting the property schema to set the description on a property is something which is wanted/expected behavior. (Imho it's correct but it could surprise people).

{
    "$ref": "#/components/schemas/Address"
}

To

"allOf": [
  { "$ref": "#/components/schemas/Address" }
],
"description": "Billing address"

[captainsafia]

@desjoerd Thanks for filing this issue and doing the heavy lifting to get repros/ideas squared away here.

With OpenAPI 3.1, I believe we can take advantage of the face that OpenApiSchemaReference objects (ref) can contain description properties. That should allow us to emit a schema description properties directly alongside the ref without having to use an allIOf. We'll need to update the schema transformer to distinguish between OpenApiSchemaReference and OpenApiSchema types.

With regard to the behavior you're seeing where type-level comments override property-level ones, I believe that's related to the application order we use in the emitted code. I think it's probably safe to switch this ordering. If you have a property, the type-level docs shouldn't override it.

Any interest in opening a PR to address this issue?

[desjoerd]

After some digging, I think you are right, $ref merges instead of replaces in 2020-12 and doesn't touch annotations, which "description" is.

I am open to create a PR for it. Looking at the unit tests I think I have most of the bits and pieces to fix and test it.

For the schema transformer OpenApiSchemaReference vs OpenApiSchema, would just changing the argument to the interface IOpenApiSchema work, as both implement that? It would mean a breaking change, but I think as soon as someone used transformers in .NET 9 they already need to handle breaking changes.

[captainsafia]

For the schema transformer OpenApiSchemaReference vs OpenApiSchema, would just changing the argument to the interface IOpenApiSchema work, as both implement that? It would mean a breaking change, but I think as soon as someone used transformers in .NET 9 they already need to handle breaking changes.

Yep, we've been communicating the nature of the breaking changes between .NET 9 and .NET 10 since we are replatting onto the new version of the Microsoft.OpenApi library.

We could change the schema transformer API to take an IOpenApiSchema but the implementation currently assumes that all schema transforrmers run on the resolved schema (not the reference) to maintain behavioral compatibility with .NET 9. I'm a nit more cautious about introducing a behavioral change here. It would require that users check if an IOpenApiSchema is a reference or a direct entity frequently in their code.

Perhaps another way to do this is to use the operation transformer emitted by the source generator which operates on the IOpenApiOperation that has a reference to an IOpenApiSchema to support setting the description property on the reference instead of the resolved schema. Thoughts on this idea?

[desjoerd]

If it is on the IOperationTransformer it would need to be recursive, following all references so I don't think that is an option.

I just got a maybe crazy idea, a reference transformer or a CreateSchemaReference option, or even a list of SchemaReferenceCreators. Currently it's hard to override the CreateSchemaReferenceId func from a library, which basically means, wrap the current func, if it's a certain type do the logic in the library else do the previous.

Example usage in my undefined wrapper library -> https://github.com/desjoerd/OptionalValues/blob/ccdab785c58e7660444feb71f512c83a8b222a91/examples/OptionalValues.Examples.OpenApi/Program.cs#L16
This is going to be moved to a IConfigureOptions or an extension method. But it requires users to wrap CreateSchemaReferenceId as well.

sorry for brevity, from my phone

[captainsafia]

Pardon the dense question, but I don't quite see how a CreateSchemaReference option would help here? 😅

I wonder if another way to address this is to make a change in the ResolveReferenceForSchema method that would check if any of the properties it is resolving references for have descriptions already set on them. If so, instead of returning the schema reference directly, it should hoist the description up to the reference then return it. Thoughts on this?

[desjoerd]

I was thinking of an overload or more sophisticated CreateSchemaReference which creates the OpenApiSchemaReference itself, but I think that happens earlier in the process so it was an idea, but not a good one!

I think your solution of merging the description on the schema reference should work also when looking at ordering.

I think it should hoist all annotations to the reference.

But I think it only should copy it to the reference when it's coming from a property comment. Otherwise you would get loads of duplications. I think that would require the Xml transformer to know whether the schema it's transforming is going to be a reference, but I am not sure if that is the case.

[captainsafia]

But I think it only should copy it to the reference when it's coming from a property comment.

Yep, I missed this in my original comment but it would only apply in the ResolveReferenceForSchema case where we are iterating through properties.

I think that would require the Xml transformer to know whether the schema it's transforming is going to be a reference, but I am not sure if that is the case.

I don't think it should, but we might be able to see what happens in practice.

[desjoerd]

Doing it as part of ResolveReferenceForSchema should work indeed.

On another note, I was trying to run the unit tests on the generation to do the first fix (order in the emitter) but as I've got the Dutch locale for numbers it generates the floating point numbers as 3,14. I don't know why yet because it looks to be using culture invariant or apis which I would assume to be invariant. I can create another issue for that for tracking.

[captainsafia]

@desjoerd I believe there may be an issue in the backlog already pertaining to this that I haven't gotten around to yet. :/ A quick glance at items labelled feature-openapi will help you see if it already exists

We really need to set up some infrastructure for testing culture-invariance on these things.