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 not working with version interleaving within single controller #1025

Closed
1 task done
AnnejanBarelds opened this issue Oct 3, 2023 · 3 comments · Fixed by #1027
Closed
1 task done

OpenAPI not working with version interleaving within single controller #1025

AnnejanBarelds opened this issue Oct 3, 2023 · 3 comments · Fixed by #1027
Assignees
Labels

Comments

@AnnejanBarelds
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Hi,

Starting note: this issue describes 2 bugs (or at least unexpected behaviors) because I can imagine that they are related. If I need to split them up into separate issues, just let me know.

It appears that generating OpenAPI specifications breaks when doing version interleaving within a single controller, where not all actions are explicitly mapped. So a controller decorated with multiple ApiVersion attributes, and two actions on the same path where only one of these actions is decorated with MapToApiVersion, will break the OpenAPI spec for the explicitly mapped version. Actually calling the actions works as expected; it's just the OpenAPI generation that breaks.

Also, is it expected that the OpenAPI specs do no respect the ApiVersionSelector? If I use LowestImplementedApiVersionSelector, I can omit the api-version (which is expected), but the generated OpenAPI spec indicates that api-version is required.

Expected Behavior

With regards to the breaking API spec: I would expect that interleaving versions within a single controller, when done according to the wiki, does not break OpenAPI. A valid outcome would be that the wiki needs to be updated to reflect that implicit version interleaving is not compatible with OpenAPI.

With regards to OpenAPI and the ApiVersionSelector: I would expect that OpenAPI does not require a parameter that is not, in fact, required.

Steps To Reproduce

With regards to the breaking API spec: adding the controller from the samples (see below) to the OpenApiExample, breaks the OpenAPI spec for version 3.0.

[ApiVersion( 2.0 )]
[ApiVersion( 3.0 )]
[ApiController]
[Route( "api/helloworld" )]
public class HelloWorld2Controller : ControllerBase
{
    [HttpGet]
    public string Get() => "Hello world v2.0!";

    [HttpGet, MapToApiVersion( 3.0 )]
    public string GetV3() => "Hello world v3.0!";
}

With regards to OpenAPI and the ApiVersionSelector: given a project where the DefaultApiVersion is not touched (so is 1.0); AssumeDefaultVersionWhenUnspecified = true; and the ApiVersionSelector is set to LowestImplementedApiVersionSelector , what I observe is

  1. A controller that implements 1.0 and 2.0, and has two actions on the same path, where one of them is mapped to 1.0 and the other to 2.0, the OpenAPI spec correctly does not mark the api-version parameter as required for 1.0.
  2. However, when the same controller implements 2.0 and 3.0, and the two actions are mapped accordingly, api-version is required for both versions in the OpenAPI spec - even though an actual call to the path without a version specified correctly maps to 2.0.

Exceptions (if any)

Exception for the broken OpenAPI spec:

Swashbuckle.AspNetCore.SwaggerGen.SwaggerGeneratorException: Conflicting method/path combination "GET api/helloworld" for actions - ApiVersioning.Examples.HelloWorld2Controller.Get (OpenApiExample),ApiVersioning.Examples.HelloWorld2Controller.GetV3 (OpenApiExample). Actions require a unique method/path combination for Swagger/OpenAPI 3.0. Use ConflictingActionsResolver as a workaround
at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GenerateOperations(IEnumerable1 apiDescriptions, SchemaRepository schemaRepository) at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GeneratePaths(IEnumerable1 apiDescriptions, SchemaRepository schemaRepository)
at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GetSwaggerDocumentWithoutFilters(String documentName, String host, String basePath)
at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GetSwaggerAsync(String documentName, String host, String basePath)
at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

.NET Version

7.0.400

Anything else?

No response

@commonsensesoftware
Copy link
Collaborator

Indeed this is an issue and I've been able to reproduce the conditions. Interestingly, it appears that this behavior has always existed or, at least, certainly for a long time, but has not been reported. Version interleaving with OpenAPI is meant to be supported and it partially is. There is clearly a missing edge case where an overlapping API path will produce a duplicate entry when an action is implicitly matched and is not discarded if an explicitly matched action supersedes it. This is a 🐞 and I've come up with a remedy. As you noted, this behavior does not affect routing, which uses the same implicit matching rules, but is implemented completely differently.

The IApiVersionSelector currently plays no role in documentation. The primary reason for this is because there will be no current HTTP request, which may be necessary for the selector to do its job. It is an open question as to what the expectation of the selector is when there is no useful information in the HTTP request. By contract, null cannot be returned. Providing an empty HTTP request might be sufficient for the purposes of documentation. Some additional enhancements are being discussed in #1009 which might affect the future direction on taking advantage of IApiVersionSelector this way.

The default behavior for the documented api-version parameter is that the first occurrence is required and all other occurrences are optional. Given that there can be more than one location that an API version can appear, that is a sensible default. Although the IApiVersionSelector is not used, if AssumeDefaultVersionWhenUnspecified = true and corresponding ApiVersion equals ApiVersioningOptions.DefaultApiVersion, then the first occurrence will also be optional. This is obviously problematic when not all of the APIs use the same default API version and the IApiVersionSelector is also not used. The only exception is when versioning by URL segment. In this case, the API version parameter is always required because it is part of the path. In addition, any other defined API version location no longer makes sense as you'd still have to specify the path parameter. In retrospect, I suppose the same thing is true when combining other API version locations, which really need to be mapped as One Of, but OpenAPI does not support that concept such as one of: header(api-version) | query(api-version). There is one other slight variant for version-neutral APIs. A version-neutral API can have a parameter, but it isn't required. The API Explorer options let you configure whether this is documented. By default, the API version parameters for version-neutral APIs are omitted.

The immediate workaround is to explicitly use [MapToApiVersion] or use a convention if you don't/can't use attributes. It's inconvenient, but it will unblock you while I work on the fix. I'm experimenting on what it would look like and how things would work if the IApiVersionSelector is used for API Exploration.

@AnnejanBarelds
Copy link
Author

Thanks a lot for the quick fix! Haven't been able to take it for a spin yet (been busy visiting a conference this week), but I'm sure I will like it :). Thanks again, and keep up the good work!

@commonsensesoftware
Copy link
Collaborator

Explicit mappings will now supersede implicit mappings so that problem is fixed.

ApiExplorerOptions now has an IApiVersionSelector ApiVersionSelector { get; set; } property. The default value will use DefaultApiVersion, which was the same as the original behavior. However, in all practical use cases, it will derive the value from ApiVersioningOptions.ApiVersionSelector, which will likely have the same behavior. This gives you the option to configure it once for both. Crucially, you can still explicitly set ApiExplorerOptions.ApiVersionSelector. This is important so that you can use a different IApiVersionSelector for documentation than you might in an actual request pipeline (e.g. an escape hatch). The supplied HTTP request is always empty so it's not useful for documentation.

Enjoy the conference. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants