Skip to content

Support options for setting callback used to generated reference IDs in OpenAPI #56305

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
captainsafia opened this issue Jun 18, 2024 · 8 comments · Fixed by #56753
Closed

Support options for setting callback used to generated reference IDs in OpenAPI #56305

captainsafia opened this issue Jun 18, 2024 · 8 comments · Fixed by #56753
Labels
api-approved API was approved in API review, it can be implemented 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
Milestone

Comments

@captainsafia
Copy link
Member

captainsafia commented Jun 18, 2024

Background and Motivation

The OpenAPI implementation generates reference IDs for schemas that are duplicated across the OpenAPI document. By default, a unique identifier is generated for the schema using a combination of the schema itself and the type associated with the schema. At times, it might be necessary for users to override our default behavior with their own reference ID generation function.

Proposed API

namespace Microsoft.AspNetCore.OpenApi;

public sealed class OpenApiOptions
{
+    public Func<JsonTypeInfo, string?> CreateSchemaReferenceId { get; set; }
}

Usage Examples

var builder = WebApplication.CreateBuilder();

builder.Services.AddOpenApi(options =>
{
	CreateSchemaReferenceId = (jsonTypeInfo) => $"My{jsonTypeInfo.Type.Name}",
});
builder.Services.AddOpenApi(options =>
{
	// Returning null means that all schemas will be inlined, can also be used to
	// conditional inline a schema
	CreateSchemaReferenceId = (jsonTypeInfo) => null,
});

var app = builder.Build();

app.MapOpenApi();

app.MapGet("/", () => "Hello world!");

app.Run();

Alternative Designs

Instead of specifying custom schema IDs via a callback, we could support a custom attribute on types that would allow users to set reference IDs for the types they care about. For example:

[SchemaReferenceId("some-name-here")]
public class Todo { }

Risks

Exposing an option for customizing the way reference IDs are created adds additional burden onto the user to ensure that the callback they provide will generate unique and accurate reference IDs for all the types that are used in their application.

@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc labels Jun 18, 2024
@michael-wolfenden
Copy link

@captainsafia In my api's I like to nest the request / responses objects within the class containing the endpoint mapping as well as the implementation.

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddOpenApi();

var app = builder.Build();
app.MapOpenApi();

CreateArticle.MapEndpoint(app);
UpdateArticle.MapEndpoint(app);

app.Run();

public class CreateArticle
{
    public static RouteHandlerBuilder MapEndpoint(IEndpointRouteBuilder endpoints) =>
        endpoints.MapPost("/articles", async (Request request) => { });

    public record Request(string title);
}

public class UpdateArticle
{
    public static RouteHandlerBuilder MapEndpoint(IEndpointRouteBuilder endpoints) =>
        endpoints.MapPost("/articles/{id}", async (string id, Request request) => { });

    public record Request(string title, string description);
}

However this results in schema's named Request and Request2.

Would this feature allow me to generate ids like CreateArticleRequest and UpdateArticleRequest based on the type, like squashbuckle's CustsomSchemaIds option?

@captainsafia
Copy link
Member Author

In my api's I like to nest the request / responses objects within the class containing the endpoint mapping as well as the implementation.

Oh! I really like this organization structure.

Would this feature allow me to generate ids like CreateArticleRequest and UpdateArticleRequest based on the type, like squashbuckle's CustsomSchemaIds option?

Yes -- it would! I'm actually hoping to take this API through review (ref) tomorrow. Do you have any thoughts on the shape I am currently proposing for it that we should consider as part of the review?

@michael-wolfenden
Copy link

Awesome @captainsafia, your initial suggestion makes the most sense to me

public Func<Type, OpenApiSchema, string> CreateReferenceId { get; set; }

@captainsafia
Copy link
Member Author

@michael-wolfenden I ended up making a few changes to the API ahead of today's API review after I started implementing it. You should still be able to achieve the desired result but the shape is a little different now.

@amcasey
Copy link
Member

amcasey commented Jul 11, 2024

[API Review]

  • What happens if CreateSchemaReferenceId returns the same value twice?
    • We have centralized de-duping that tacks on sequential integers
    • This is a way users can make that de-duping less likely by more fully qualifying their names
  • Why is the return value nullable?
    • Can return null to prevent referencing (i.e. force inlining)
    • People might plausibly think it means "use the default schema name"
  • The default implementation isn't just trivially returning the name
  • Options:
      1. Make signature Func<JsonTypeInfo, string>, always require returning a value
      1. Keep signature Func<JsonTypeInfo, string?> and say that null means we call the framework default
      1. Keep the signature as it is and provide the framework default as an option the user can invoke
      1. Separate filtering from naming with an out param
CreateSchemaReferenceId = (type) => {
  - if (type ...)
      - return type.FullName;
  - else
      - return OpenApiOptions.CreateDefaultSchemaReferenceId(type);
}
  • CreateSchemaReferenceId is non-optional in the API but has a default value
namespace Microsoft.AspNetCore.OpenApi;

public sealed class OpenApiOptions
{
+    public static string? CreateDefaultSchemaReferenceId(JsonTypeInfo jsonTypeInfo);
+    public Func<JsonTypeInfo, string?> CreateSchemaReferenceId { get; set; }
}

API Approved!

@amcasey amcasey added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jul 11, 2024
@captainsafia captainsafia added this to the 9.0-preview7 milestone Jul 11, 2024
@bkoelman
Copy link

bkoelman commented Jul 12, 2024

This looks great. Though I find returning null to inline the schema kinda odd. What's the use case for inlining?

Would this also enable to specify the name of the discriminator property? I'm asking because we have a use case where it must be one of the model properties (the JSON:API spec we implement requires this). Example:

"dataInResponse": {
  "required": [
    "type"
  ],
  "type": "object",
  "properties": {
    "type": {
      "minLength": 1,
      "type": "string"
    }
  },
  "additionalProperties": false,
  "discriminator": {
    "propertyName": "type",
    "mapping": {
      "people": "#/components/schemas/personDataInResponse",
      "tags": "#/components/schemas/tagDataInResponse",
      "todoItems": "#/components/schemas/todoItemDataInResponse"
    }
  },
  "x-abstract": true
}

In the example above, we must ensure that:

  • discriminator.propertyName is type (the model class doesn't actually contain a Type property, a concern for later)
  • mapping values must be people/tags/todoItems (they do not match the class names)
  • I'm assuming this issue enables to set personDataInResponse/tagDataInResponse/todoItemDataInResponse

For completeness, one of the derived types looks like this (I've left out extra properties in both snippets for brevity):

"personDataInResponse": {
  "allOf": [
    {
      "$ref": "#/components/schemas/dataInResponse"
    },
    {
      "required": [
        "id"
      ],
      "type": "object",
      "properties": {
        "id": {
          "minLength": 1,
          "type": "string"
        }
      },
      "additionalProperties": false
    }
  ],
  "additionalProperties": false
},

@captainsafia
Copy link
Member Author

This looks great. Though I find returning null to inline the schema kinda odd. What's the use case for inlining?

It can be helpful if the user wishes to override the framework defaults around inlined schemas (e.g. we currently capture dictionary and array types by reference).

Would this also enable to specify the name of the discriminator property? I'm asking because we have a use case where it must be one of the model properties (the JSON:API spec we implement requires this).

The values ofdiscriminator.propertyName and the keys in the discriminator.mapping dictionary are controlled by the polymorphism attributes in System.Text.Json. You can take a look at the tests to see how these mappings line up.

If you're system builds on top of System.Text.Json's polymorphism attributes, then you should be good to go here.

I'm assuming this issue enables to set personDataInResponse/tagDataInResponse/todoItemDataInResponse

Yes, the API proposed here would impact the references that are used in the response in particular. For our implementation, we're opinionated about the unique reference IDs for polymorphic schemas being BaseTypeReferenceId + SubTypeReferenceId so the reference IDs manifest like ShapeTriangle or ShapeSquare.

@bkoelman
Copy link

Thanks for the clarifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented 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

Successfully merging a pull request may close this issue.

4 participants