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

OpenAPI: Support including related resources #1448

Merged
merged 13 commits into from
Feb 2, 2024
Merged

Conversation

bkoelman
Copy link
Member

@bkoelman bkoelman commented Feb 1, 2024

This PR adds OAS support for the top-level "included" array, which is used in JSON:API responses. You get them when using the include query string parameter.

As I'm getting more familiar with the OpenAPI code originally written by @maurei, I found opportunities for improving consistency, simplifying things, applying corrections, and fixing some bugs. These are part of the PR as well, see the individual commits for details.

An example is often easier to understand, so let's see what's changed from a usage perspective.

snippet: allOf inheritance in swagger.json
"components": {
  "schemas": {
    "dataInResponse": {
      "required": [
        "id",
        "type"
      ],
      "type": "object",
      "properties": {
        "type": {
          "minLength": 1,
          "type": "string"
        },
        "id": {
          "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
    },
    "personDataInResponse": {
      "allOf": [
        {
          "$ref": "#/components/schemas/dataInResponse"
        },
        {
          "required": [
            "links"
          ],
          "type": "object",
          "properties": {
            "attributes": {
              "allOf": [
                {
                  "$ref": "#/components/schemas/personAttributesInResponse"
                }
              ]
            },
            "relationships": {
              "allOf": [
                {
                  "$ref": "#/components/schemas/personRelationshipsInResponse"
                }
              ]
            },
            "links": {
              "allOf": [
                {
                  "$ref": "#/components/schemas/linksInResourceData"
                }
              ]
            },
            "meta": {
              "type": "object",
              "additionalProperties": {
                "type": "object",
                "nullable": true
              }
            }
          },
          "additionalProperties": false
        }
      ],
      "additionalProperties": false
    },
    "tagDataInResponse": {
      "allOf": [
        {
          "$ref": "#/components/schemas/dataInResponse"
        },
        {
          "required": [
            "links"
          ],
          "type": "object",
          "properties": {
            "attributes": {
              "allOf": [
                {
                  "$ref": "#/components/schemas/tagAttributesInResponse"
                }
              ]
            },
            "relationships": {
              "allOf": [
                {
                  "$ref": "#/components/schemas/tagRelationshipsInResponse"
                }
              ]
            },
            "links": {
              "allOf": [
                {
                  "$ref": "#/components/schemas/linksInResourceData"
                }
              ]
            },
            "meta": {
              "type": "object",
              "additionalProperties": {
                "type": "object",
                "nullable": true
              }
            }
          },
          "additionalProperties": false
        }
      ],
      "additionalProperties": false
    },
    "todoItemDataInResponse": {
      "allOf": [
        {
          "$ref": "#/components/schemas/dataInResponse"
        },
        {
          "required": [
            "links"
          ],
          "type": "object",
          "properties": {
            "attributes": {
              "allOf": [
                {
                  "$ref": "#/components/schemas/todoItemAttributesInResponse"
                }
              ]
            },
            "relationships": {
              "allOf": [
                {
                  "$ref": "#/components/schemas/todoItemRelationshipsInResponse"
                }
              ]
            },
            "links": {
              "allOf": [
                {
                  "$ref": "#/components/schemas/linksInResourceData"
                }
              ]
            },
            "meta": {
              "type": "object",
              "additionalProperties": {
                "type": "object",
                "nullable": true
              }
            }
          },
          "additionalProperties": false
        }
      ],
      "additionalProperties": false
    },
    "personCollectionResponseDocument": {
      "required": [
        "data",
        "links"
      ],
      "type": "object",
      "properties": {
        "links": {
          "allOf": [
            {
              "$ref": "#/components/schemas/linksInResourceCollectionDocument"
            }
          ]
        },
        "data": {
          "type": "array",
          "items": {
            "$ref": "#/components/schemas/personDataInResponse"
          }
        },
        "included": {
          "type": "array",
          "items": {
            "$ref": "#/components/schemas/dataInResponse"
          }
        },
        "meta": {
          "type": "object",
          "additionalProperties": {
            "type": "object",
            "nullable": true
          }
        }
      },
      "additionalProperties": false
    }
  }
}

In the snippet above, observe that:

  • dataInResponse (newly added) defines the base schema for included resources (which consist of type and id). It uses the type element from JSON:API to list the available derived schemas in its discriminator mapping.
  • personDataInResponse, tagDataInResponse and todoItemDataInResponse use allOf to refer to the base schema, followed by defining their additional properties.
  • personCollectionResponseDocument (I've left out the others for brevity) has an included property, which is an array of type dataInResponse.

When using NSwag, the discriminator is used to generate derived classes, so you can write type checks on the included resources. For example:

var getResponse = await apiClient.GetPersonCollectionAsync(new Dictionary<string, string?>
{
    ["include"] = "assignedTodoItems"
});

foreach (var person in getResponse.Data)
{
    Console.WriteLine($"Found person {person.Id}: {person.Attributes.DisplayName}");

    foreach (var todoItemIdentifier in person.Relationships.AssignedTodoItems.Data)
    {
        var relatedTodoItem = getResponse.Included
            .OfType<TodoItemDataInResponse>()
            .Single(include => include.Id == todoItemIdentifier.Id);

        Console.WriteLine(
            $"  with assigned todo-item {relatedTodoItem.Id}: {relatedTodoItem.Attributes.Description}");
    }
}

As shown in the picture below, the compiler (Resharper in this case) is now aware of the base and derived classes:

image

Getting this feature to work was quite a journey! I started with implementing oneOf as documented by Swashbuckle, but it turned out that NSwag doesn't support that. I then tried switching to allOf, which didn't work either. I couldn't figure out how to make NSwag generate the derived types properly; it would only generate the first derived type. So I built an API using NSwag to see what it needed to look like. Turned out that Swashbuckle using allOf is fundamentally broken. Because the project hasn't been updated in a year (pretty concerning), I didn't bother creating a PR with my fix. So instead I included a patched copy of SchemaGenerator in our codebase.

When I tried mapping the JSON:API type field as the discriminator property, I found that NSwag would not expose the Type property anymore. This is great because that's just a JSON:API protocol detail that consumers should not need to worry about. So I thought it would be nice to have the same for the returned array from relationship endpoints. It didn't work out, because a bug in NSwag (actually, NSchema) resulted in generating the derived types as abstract because they didn't have any properties of their own. To work around that, I would need to turn off x-abstract in the base schema we produce.

I'm also experimenting with Kiota, a fairly new OpenAPI client generator built by Microsoft. It functions (after working around microsoft/kiota#3800), but because it always generates all properties as nullable (and does not hide the Type property like NSwag does, and does not populate it when sending a derived type), this made things worse: instead of passing an enum, it would now require the consumer to pass a string like "todoItems" for the resource type. The same problem would occur when trying to use inheritance for the POST/PATCH resource request body, so I didn't even try. I also found that Kiota generates multiple identical derived classes (I haven't investigated further yet), so I didn't like the idea of making it more complicated than needed. Therefore I reverted all that. And that's why this PR only implements inheritance for the included array.

Closes #1059, fixes #1233.

QUALITY CHECKLIST

@bkoelman bkoelman changed the base branch from master to openapi February 1, 2024 23:56
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 95 lines in your changes are missing coverage. Please review.

Comparison is base (37f41fe) 91.08% compared to head (97bd1bc) 90.68%.

Files Patch % Lines
src/JsonApiDotNetCore.OpenApi/SchemaGenerator.cs 63.67% 56 Missing and 25 partials ⚠️
...rc/JsonApiDotNetCore.OpenApi.Client/ApiResponse.cs 0.00% 6 Missing ⚠️
...c/JsonApiDotNetCore.OpenApi/ConfigureMvcOptions.cs 86.66% 1 Missing and 1 partial ⚠️
...piDotNetCore.OpenApi/ConfigureSwaggerGenOptions.cs 95.00% 1 Missing and 1 partial ⚠️
...re.OpenApi/JsonApiRequestFormatMetadataProvider.cs 50.00% 0 Missing and 1 partial ⚠️
...pi/SwaggerComponents/ResourceFieldSchemaBuilder.cs 97.50% 0 Missing and 1 partial ⚠️
...Core.OpenApi/SwaggerComponents/ResourceTypeInfo.cs 87.50% 1 Missing ⚠️
...DotNetCore/Diagnostics/CodeTimingSessionManager.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           openapi    #1448      +/-   ##
===========================================
- Coverage    91.08%   90.68%   -0.41%     
===========================================
  Files          384      390       +6     
  Lines        12358    12717     +359     
  Branches      1989     2056      +67     
===========================================
+ Hits         11256    11532     +276     
- Misses         710      771      +61     
- Partials       392      414      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bkoelman bkoelman force-pushed the openapi-includes branch 4 times, most recently from b2ee92b to 24123dd Compare February 2, 2024 02:11
@bkoelman bkoelman marked this pull request as ready for review February 2, 2024 02:52
@bkoelman bkoelman merged commit 6c9aff8 into openapi Feb 2, 2024
16 checks passed
@bkoelman bkoelman deleted the openapi-includes branch February 2, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Generating multiple swagger documents is not supported Support for top-level included object
1 participant