OpenAPI schema references feedback #56318
Comments
|
@martincostello Wow! I think you've won the award for the fastest commit merged -> feedback given time. 😸 Your feedback aligns with some of the things I expected to hear so I appreciate you chiming in and affirming some of my hunches here.
The implementation creates reference schemas for any schema that appears more than once in the document. This has a particularly sharp impact for schemas associated with primitive types. I'm open to adjusting this although we'd have to go through the exercise of figuring out what primitives should be inline and which shouldn't.
I suspect the issue here is in the nullability checks below. These need to account for nullable reference types like aspnetcore/src/OpenApi/src/Extensions/JsonTypeInfoExtensions.cs Lines 104 to 108 in 613ac88
It looks like the We did discuss the possibility of this particular problem occuring. I had concluded that it would be ✨ rare ✨ given that it seems like most REST APIs use the same DTO in multiple places, although your sample app has provided a good counter point here. I'll noodle more on how we might want to approach this. |
|
Having primitives as full schemas is very... unusual and I think plainly incorrect. I wouldn't expect a schema for anything that isn't a class with properties, as all the primitives, etc. are definable directly within the openapi spec otherwise. OpenAPI has a well defined specification for data types and formats of which "string" is an integral part. I think anything they define with formats/data types ought not be a whole schema which is likely in violation of the spec. A seperate schema for the nullable and non-nullable versions of a type also feels incorrect in prescense of the nullable attribute/property. Schemas with symbols/numbers in the name are very odd and I'd not expect that unless the user designated them that way via some customizations. I suspect this would break a lot of tools that read the document (code generators for one). If this was the finalized schema I would simply opt out altogether and continue using other libraries. Please reconsider. |
I think for this one it's more that it's different to what I'm used to in .NET. If it's more technically correct (the best kind of correct) to inline things as much as possible, then it's probably better to go in that direction even if it feels "weird" for us now. On balance if it's not technically difficult if it were me I'd still be inclined to go with the not inlining "primitives" as noted above based on the commonly known built-in OpenAPI data types, at least for the "obvious" types like strings, "numbers" (int, long, float, double, decimal), booleans etc. There'd probably need to be a discussion like you mention that draws a line between primitive and not that doesn't get too wrapped up in the .NET type system but doesn't seem too arbitrary.
Here I think I'd prefer the consistency (though it kinda contradicts my point above 😄) of having all the object-like types being defined in the components consistently, rather than them only not being inlined if used more than once.
Having just tried to generate a client for the OpenAPI specification generated by the latest bits ( Running Kiota, it outputs this failure message (the warnings are #56188): ❯ kiota generate -l CSharp -c ApiClient -n KiotaExperiment.Client -d ./openapi.json -o ./Client
warn: Kiota.Builder.KiotaBuilder[0]
OpenAPI warning: #/ - A servers entry (v3) or host + basePath + schemes properties (v2) was not present in the OpenAPI description. The root URL will need to be set manually with the request adapter.
- fail: Kiota.Builder.KiotaBuilder[0]
- OpenAPI error: #/components - The key 'int?' in 'schemas' of components MUST match the regular expression '^[a-zA-Z0-9\.\-_]+$'.
warn: Kiota.Builder.KiotaBuilder[0]
No server url found in the OpenAPI document. The base url will need to be set when using the client.
Generation completed successfullyAdmittedly this is a sample size of 1 (one) code generator, but given its one of the ones being promoted around this initiative, the schemas shouldn't be producing errors in client-side tooling. As noted above, this would likely be a big barrier (or blocker) to adoption within the community. The generated code does build though (I haven't actually pointed it at the API to see if it works yet). |
It does work - I've pushed the code up here. |
|
@martincostello @pinkfloydx33 Thanks for all your feedback here. I've been iterating on it in the PRs linked above. Can you try the changes in Microsoft.AspNetCore.OpenApi 9.0.0-preview.7.24353.6 and see how they work for you? |
captainsafia
added
the
Needs: Author Feedback
|
@captainsafia Comparing things to NSwag, things look like they're getting a lot more similar now: martincostello/api@684485c Remaining differences that are potentially interesting:
Otherwise things seem to be coming out functionally the same in terms of schemas/references. |
dotnet-policy-service
bot
added
Needs: Attention 👋
|
Also, I just regenerated a Kiota client from the new OpenAPI document and I get these warnings: |
Yep, for now, we only support setting descriptions via the
Hmmm....we have test coverage for this here. Can you clue me in on which type isn't handling this correctly in your sample app and I can look further?
Ooooooh! Good find. I think this one falls into the realm of things the
Interesting...I'll try to take a look at this next week. |
captainsafia
removed
the
Needs: Attention 👋
Comparing the two documents, it looks like it's |
Hm....I think I might be missing something here. I see the openapi.json has three items in its required list and so does swagger.json. Are you referring to the difference in |
|
I'll look at it again in BeyondCompare when I'm next at my laptop and see where I've gotten confused... |
|
It was this. If I understand correctly, because |
I went on a journey with this one but as you can see from the comments in the associated PR it looks like we actually are spec-compliant here. OpenAPI, like JSON Schema, assumes that
Ah, I see. I assume this is a constraint that you can only apply on required string values. I agree that it's not clear how helpful |
OK, I took a look at this and here's what I understand. The warnings that you are seeing here come from a set of validation rules in the OpenAPI library. It appears that these rules attempt to validate that you are using the correct types in your examples based on the schemas for properties in a document. For example, if a schema is It seems like the types it's buggy about are ones where a |
|
Thanks for looking - I haven't used Kiota with my NSwag schemas, I've only started playing with it as part of testing the new OpenAPI stuff. I'll dig into this further today as I'm reworking some of my customisation on top of #56395. Separately, I haven't worked out why yet, but trying to apply examples to the schemas they don't seem, to be coming out in the rendered document. Not sure if that's my bug or something in the library yet. Essentially I'm doing this: public Task TransformAsync(
OpenApiSchema schema,
OpenApiSchemaTransformerContext context,
CancellationToken cancellationToken)
{
var metadata = context.JsonTypeInfo.Type.GetCustomAttributes(false)
.OfType<IOpenApiExampleMetadata>()
.FirstOrDefault();
if (metadata?.GenerateExample(_options) is { } value)
{
schema.Example = value;
}
return Task.CompletedTask;
}
but it's not rendered in the schema component:
|
Still digging, but I think it's an issue in Microsoft.OpenApi - the |
|
|
Checked, and I get the behaviour I was originally expecting with that change applied locally. |
|
Something else I'm just trying to hack together until #39927 is ready is to populate the descriptions for my schema models from my XML comments. However, I'm stuck because I can't see how to get the original name of the property associated with a schema so I can build the right xpath. (Ignore how hacky and unperformant this is, just trying to get something basic working first) public Task TransformAsync(OpenApiSchema schema, OpenApiSchemaTransformerContext context, CancellationToken cancellationToken)
{
var thisAssembly = GetType().Assembly;
if (context.JsonPropertyInfo?.DeclaringType.Assembly != thisAssembly)
{
return Task.CompletedTask;
}
var path = Path.GetDirectoryName(thisAssembly.Location) ?? Environment.CurrentDirectory;
var xml = Path.Combine(path, "API.xml");
using var reader = XmlReader.Create(xml);
var xpath = new XPathDocument(reader);
var navigator = xpath.CreateNavigator();
var description = navigator.SelectSingleNode(
$"/doc/members/member[@name='P:{context.JsonPropertyInfo.DeclaringType.FullName}.{context.JsonPropertyInfo.Name}']/summary");
if (!string.IsNullOrEmpty(description?.InnerXml))
{
schema.Description = description.Value.Trim();
}
return Task.CompletedTask;
}
Similarly, there doesn't seem to be a way to get the name of the property a schema component is being written as. Am I missing something on where this information is kept, or do I need to use a document transformer to try and piggy-back some additional data through to read later in the schema transformer? |
Thanks for digging! There have been other bugs related to the copy constructors in OpenAPI.NET in the past. Perhaps this is an area where we can contribute improved tests in the library...I'll chime in in the issue that you opened.
I ran into this issue as well in my implementation and ended up applying a workaround to do lookups by property name in the transformer. Perhaps we should open an issue on the runtime repo about making
I think you're referring to the schema ID here? If so, you can use the Out of curiousity, what scenario requires that you know the schema ID (assuming I understood that part correctly)? |
I added a few related to the specific bug, but feel free to suggest any other areas that might be lacking.
Yep, I'll open something shortly.
Yep, my thinking was to use it to get the class name backing the schema so I could get the summary text for the class out of the XML documentation to set the description. I'll see if |
Oh, I see. In that case, isn't |
|
I may have gotten confused somewhere so I'll need to check again, but in this example: I was getting calls into the schema about the properties for Foo's schema, and the information about what type it was, but I wasn't explicitly getting the key value for If |
Raised dotnet/runtime#105155. |
Ah, I see. You should be able to access this via How are you using the schema reference ID in your scenario? |
|
I probably just got confused and should have just been using I'll try out my hacky code again and see where I went wrong, but it's good to know that if there's a reason someone really needs to know the key that goes with the schema for the component they can get hold of it. |
|
So I think I've found the source of my confusion - either I'm being stupid, or this is a bug. I've pushed the code up here if you want to take a look: martincostello/api@12237af It seems like when I get the first call into my transformer, which is the schema for a the full containing type This then leads me to incorrectly assign the summary for the last property in the schema for the object as the summary for the schema object itself. Feels like there's a bug and the context for the top-level schema objects is incorrect. This is the "TimeResponse": {
"type": "object",
"properties": {
"timestamp": {
"type": "string",
"description": "The timestamp for the response for which the times are generated.",
"format": "date-time"
},
"rfc1123": {
"type": "string",
"description": "The current UTC date and time in RFC1123 format."
},
"unix": {
"type": "integer",
"description": "The number of seconds since the UNIX epoch.",
"format": "int64"
},
"universalSortable": {
"type": "string",
"description": "The current UTC date and time in universal sortable format."
},
"universalFull": {
"type": "string",
"description": "The current UTC date and time in universal full format."
}
},
"description": "The current UTC date and time in universal full format."
} |
Hmmm....I'm getting vibes here too... Mind filling a new issue for this and I can take a look? |
|
With the fix for #56899 I think everything in the feedback is addressed now. I just need to resolve the Kiota-related warnings my spec creates. Excluding the changes I need from microsoft/OpenAPI.NET#1736 to make functionality equivalent, every difference between my OpenAPI schema and the NSwag equivalent has a justification for now. |
|
For reference, this is what I came up with to populate the using System.Collections.Concurrent;
using System.Reflection;
using System.Text.Json.Serialization.Metadata;
using System.Xml;
using System.Xml.XPath;
using Microsoft.OpenApi.Models;
namespace Microsoft.AspNetCore.OpenApi;
internal sealed class AddSchemaDescriptionsTransformer : IOpenApiSchemaTransformer
{
private readonly Assembly _thisAssembly = typeof(AddSchemaDescriptionsTransformer).Assembly;
private readonly ConcurrentDictionary<string, string?> _descriptions = [];
private XPathNavigator? _navigator;
public Task TransformAsync(
OpenApiSchema schema,
OpenApiSchemaTransformerContext context,
CancellationToken cancellationToken)
{
if (schema.Description is null &&
GetMemberName(context.JsonTypeInfo, context.JsonPropertyInfo) is { Length: > 0 } memberName &&
GetDescription(memberName) is { Length: > 0 } description)
{
schema.Description = description;
}
return Task.CompletedTask;
}
private string? GetDescription(string memberName)
{
if (_descriptions.TryGetValue(memberName, out string? description))
{
return description;
}
var navigator = CreateNavigator();
var node = navigator.SelectSingleNode($"/doc/members/member[@name='{memberName}']/summary");
if (node is not null)
{
description = node.Value.Trim();
}
_descriptions[memberName] = description;
return description;
}
private string? GetMemberName(JsonTypeInfo typeInfo, JsonPropertyInfo? propertyInfo)
{
if (typeInfo.Type.Assembly != _thisAssembly &&
propertyInfo?.DeclaringType.Assembly != _thisAssembly)
{
return null;
}
else if (propertyInfo is not null)
{
string? typeName = propertyInfo.DeclaringType.FullName;
string propertyName =
propertyInfo.AttributeProvider is PropertyInfo property ?
property.Name :
$"{char.ToUpperInvariant(propertyInfo.Name[0])}{propertyInfo.Name[1..]}";
return $"P:{typeName}{Type.Delimiter}{propertyName}";
}
else
{
return $"T:{typeInfo.Type.FullName}";
}
}
private XPathNavigator CreateNavigator()
{
if (_navigator is null)
{
string path = Path.Combine(AppContext.BaseDirectory, $"{_thisAssembly.GetName().Name}.xml");
using var reader = XmlReader.Create(path);
_navigator = new XPathDocument(reader).CreateNavigator();
}
return _navigator;
}
} |
|
Hot off the presses, the app I'm testing daily builds and OpenAPI with has picked up the changes from #56175 and there's a few things in the new schema that seem a bit odd to me. Some might be intentional, some might be known and just not gotten to yet, so sorry if any of this is just noise.
The observations are also based on comparison to NSwag's and Swashbuckle.AspNetCore's current behaviour, though I appreciate one-to-one parity isn't a goal.
The points referenced below are all from commit martincostello/api@f72f01b using Microsoft.AspNetCore.OpenApi
9.0.0-preview.6.24318.18. The schema generated is at the bottom of this issue.Overly inlined schemas?
One of the components in the schema is this:
This seems maybe a bit too normalized compared to the relevant properties in an associated model just being declared as of type string directly?
Question marks in schema names
One of the schema names is generated thus:
I wonder if some client generation tooling might have issues with trying to generate models with a
?in the name? It's at least not something I've seen before.Inconsistent schema names for nullable types?
In the generated OpenAPI document is the following schema:
This seems to be inconsistent with the
int?schema - assuming the question marks as intentional in the names. I would have thoughtstring?would be the more appropriate name for the sake of comparison with another nullable type.Inline objects still rendered for operations
The following schema is present inline for the response of one of the operations:
I would have expected the response to be a reference to a schema for the type as a whole, and then the model emitted as a component which is itself composed of three string properties (notwithstanding the point above about maybe too much inlining).
/cc @captainsafia
OpenAPI schema
{ "openapi": "3.0.1", "info": { "title": "api.martincostello.com", "description": "Martin Costello's API", "contact": { "name": "Martin Costello", "url": "https://martincostello.com/" }, "license": { "name": "This API is licensed under the MIT License.", "url": "https://github.com/martincostello/api/blob/main/LICENSE" }, "version": "" }, "paths": { "/time": { "get": { "tags": [ "API" ], "summary": "Gets the current UTC time.", "description": "Gets the current date and time in UTC.", "operationId": "Time", "responses": { "200": { "description": "The current UTC date and time.", "content": { "application/json": { "schema": { "type": "object", "properties": { "timestamp": { "type": "string", "format": "date-time" }, "rfc1123": { "$ref": "#/components/schemas/string" }, "unix": { "type": "integer", "format": "int64" }, "universalSortable": { "$ref": "#/components/schemas/string" }, "universalFull": { "$ref": "#/components/schemas/string" } } }, "example": { "timestamp": "2016-06-03T18:44:14+00:00", "rfc1123": "Fri, 03 Jun 2016 18:44:14 GMT", "unix": 1464979454, "universalSortable": "2016-06-03 18:44:14Z", "universalFull": "Friday, 03 June 2016 18:44:14" } } } } } } }, "/tools/guid": { "get": { "tags": [ "API" ], "summary": "Generates a GUID.", "description": "Generates a new GUID in the specified format.", "operationId": "Guid", "parameters": [ { "name": "format", "in": "query", "schema": { "type": "string", "description": "The format for which to generate a GUID.", "nullable": true }, "example": "D" }, { "name": "uppercase", "in": "query", "schema": { "type": "boolean", "description": "Whether to return the GUID in uppercase.", "nullable": true } } ], "responses": { "200": { "description": "A GUID was generated successfully.", "content": { "application/json": { "schema": { "type": "object", "properties": { "guid": { "$ref": "#/components/schemas/string" } } }, "example": { "guid": "6bc55a07-3d3e-4d52-8701-362a1187772d" } } } }, "400": { "description": "The specified format is invalid.", "content": { "application/problem+json": { "schema": { "$ref": "#/components/schemas/ProblemDetails" }, "example": { "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1", "title": "Bad Request", "status": 400, "detail": "The specified value is invalid." } } } } } } }, "/tools/hash": { "post": { "tags": [ "API" ], "summary": "Hashes a string.", "description": "Generates a hash of some plaintext for a specified hash algorithm and returns it in the required format.", "operationId": "Hash", "requestBody": { "content": { "application/json": { "schema": { "required": [ "algorithm", "format", "plaintext" ], "type": "object", "properties": { "algorithm": { "$ref": "#/components/schemas/string" }, "format": { "$ref": "#/components/schemas/string" }, "plaintext": { "$ref": "#/components/schemas/string" } }, "nullable": true }, "example": { "algorithm": "sha256", "format": "base64", "plaintext": "The quick brown fox jumped over the lazy dog" } } } }, "responses": { "200": { "description": "The hash was generated successfully.", "content": { "application/json": { "schema": { "type": "object", "properties": { "hash": { "$ref": "#/components/schemas/string" } } }, "example": { "hash": "fTi1zSWiuvha07tbkxE4PmcaihQuswKzJNSl+6h0jGk=" } } } }, "400": { "description": "The specified hash algorithm or output format is invalid.", "content": { "application/problem+json": { "schema": { "$ref": "#/components/schemas/ProblemDetails" }, "example": { "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1", "title": "Bad Request", "status": 400, "detail": "The specified value is invalid." } } } } } } }, "/tools/machinekey": { "get": { "tags": [ "API" ], "summary": "Generates a machine key.", "description": "Generates a machine key for a Web.config configuration file for ASP.NET.", "operationId": "MachineKey", "parameters": [ { "name": "decryptionAlgorithm", "in": "query", "schema": { "type": "string", "description": "The name of the decryption algorithm.", "nullable": true }, "example": "AES-256" }, { "name": "validationAlgorithm", "in": "query", "schema": { "type": "string", "description": "The name of the validation algorithm.", "nullable": true }, "example": "SHA1" } ], "responses": { "200": { "description": "The machine key was generated successfully.", "content": { "application/json": { "schema": { "type": "object", "properties": { "decryptionKey": { "$ref": "#/components/schemas/string" }, "validationKey": { "$ref": "#/components/schemas/string" }, "machineKeyXml": { "$ref": "#/components/schemas/string" } } }, "example": { "decryptionKey": "2EA72C07DEEF522B4686C39BDF83E70A96BA92EE1D960029821FCA2E4CD9FB72", "validationKey": "0A7A92827A74B9B4D2A21918814D8E4A9150BB5ADDB284533BDB50E44ADA6A4BCCFF637A5CB692816EE304121A1BCAA5A6D96BE31A213DEE0BAAEF102A391E8F", "machineKeyXml": "<machineKey validationKey=\"0A7A92827A74B9B4D2A21918814D8E4A9150BB5ADDB284533BDB50E44ADA6A4BCCFF637A5CB692816EE304121A1BCAA5A6D96BE31A213DEE0BAAEF102A391E8F\" decryptionKey=\"2EA72C07DEEF522B4686C39BDF83E70A96BA92EE1D960029821FCA2E4CD9FB72\" validation=\"SHA1\" decryption=\"AES\" />" } } } }, "400": { "description": "The specified decryption or validation algorithm is invalid.", "content": { "application/problem+json": { "schema": { "$ref": "#/components/schemas/ProblemDetails" }, "example": { "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1", "title": "Bad Request", "status": 400, "detail": "The specified value is invalid." } } } } } } } }, "components": { "schemas": { "int?": { "type": "integer", "format": "int32", "nullable": true }, "ProblemDetails": { "type": "object", "properties": { "type": { "$ref": "#/components/schemas/string2" }, "title": { "$ref": "#/components/schemas/string2" }, "status": { "$ref": "#/components/schemas/int?" }, "detail": { "$ref": "#/components/schemas/string2" }, "instance": { "$ref": "#/components/schemas/string2" } } }, "string": { "type": "string" }, "string2": { "type": "string", "nullable": true } } }, "tags": [ { "name": "API" } ] }The text was updated successfully, but these errors were encountered: