Update Swashbuckle.AspNetCore to latest - #67136
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the centrally-managed Swashbuckle.AspNetCore dependency version used across the repo, aligning the build’s external dependency set with a newer upstream release while the gRPC Swagger deprecation work (Issue #67134) is still pending.
Changes:
- Bump
Swashbuckle.AspNetCorefrom6.6.2to10.2.1viaeng/Versions.props.
wtgodbe
left a comment
There was a problem hiding this comment.
May need to kick off a run of https://dev.azure.com/dnceng/internal/_build?definitionId=931&_a=summary to get the package mirrored
Looks like it's already in dotnet-public since 2nd June. |
| <StackExchangeRedisVersion>2.7.27</StackExchangeRedisVersion> | ||
| <SystemReactiveLinqVersion>5.0.0</SystemReactiveLinqVersion> | ||
| <SwashbuckleAspNetCoreVersion>6.6.2</SwashbuckleAspNetCoreVersion> | ||
| <SwashbuckleAspNetCoreVersion>10.2.1</SwashbuckleAspNetCoreVersion> |
There was a problem hiding this comment.
This is multiple major (breaking change) releases upgrade. If we are impacted by too much breaking changes, it might not be worth spending the time to fix those. Waiting for CI to tell.
There was a problem hiding this comment.
The breaking changes should all be related to the upgrade from Microsoft.OpenApi v1 ➡️ v2.
|
@copilot Try to address the build errors in Microsoft.AspNetCore.Grpc.Swagger.Tests |
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
|
@copilot There are additional errors: |
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
... Addressed in d520f839. The remaining gRPC Swagger test breakages now use the updated |
| { | ||
| var code = responseNodes.Current!.GetAttribute("code", ""); | ||
| if (!operation.Responses.TryGetValue(code, out var response)) | ||
| if (!operation.Responses!.TryGetValue(code, out var response)) |
There was a problem hiding this comment.
| if (!operation.Responses!.TryGetValue(code, out var response)) | |
| if (operation.Responses?.TryGetValue(code, out var response) == false) |
can we handle the operation.Responses being null somehow more elegantly? maybe do null-assignment as well?
if we do not need it, maybe adding a comment helps explain this null-forgiving operator?
There was a problem hiding this comment.
The API is extremely confusing:
Esp when the documentation says "REQUIRED".
@baywet Could you clarify why Responses is annotated as nullable while the documentation says it's required?
There was a problem hiding this comment.
This was introduced in microsoft/OpenAPI.NET#1710
There was a problem hiding this comment.
@Youssef1313 I think the doc comment is an oversight... it's 9 years old!
The specification DOES NOT call this field out as required. Example of a required field in the spec
There was a problem hiding this comment.
@baywet And besides that, is an empty dictionary here different from null?
Given that the property is already initialized to empty dictionary, what's the use case for setting it later to null? Why is that allowed?
There was a problem hiding this comment.
I'm not sure why it's initialized to an empty dictionary, IMHO it shouldn't for multiple reasons:
- deserialization: some documents might not have that property
- confusing mix of implementation and API surface as you've outlined
- increased memory consumption/pressure as we probably throw away that instance in a couple of scenarios like deserialization when responses are present, and collection initialization when creating a new document from the object model.
Again, the initial "initialization code" of the property is 9 years old!. I suspect this was done because at the time nullable reference types, which were introduced in C#8/2019, didn't exist.
There was a problem hiding this comment.
As for changing the value (not initializing it), it's been in here for a long time. I suspect it'd cause a bunch of regressions even though the API surface "says" you can have a null value in here.
As for changing the API surface to make it non-nullable, that'd be a breaking change, and wouldn't match the specification.
There was a problem hiding this comment.
I don't think in this instance the specification makes a difference between null and empty for that collection. However it does for things like security requirements. Which is one reason why the library offers the distinction between null and empty collections as a general design principle.
| @@ -100,7 +100,12 @@ private static void ApplyResponseTags(OpenApiOperation operation, XPathNodeItera | |||
| while (responseNodes.MoveNext()) | |||
There was a problem hiding this comment.
| while (responseNodes.MoveNext()) | |
| operation.Responses ??= []; | |
| while (responseNodes.MoveNext()) |
| if (operation.Responses is null) | ||
| { | ||
| operation.Responses = []; | ||
| operation.Responses[code] = response = new OpenApiResponse(); | ||
| } | ||
| else if (!operation.Responses.TryGetValue(code, out var response)) |
There was a problem hiding this comment.
| if (operation.Responses is null) | |
| { | |
| operation.Responses = []; | |
| operation.Responses[code] = response = new OpenApiResponse(); | |
| } | |
| else if (!operation.Responses.TryGetValue(code, out var response)) | |
| if (!operation.Responses.TryGetValue(code, out var response)) |
Wouldn't this perform better?
|
We will still prefer to simply tackle #67134 instead. |
Until dotnet/aspnetcore#67134 is done, I think it should be safe to take this dependency update.