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

Invalid nullability annotation on HttpRequest.ContentType #32097

Closed
bart-degreed opened this issue Apr 23, 2021 · 9 comments
Closed

Invalid nullability annotation on HttpRequest.ContentType #32097

bart-degreed opened this issue Apr 23, 2021 · 9 comments
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-http-abstractions good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue nullable
Milestone

Comments

@bart-degreed
Copy link

After updating my Web API project from .NET Core 3.1 to .NET 5, I'm getting a warning along the lines of "variable is never null".

var contentType = httpContext.Request.ContentType;

if (contentType != null && contentType != allowedContentType)
{
// ^^^^^^^^^^^^^
}

The warning is incorrect, because when I remove the null check, lots of my integration tests start to fail. Specifically, the ones where no Content-Type header is sent in the request.

This makes me believe the HttpRequest.ContentType property type should be string? instead of string.

@ghost
Copy link

ghost commented Apr 23, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@BrennanConroy BrennanConroy added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Apr 23, 2021
@BrennanConroy
Copy link
Member

Good catch thanks! Interested in contributing a change for this?

@BrennanConroy BrennanConroy added the good first issue Good for newcomers. label Apr 23, 2021
@bart-degreed
Copy link
Author

Thanks for the offer, but I'd be happy to give someone else the opportunity to take this.

@KellenCarl
Copy link
Contributor

KellenCarl commented Apr 26, 2021

I'm interested in contributing. This would be my first non-doc contribution. If you and/or other reviewers don't mind helping me through the process, I'd be more than happy to do the "leg-work." Just let me know what needs to be done, and I can give it my best. I've read the contribution guidelines and code of conduct.

Are you looking to just have the "?" added to the HttpRequest.ContentType to make the type nullable, and run a unit test for both type string and null?

@BrennanConroy
Copy link
Member

Are you looking to just have the "?" added to the HttpRequest.ContentType to make the type nullable

Yep! The trick with changing APIs is that we need to update the baseline files. You can read about that at https://github.com/dotnet/aspnetcore/blob/main/docs/APIBaselines.md#updated-apis. Essentially a *REMOVED*some.api and some.api? will be added to the unshipped.txt file.

Another thing with updating nullability on an API is that it can affect other code that might be using the changed API and didn't expect the property to ever be null. Fortunately the PR builds will find any of those, but building a couple projects locally can help find some of those quicker so there is less PR churn 😃

run a unit test for both type string and null?

Don't think that's needed for this change, we're not changing any behavior or adding new code, but good thought 👍

@KellenCarl
Copy link
Contributor

Ok. Sounds good. I'll start taking a look into this.

@Tratcher
Copy link
Member

@bart-degreed
Copy link
Author

Thanks for the fix! Which release will include it?

@Tratcher
Copy link
Member

6.0-preview5. since this is an api change it's not something we can backport.

@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-http-abstractions good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue nullable
Projects
None yet
Development

No branches or pull requests

6 participants