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

Microsoft.AspNetCore.Http: Decorate priorFeature as nullable on StreamResponseBodyFeature ctor #38875

Merged

Conversation

CodeBlanch
Copy link
Contributor

Passing null for priorFeature seems to be a totally valid case for StreamResponseBodyFeature but the parameter isn't flagged as nullable.

What I want to do is this:

var redirectedResponseBodyFeature = new StreamResponseBodyFeature(new MemoryStream(), context.Features.Get<IHttpResponseBodyFeature>());

But to make it happy (as far as nullable analysis warnings go) I have to do:

var priorFeature = context.Features.Get<IHttpResponseBodyFeature>();
var redirectedResponseBodyFeature = priorFeature == null
   ? new StreamResponseBodyFeature(new MemoryStream())
   : new StreamResponseBodyFeature(new MemoryStream(), priorFeature);

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Dec 7, 2021
@Tratcher
Copy link
Member

Tratcher commented Dec 7, 2021

The nullability change looks correct, we just need to fix the API file. If you open it in VS it should give you a tool hint to fix it automatically. It needs to show that the old API was removed.

@CodeBlanch CodeBlanch requested a review from a team as a code owner December 7, 2021 21:42
@CodeBlanch
Copy link
Contributor Author

@Tratcher For some reason the VS hint only added it to unshipped, didn't remove from shipped. I just did that manually let me know if it looks good.

@@ -1 +1,2 @@
#nullable enable
Microsoft.AspNetCore.Http.StreamResponseBodyFeature.StreamResponseBodyFeature(System.IO.Stream! stream, Microsoft.AspNetCore.Http.Features.IHttpResponseBodyFeature? priorFeature) -> void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Microsoft.AspNetCore.Http.StreamResponseBodyFeature.StreamResponseBodyFeature(System.IO.Stream! stream, Microsoft.AspNetCore.Http.Features.IHttpResponseBodyFeature? priorFeature) -> void
*REMOVED*Microsoft.AspNetCore.Http.StreamResponseBodyFeature.StreamResponseBodyFeature(System.IO.Stream! stream, Microsoft.AspNetCore.Http.Features.IHttpResponseBodyFeature! priorFeature) -> void
Microsoft.AspNetCore.Http.StreamResponseBodyFeature.StreamResponseBodyFeature(System.IO.Stream! stream, Microsoft.AspNetCore.Http.Features.IHttpResponseBodyFeature? priorFeature) -> void

Copy link
Member

@halter73 halter73 Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You beat me to it! @dotnet/aspnet-build Any idea why the code fix doesn't do it this way automatically? Is it not supposed to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, @agocke do you know who we should consult with on the API analyzer?

Copy link
Contributor

@pranavkm pranavkm Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codefix updates the shipping file as intended. For some reason our team insists it is wrong thing to do: #31076 (comment) even tho these files exist solely for the benefit of the tool. 🤷🏽

@pranavkm pranavkm merged commit d84666f into dotnet:main Dec 8, 2021
@ghost ghost added this to the 7.0-preview1 milestone Dec 8, 2021
@CodeBlanch CodeBlanch deleted the StreamResponseBodyFeature-ctor-nullable branch February 2, 2022 17:52
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants