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

Upgrade to Microsoft.OpenApi v2 #3252

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

martincostello
Copy link
Collaborator

@martincostello martincostello commented Feb 5, 2025

Upgrade to Microsoft.OpenApi v2, which would unlock support for OpenAPI 3.1.

Contributes to #2349.


This currently targets preview 5 and compiles, but lots of tests are broken.

The migration was guided by the release notes for the first preview.

Some might be bugs in the upgrade, others might be bugs in Microsoft.OpenApi v2 itself. I'll need to investigate further, then I can open any upstream issues as neccessary.

The most worrying failure so far is this one:

System.MissingMethodException: Method not found: 'Void Microsoft.OpenApi.Models.OpenApiOperation.set_Tags(System.Collections.Generic.IList`1<Microsoft.OpenApi.Models.OpenApiTag>)'.
   at Microsoft.AspNetCore.OpenApi.OpenApiGenerator.GetOperation(String httpMethod, MethodInfo methodInfo, EndpointMetadataCollection metadata, RoutePattern pattern)
   at Microsoft.AspNetCore.OpenApi.OpenApiGenerator.GetOpenApiOperation(MethodInfo methodInfo, EndpointMetadataCollection metadata, RoutePattern pattern)
   at Microsoft.AspNetCore.Builder.OpenApiEndpointConventionBuilderExtensions.AddAndConfigureOperationForEndpoint(EndpointBuilder endpointBuilder, Func`2 configure)
   at Microsoft.AspNetCore.Builder.OpenApiEndpointConventionBuilderExtensions.<>c__0`1.<WithOpenApi>b__0_0(EndpointBuilder builder)
   at Microsoft.AspNetCore.Routing.RouteEndpointDataSource.CreateRouteEndpointBuilder(RouteEntry entry, RoutePattern groupPrefix, IReadOnlyList`1 groupConventions, IReadOnlyList`1 groupFinallyConventions)
   at Microsoft.AspNetCore.Routing.RouteEndpointDataSource.GetGroupedEndpoints(RouteGroupContext context)
   at Microsoft.AspNetCore.Routing.RouteGroupBuilder.GroupEndpointDataSource.GetGroupedEndpointsWithNullablePrefix(RoutePattern prefix, IReadOnlyList`1 conventions, IReadOnlyList`1 finallyConventions, IServiceProvider applicationServices)
   at Microsoft.AspNetCore.Routing.RouteGroupBuilder.GroupEndpointDataSource.get_Endpoints()
   at Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.CreateEndpointsUnsynchronized()
   at Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.EnsureEndpointsInitialized()
   at Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.get_Endpoints()
   at Microsoft.AspNetCore.Routing.DataSourceDependentCache`1.Initialize()
   at System.Threading.LazyInitializer.EnsureInitializedCore[T](T& target, Boolean& initialized, Object& syncLock, Func`1 valueFactory)
   at Microsoft.AspNetCore.Routing.Matching.DataSourceDependentMatcher..ctor(EndpointDataSource dataSource, Lifetime lifetime, Func`1 matcherBuilderFactory)
   at Microsoft.AspNetCore.Routing.Matching.DfaMatcherFactory.CreateMatcher(EndpointDataSource dataSource)
   at Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.InitializeCoreAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.<Invoke>g__AwaitMatcher|10_0(EndpointRoutingMiddleware middleware, HttpContext httpContext, Task`1 matcherTask)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

This suggests that the changes are so breaking that it is basically impossible to use Microsoft.OpenApi v2 with ASP.NET Core versions earlier than 10. This creates a difficult situation for us, as we would potentially need to conditionally compile across TFMs and only be able to support OpenAPI 3.1 with net10.0 and need to stay on 1.x for lower TFMs while we still support them. This could get quite messy and complex quite quickly.

Maybe it's not actually that bad and it's an artifact of how our integration test suite is set up, but it's not an encouraging start. We can get further insight on that once ASP.NET Core 10 Preview 1 is available, as then this PR can be updated to additionally target net10.0 as that has support for OpenAPI 3.1.

/cc @captainsafia For thoughts on the above.

Also applies some refactorings in files that needed to be touched, but those will be extracted out and merged ahead of time where possible in a future separate PR. #3254

@martincostello martincostello added dependencies Pull requests that update a dependency file .NET Pull requests that update .NET code version-major A change suitable for release in a major version labels Feb 5, 2025
@captainsafia
Copy link
Contributor

This suggests that the changes are so breaking that it is basically impossible to use Microsoft.OpenApi v2 with ASP.NET Core versions earlier than 10.

In general, yes. We can't introduce breaking changes in .NET 9/.NET 8 at this point so there's no way for us to retroactively upgrade the OpenAPI.NET version in those versions.

I believe the crux of the issue here is with the OpenApiOperation metadata that gets included in the document as part of the WithOpenApi call. In #2849, we made the decision to keep this behavior for back-compat but perhaps a new major version of the Swashbuckle.AspNetCore package is a safe place to introduce that break?

We can validate this by seeing if a project that contains no references to Microsoft.AspNetCore.OpenApi, a reference to the new major version of Swashbuckle.AspNetCore, and targets net10 is also suspectible to this issue.

@martincostello
Copy link
Collaborator Author

We can't introduce breaking changes in .NET 9/.NET 8 at this point so there's no way for us to retroactively upgrade the OpenAPI.NET version in those versions.

I was thinking less "add support for it" and more "not hard dependent on it", but I guess the difference is moot with regards to .NET's servicing branches if any changes need to be made to enable that.

The ideal scenario for Swashbuckle would be that using it in a .NET 8/9 app is possible, provided that that any APIs dependent on v1 can be opted-out of. That is to say, it would still be a breaking change (you need to change your filters, customisations etc.), but you could still move up without using .NET 10 if you wanted to use the new OpenAPI 3.1 features.

I guess that's something I can experiment with more in our tests by removing the Microsoft.AspNetCore.OpenApi dependency here and see what happens.

The most radical solution could be "new versions only support .NET 10" on the basis that 8 and 9 go out of support in 2026, and we just draw a hard line and also drop .NET Framework support going forwards. That would certainly make the codebase easier to support going forwards, it might just be unpopular with users that .NET 8 and 9 apps won't get Swashbuckle bug fixes for 6-12 months (unless we do branches and backporting for earlier versions, which of course has its own costs for an OSS project).

We already have at least one major version in the pipeline to drop .NET 6 support (#3183) with no concrete date yet, so if necessary we could always lay the groundwork for Microsoft.OpenApi v2 by dropping some things incrementally over the year as we move towards .NET 10 shipping so that there's less of a difference and people who care about using latest/using OpenAPI 3.1 have time to prepare.

Cherry-pick code refactoring/modernisation from #3252.
Upgrade to preview 5 of Microsoft.OpenApi v2.
Lots of tests are broken.
Also applies some refactorings in files that needed to be touched, but those will be extracted out and merged ahead of time where possible in a future PR.
Reset `Stream` after copy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file .NET Pull requests that update .NET code version-major A change suitable for release in a major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants