Giter Site home page Giter Site logo

Using ApiExplorerSettingsAttribute together with ApiVersionAttribute produces unexpected number of ApiVersionDescriptions about aspnet-api-versioning HOT 5 CLOSED

folbo avatar folbo commented on June 5, 2024
Using ApiExplorerSettingsAttribute together with ApiVersionAttribute produces unexpected number of ApiVersionDescriptions

from aspnet-api-versioning.

Comments (5)

commonsensesoftware avatar commonsensesoftware commented on June 5, 2024 1

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;
    }
}

from aspnet-api-versioning.

commonsensesoftware avatar commonsensesoftware commented on June 5, 2024 1

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.

from aspnet-api-versioning.

commonsensesoftware avatar commonsensesoftware commented on June 5, 2024

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.

from aspnet-api-versioning.

folbo avatar folbo commented on June 5, 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)

from aspnet-api-versioning.

folbo avatar folbo commented on June 5, 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.

from aspnet-api-versioning.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.