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: Hide inaccessible links #1518

Merged
merged 6 commits into from
Mar 27, 2024
Merged

OpenAPI: Hide inaccessible links #1518

merged 6 commits into from
Mar 27, 2024

Conversation

bkoelman
Copy link
Member

@bkoelman bkoelman commented Mar 27, 2024

This PR updates OpenAPI to take into account whether JSON:API links are available.

My first take on implementing this was to declare nearly all links as required in the component schemas. Then at runtime remove the links we know can't be returned, which can be determined from the cascading links configuration.

However, because links can be configured per resource type, this requires expansion of the link schema types. For example, linksInResourceData becomes linksInTodoItemData, linksInPersonData, linksInTagData, etc. This introduces many new schemas for questionable gain, as using OpenAPI typically makes users care less about links. But it gets worse. Relationship links can be overruled per relationship within a resource type. This means we need to expand link schema types for them as well. But the relationship links are defined on the relationship left-type, whereas the currently produced schemas for relationships reflect their right-type. For example, todoItems contains relationships owner and assignee, both of type people. Let's assume both relationships are required. In that case, a single schema toOnePersonInResponse is produced for both relationships' right-side types, because they have the same data structure. Now taking expansion for relationship links into account, we'd need to generate separate schemas: ownerToOnePersonInResponse and assigneeToOnePersonInResponse. As a result, consumers can't write the following code anymore, because different schemas would be produced:

var todoItemsResponse = await _apiClient.GetTodoItemCollectionAsync(null, null);
var firstTodoItem = todoItemsResponse.Result.Data.First();

PrintPersonRelationship(firstTodoItem.Relationships.Owner);
PrintPersonRelationship(firstTodoItem.Relationships.Assignee);

private void PrintPersonRelationship(ToOnePersonInResponse person)
{
    // ....
}

For the reasons mentioned above, I chose not to implement schema expansion at all. The next best thing is to determine which links can be eliminated by taking a union of the configured link types. So if all links are turned off, but todoItems turns on the top-level self link, then all resource types get only the top-level self link. Likewise, if the owner relationship turns on the related relationship-level link, while the assignee relationship turns on the self link, then all relationship schemas get both links. If no link overrules are used and links are turned off in options, then no links appear in the component schemas.

As a result, all links must be declared as non-required in the component schemas, because now the schemas may contain links that won't actually be returned at runtime (and we haven't eliminated them from the schema).

Closes #1062.

QUALITY CHECKLIST

@bkoelman bkoelman changed the base branch from master to openapi March 27, 2024 06:00
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 97.77778% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 90.85%. Comparing base (66be210) to head (7722b87).

Files Patch % Lines
...waggerComponents/LinksVisibilitySchemaGenerator.cs 96.87% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           openapi    #1518      +/-   ##
===========================================
+ Coverage    90.77%   90.85%   +0.07%     
===========================================
  Files          394      396       +2     
  Lines        12993    13115     +122     
  Branches      2058     2078      +20     
===========================================
+ Hits         11795    11916     +121     
+ Misses         782      781       -1     
- Partials       416      418       +2     

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

@bkoelman bkoelman marked this pull request as ready for review March 27, 2024 07:31
@bkoelman bkoelman merged commit 1bdee8b into openapi Mar 27, 2024
16 checks passed
@bkoelman bkoelman deleted the openapi-links branch March 27, 2024 07:37
@xontab
Copy link

xontab commented May 13, 2024

@bkoelman Will this feature be released soon?

@bkoelman
Copy link
Member Author

This is available for use from the GitHub NuGet feed, see https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/master/README.md#trying-out-the-latest-build. The overall progress of adding OpenAPI support is tracked at #1046. It'll take a while to complete that, after which it'll be published to NuGet. To speed that up, you can help if you want by working on open issues.

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.

Generalized handling of link objects
2 participants