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

Migration from Microsoft.AspNetCore.Mvc.Versioning to Asp.Versioning.Mvc causes versioning to no longer work #917

Closed
1 task done
ustarrenato opened this issue Nov 17, 2022 · 3 comments
Assignees
Labels

Comments

@ustarrenato
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Hi,

We are currently in a process of migrating our codebase to .NET 6, and we noticed we no longer could get versioning to work.

I have successfully recreated the issue in a new .NET 6 ASP.NET Core project, and was able to confirm that using Microsoft.AspNetCore.Mvc.Versioning (5.0.0) and AddVersionedApiExplorer, IApiVersionDescriptionProvider works as expected.

This means that IApiVersionDescriptionProvider is able to pick up two versions specified in a versioned controller, and this can then be passed to Swagger UI.

Using Asp.Versioning.Mvc.ApiExplorer (6.2.1, 6.2.0, 6.0.0) and AddApiExplorer causes IApiVersionDescriptionProvider to only show default API version (defaulting to 1.0 when not specified) and does not register any versioning applied to the versioned controller.

I have included link to a solution containing two projects, one which uses the latest Microsoft.AspNetCore.Mvc.Versioning at the time of writing (5.0.0), and one that uses the latest Asp.Versioning.Mvc (6.2.1).

I would kindly ask to confirm if this is indeed the BUG, or am I simply missing something in the configuration.

I should also note InfoController is simply added to test that endpoints are mapped correctly, and is not used as a part of versioning (And the issue persists even if the controller is removed).

image
image

Expected Behavior

After successful build, both projects should be able to display swagger UI with two versions, 2.0 and 3.0.

Steps To Reproduce

Link to repo:

https://github.com/ustarrenato/WebAppWithVersioning

Setting the breakpointing in Program.cs line 70 in VS shows that apiVersionDescriptionProvider?.ApiVersionDescriptions has only one version in WebAppWithVersioning (1.0) while WebAppWithVersioningMicrosoft5 has two (2.0, 3.0).

Exceptions (if any)

No response

.NET Version

7.0.100

Anything else?

Microsoft.NETCore.App 6.0.11
Visual Studio 2022 Version 17.4.0
Microsoft .NET Framework 4.8.09037
ASP.NET and Web Tools 17.4.326.54890
Swagger 6.4.0

@ustarrenato ustarrenato changed the title Migration from Microsoft.AspNetCore.Mvc.Versioning to Asp.Versioning.Mvc causes versining to no longer work Migration from Microsoft.AspNetCore.Mvc.Versioning to Asp.Versioning.Mvc causes versioning to no longer work Nov 17, 2022
@commonsensesoftware
Copy link
Collaborator

Thanks for the repro. It's very helpful.

Interestingly, there is a 🐞 , but not quite for the reason you may think. I'm surprised this hasn't been hit before. I'd have to dig up the issue, but there was a much older problem where unversioned APIs that could show up in OpenAPI were completely discarded. While that's probably what most people wanted (as you expect here), it's wrong and it was a problem for people that wanted it. Unfortunately, that fix didn't make it into any of the 5.x builds. 😞 The actual bug that you are observing is that InfoController is [appropriately] unversioned, but it is being added for exploration multiple times (per API version). If you only had one API version, this wouldn't even manifest. ☹️

There are a number of other problems however:

  1. You now need to call .AddMvc() on the API Versioning setup
    a. This part of the IApiVersioningBuilder setup
    b. This does not bring full MVC, just MVC Core as it always has
    c. This change was necessary for those that use Minimal APIs without any form of MVC
    d. Arguably, .AddApiExplorer() should call this automatically; I'll investigate adding that
  2. The template [Route("v{version:ApiVersion}/Names")] should be [Route("v{version:apiVersion}/Names")] because the name of the route constraint is apiVersion. The processing seems to be case-insensitive, but this might cause issues elsewhere (though I didn't find any).
  3. Did you mean to use [Route("All")] or [HttpGet("All")]?
    a. The setup seemed strange to me and appears to have the same net effect
  4. The main culprit is that InfoController is excluded from API versioning by convention, but it's not excluded from the API Explorer. You need to add [ApiExplorerSettings(IgnoreApi = true)].
    a. Due to the aforementioned bug, in previous versions this would have been automatically excluded.
  5. You don't need to set ApiVersionParameterSource as it will derive from the configured ApiVersionReader. It's not wrong, but it's unnecessary.
  6. Consider using foreach (var desc in app.DescribeApiVersions()) instead of foreach (var desc in apiVersionDescriptionProvider?.ApiVersionDescriptions). It's not wrong, but DescribeApiVersions() is a new shortcut extension method that will work for both traditional MVC controllers (e.g. the same thing) as well as Minimal APIs.

The setup should look like:

builder.Services.AddApiVersioning(options =>
{
    options.ReportApiVersions = true;
    options.ApiVersionReader = new UrlSegmentApiVersionReader();
})
.AddMvc() // ← new; brings in MVC Core support
.AddApiExplorer(options =>
{
    options.GroupNameFormat = "'v'VVV";
    options.SubstituteApiVersionInUrl = true;
});

@ustarrenato
Copy link
Author

Thank you for the fast response, i was able to get the demo repo working after applying the steps you provided.

Regarding the individual points:

  1. This is indeed something I did not know, and is very helpful to know moving forward as a lot of online resources seems to miss this step. It might be a good idea to stress this point in the upcoming documentation, as it is easily missed.
  2. Good to know, it seems to be a typo on my behalf as the original repo which we are migrating uses proper casing
  3. I have indeed,
  4. Thank you for the clarification
  5. I see, so its part of the builder pattern set up by IApiVersioningBuilder.
  6. Noted, also something I did not know, and will use moving forward.

Thank you for the example provided. I will leave a link to a new working demo repo in case somebody stumbles on this issue so they can have a baseline to start and compare differences to the 5.0.0 version.

Link to working demo repo: https://github.com/ustarrenato/WebAppWithVersioningWorkingSample

@commonsensesoftware
Copy link
Collaborator

FYI, 6.3.x and 7.0.0-* will call the necessary AddMvc to help avoid this in the future.

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

No branches or pull requests

2 participants