Skip to content

Using ApiExplorerSettingsAttribute together with ApiVersionAttribute produces unexpected number of ApiVersionDescriptions #1066

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
1 task done
folbo opened this issue Jan 11, 2024 · 5 comments · Fixed by #1067
Assignees

Comments

@folbo
Copy link

folbo commented Jan 11, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

In my application I'm grouping some controllers into ApiExplorer groups to produce multiple swagger files. For each group I'm trying to implement independent versioning using Asp.Versioning.

When I mix [ApiExplorer(GroupName = "WeatherForecastGroupName")] with [ApiVersion(1)] attributes my IApiVersionDescriptionProvider creates two ApiVersionDescriptions, both with version 1. Provider's output attached below:

provider.ApiVersionDescriptions.Select(x => x.GroupName).ToArray()
{string[2]}
    [0]: "1.0"
    [1]: "WeatherForecastGroupName_1.0"

Environment: AspNetCore 6 application with controllers.

My code used to reproduce the issue:

[ApiController]
[Route("api/v{version:apiVersion}/[controller]")]
[ApiVersion(1)]
[ApiExplorerSettings(GroupName = "WeatherForecastGroupName")]
public class WeatherForecastController : ControllerBase
{
    private static readonly string[] Summaries = new[]
    {
        "Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching"
    };

    [HttpGet(Name = "GetWeatherForecast")]
    public IEnumerable<WeatherForecast> Get()
    {
        return Enumerable.Range(1, 5).Select(index => new WeatherForecast
        {
            Date = DateTime.Now.AddDays(index),
            TemperatureC = Random.Shared.Next(-20, 55),
            Summary = Summaries[Random.Shared.Next(Summaries.Length)]
        })
        .ToArray();
    }
}

builder.Services.AddApiVersioning(
        options =>
        {
            options.ApiVersionReader = new UrlSegmentApiVersionReader();
        })
    .AddMvc()
    .AddApiExplorer(
        options =>
        {
            options.SubstituteApiVersionInUrl = true;
            options.FormatGroupName = (name, version) => $"{name}_{version}";
        })

Expected Behavior

I'd expect that Asp.Versioning.ApiExplorer would use options.FormatGroupName as the only source of ApiVersionDescriptions if provided.

Steps To Reproduce

  1. Define aspnetcore 6 controller with sample action and with [AspVersion(1)] and [ApiExplorerSettings(GroupName = "name")] attributes
  2. Configure package with FormatGroupName
  3. Notice contents of IApiVersionDescriptionProvider

Exceptions (if any)

No response

.NET Version

7.0.403

Anything else?

No response

@commonsensesoftware
Copy link
Collaborator

First, a point of clarity. The issue indicates you are targeting .NET 7, but the text says ASP.NET Core 6, which would be .NET 6. .NET 8 is the current version. Which version are you actually using? I need to make sure I know which code I'm looking at.

Without additional context, it would appear that you have at least one other controller that that is 1.0 and it does not have [ApiExplorerSettings(GroupName = "WeatherForecastGroupName")]. Since you have also specified options.FormatGroupName = (name, version) => $"{name}_{version}"; and that controller (or endpoint) does not have a group name, it will yield ApiVersion.ToString, which is clearly 1.0. This is the expected behavior. You don't want the other controller included in documentation, then [ApiExplorerSettings(IgnoreApi = true)] will do the trick.

Neither the API Explorer (a la ApiDescription.GroupName) nor the Swagger UI support multi-level groups. It is pseudo simulated by combining the original group name and API version together if you so choose. If you don't specify a group, then just the version is used. If you specify a group name, but don't specify how to format the two together, then only the group name is used. This is an all or nothing behavior for the application.

I suspect you must have more controllers to observe this behavior. If not, then the world's simplest repo would go a lot way to recreating the conditions you have. I attempted to simulate the behavior from the information you have shared and the issue did not reproduce.

@folbo
Copy link
Author

folbo commented Jan 12, 2024

Thank you for your reply. No, I don't have more controllers than one. I've created the repository which reproduces the problem. https://github.com/folbo/ApiVersioningBehaviorRepro/blob/master/ReproVersioningBug/Program.cs#L64C8-L64C8
Notice the assert in Program.cs.

I noticed that when I call the IApiVersionDescriptionProvider from the UseSwaggerUI callback it contains a correct list of versions. However, when I call it from within IConfigureOptions<SwaggerGenOptions> it contains a wrong number of version descriptions. Maybe it has something to do with order of calls?

EDIT: You are correct, executing dotnet --version in my newly created .net6 project directory returns my most recent installation of dotnet CLI - 7.0.403 and this is the SDK version I used to create this reproductible project I linked. Normally I work with .net 6 on SDK 6.0.413 (I usually use global.json to specify the sdk version)

@commonsensesoftware
Copy link
Collaborator

Thanks for the repro. It has been useful. It turns out that I had a repro, but I kept missing the scenario. This is a bug.

The reason this happens is that collation needs to occur for Endpoints that can be defined by controller as well as Minimal APIs. It seems odd that someone would mix and match these two, but - technically - they can. The Endpoints for Minimal APIs are defined in a completely different way (by ASP.NET Core). In fact, they are not part of DI at all! They are defined dynamically after the container is created. This is the reason that the new[er] app.DescribeApiVersions() extension method exists (as shown in the examples) instead of just going to straight to the IApiVersionDescriptionProvider as has been the case in the past. The IApiVersionDescriptionProvider is still involved, but the process has to account for Endpoints registered outside of DI.

All APIs ultimately register an Endpoint as some point; even, controllers. The way that you define a group name for a Minimal API is different than a controller. They don't use the same metadata or, even, the same interfaces. This was to enable ASP.NET Core to separate the backing libraries. If we ever go back to a package references instead of framework references or some hybrid thereof, it may matter. API Versioning follows this approach as well. Asp.Versioning.Http doesn't use any part of MVC Core. This leads to the bug.

If, and only if, you define a group name, the Minimal API logic still runs. It doesn't know how to distinguish a controller from any other 'ol Endpoint (e.g. a Minimal API). On the first pass of configuring Swashbuckle, dynamic endpoints have not been evaluated. If you had Minimal APIs or mixed and matched, you'd actually get 0 without using DescribeApiVersions(). A second pass occurs while configuring the SwaggerOptions. This happens because just before the application runs, the app is an EndpointDataSource that signals a change. The change simply informs the IApiVersionDescriptionProvider that it needs to re-evalute all endpoints. This time the Minimal API logic sees an Endpoint, but it doesn't have the expected metadata because the way it's defined is unknown to a Minimal API. This results in the group name being null. The consequence that one collation method sees the one endpoint without a group and the other collation method (for controller) sees the endpoint the group name. Even though duplicates are accounted for, this will produce 2 entries - one with a group and one without.

This was easily missed because I don't believe there are any examples or tests that setup this specific combination. I also noticed that even though there are multiple versions reported, you still see the expected results in the UI.

This will certainly be fixed. I already have some thoughts on how to do it. I have a couple of follow up questions:

  1. Is there a reason to stay on .NET 6 vs .NET 8 since they are both LTS?
  2. If the UI shows things correctly, is this still a must fix issue (for .NET 6)
  3. If the context is something other than the UI, a distinct list of versions can still be retrieved from IApiVersionDescriptionProvider

The fix is simple, but supporting multiple TFMs is non-trivial and time consuming. Historically, I don't service older versions except for the rarest of scenarios (ex: security). Now that we're in a LTS/STS model for .NET, my policy (though not officially stated) will be primarily servicing the LTS releases. It's not advertised explicitly because there's always a chance I'll make the fix. As a virtual one man show, it's quite a bit of extra work. I can backport the fix to .NET 6, but I want to confirm that it's really necessary. It even seems that while there certainly is an issue, you might be able to live with it on .NET 6 because there are no visible side effects (in the UI). There might be another use case that is blocking you that I'm not privy to. The ideal scenario would be to only patch the .NET 8 release.

If it is a blocking issue, there are some other immediate workarounds that you can do right now to unblock yourself. If you remove the collation for Minimal APIs, the issue is resolved. If you're stuck on .NET 6, this might be the compromise so you get the results you expect, you don't have to wait on me, and I don't have to backport. The behavior will still be correct after the fix when you update to .NET 8+, but it won't be required anymore. The following will remove Minimal API version collation:

var services = builder.Services;

for (var i = 0; i < services.Count; i++)
{
    var service = services[i];

    if (service.ServiceType == typeof(IApiVersionMetadataCollationProvider)
        && service.ImplementationType == typeof(EndpointApiVersionMetadataCollationProvider))
    {
        services.RemoveAt(i);
        break;
    }
}

@folbo
Copy link
Author

folbo commented Jan 12, 2024

Thank you for this broad explanation. I'm glad that what I've found out might be useful for further fixes. I think I can live with workarounds. To be honest I might not need to do that in .net6 since I think what I try to achieve is not feasible. Also I plan to eventually migrate to .net 8.

In my usecase, which I didn't present in the example, I register a list of api definitions that contain api name, description, version - everything that is needed for Swashbuckle to generate a swagger document - in app startup code (DI registration phase). I wanted to make this configuration the only source of truth about what api do I host in which version.
I still don't understand Swashbuckle very well but as I'm getting more involved I realize that it's interface isn't very convenient for me:

  • 1st, for SwaggerGen I need to add swagger documents based on my configuration - I already need to match each of my pre-declared api definitions with what's been discovered in VersionedApiExplorer to get the group name since it's the group name that tells which of my endpoints are belonging to which swagger doc. And that's the question - how do I match items of both collection? This is how I discovered the bug 😛 First I verified that both collections contain the same number of items. And.. zonk! ApiExplorer has 2 times more items at this point... But still I'm missing a proper key on which I could 'join' my collection items with api explorer contents.
  • then, I need to configure SwaggerUI to expose generated docs on dedicated urls (called SwaggerEndpoints). I already figured out that the only key used to match SwaggerUI's SwaggerEndpoint with SwaggerGen's SwaggerDoc is... that's it! a group name. And here I see two options to configure it:
    • to iterate over VersionedApiExplorer (which works fine in SwaggerUI, as you just explained) - this is a common approach found in all documentations of Asp.Versioning.
    • or to iterate over my preconfigured api definitions - which I chose to be my preferred way (it's the only source of truth after all). The only thing I need to do here is to search the VersionApiExplorer for matching VersionedApiDefinition to get the group name. And here I hit the same problem as in the SwaggerGen step - on what key do I match my setup with api explorer?

If I only knew group names beforehand...

Is it possible to configure Asp.Versioning with explicit group names? If not then I doubt I could make it my way without changing the core of this library.

@commonsensesoftware
Copy link
Collaborator

You're supposed to be able to. I've noticed another bug. 😞 I over-optimized the support for grouping and I now realize that if you only use a group name the DefaultApiVersionDescriptionProvider does not do the right thing because it doesn't consider group names at all and causes them to be filtered out. I also noticed that even the GroupedApiVersionDescriptionProvider misses the case where there is only a group name. In hindsight, there probably is no real benefit to DefaultApiVersionDescriptionProvider vs GroupedApiVersionDescriptionProvider. The group name only scenario can only be determined at runtime and they both have to handle it. I think that I did it that way because I didn't want to break the signatures for DefaultApiVersionDescriptionProvider when grouping support was added.

There is an escape hatch here without a fix because you can replace the implementation. The fastest path to victory would be to subclass GroupApiVersionDescriptionProvider and override Describe:

protected virtual IReadOnlyList<ApiVersionDescription> Describe( IReadOnlyList<GroupedApiVersionMetadata> metadata )

Unfortunately, a lot of the critical bits that you'd want are internal, so you'll have to copy that part. It will be unchanged. The part that needs to change is here:

The logic should be:

var formattedGroupName = groupName;

if ( string.IsNullOrEmpty( formattedGroupName ) )
{
    formattedGroupName = formattedVersion;
}
else if ( formatGroupName is not null )
{
    formattedGroupName = formatGroupName( formattedGroupName, formattedVersion );
}

Then you can add (before AddApiVersioning) with your implementation:

builder.Services.AddSingleton<IApiVersionDescriptionProvider, MyApiVersionDescriptionProvider>();

You can also replace the service after calling AddApiVersioning

It's little bit of work on your part, but that should get you on your way.

commonsensesoftware added a commit that referenced this issue Jan 12, 2024
…processed by min api endpoint collators. Related #1066
commonsensesoftware added a commit that referenced this issue Jan 12, 2024
…plicit group names can only be determined at runtime. Related #1066
commonsensesoftware added a commit that referenced this issue Mar 20, 2024
…processed by min api endpoint collators. Related #1066
commonsensesoftware added a commit that referenced this issue Mar 20, 2024
…plicit group names can only be determined at runtime. Related #1066
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants