OpenAPI schema references feedback

[martincostello]

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:

"string": {
  "type": "string"
}

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:

"int?": {
  "type": "integer",
  "format": "int32",
  "nullable": true
}

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:

"string2": {
  "type": "string",
  "nullable": true
}

This seems to be inconsistent with the int? schema - assuming the question marks as intentional in the names. I would have thought string? 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:

"schema": {
  "type": "object",
  "properties": {
    "decryptionKey": {
      "$ref": "#/components/schemas/string"
    },
    "validationKey": {
      "$ref": "#/components/schemas/string"
    },
    "machineKeyXml": {
      "$ref": "#/components/schemas/string"
    }
  }
}

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"
    }
  ]
}

[captainsafia]

@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.

Overly inlined schemas?

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.

Inconsistent schema names for nullable types?

I suspect the issue here is in the nullability checks below. These need to account for nullable reference types like string.

aspnetcore/src/OpenApi/src/Extensions/JsonTypeInfoExtensions.cs

Lines 104 to 108 in 613ac88

	if (type.GetGenericTypeDefinition() == typeof(Nullable<>)
	&& Nullable.GetUnderlyingType(type) is { } underlyingType)
	{
	return $"{underlyingType.GetSchemaReferenceId(options)}?";
	}

Inline objects still rendered for operations

It looks like the MachineKeyResponse type returned here is only used once in the entire application? If so, this is matches the expected behavior of this implementation.

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.

[pinkfloydx33]

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.

[martincostello]

Overly inlined schemas?

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.

Inline objects still rendered for operations

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.

Question marks in schema names

Having just tried to generate a client for the OpenAPI specification generated by the latest bits (9.0.0-preview.6.24320.8) by following this tutorial, I think this needs rethinking and the ? removing.

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 successfully

Admittedly 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).

[martincostello]

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.

[captainsafia]

@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?

[martincostello]

@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:

• Descriptions are missing for models parameters - I know this is in the epic for supporting /// docs to populate them at some point.
• [Required] appears to not being honoured on properties from request body models.
• ProblemDetails' Extensions property with [JsonExtensionData] isn't generating "additionalProperties": {}.

Otherwise things seem to be coming out functionally the same in terms of schemas/references.

[martincostello]

Also, I just regenerated a Kiota client from the new OpenAPI document and I get these warnings:

.\generate.ps1 -Regenerate
warn: Kiota.Builder.KiotaBuilder[0]
      OpenAPI warning: #/paths/~1time/get/responses/200/content/application~1json/example/rfc1123 - Data and type mismatch found.
warn: Kiota.Builder.KiotaBuilder[0]
      OpenAPI warning: #/paths/~1time/get/responses/200/content/application~1json/example/unix - Data and type mismatch found.
warn: Kiota.Builder.KiotaBuilder[0]
      OpenAPI warning: #/paths/~1time/get/responses/200/content/application~1json/example/universalSortable - Data and type mismatch found.
warn: Kiota.Builder.KiotaBuilder[0]
      OpenAPI warning: #/paths/~1time/get/responses/200/content/application~1json/example/universalFull - Data and type mismatch found.