Skip to content

Adding nullable type functionality for HttpRequest Context Type #32203

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

Merged
6 commits merged into from
Apr 28, 2021

Conversation

KellenCarl
Copy link
Contributor

…icAPI.Unshipped.txt to reflect change to nullable type.

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

PR Title
Adding nullability annotation on HttpRequest.ContentType

PR Description
Added "?" to make type string nullable for ContentType. Updated PublicAPI.Unshipped.txt to reflect change. I know there is at least one implementation error, but wanted to let code owners make decisions, provide feedback.

Addresses #32097

…cAPI.Unshipped.txt to reflect change to nullable type.
@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Apr 27, 2021
@dnfadmin
Copy link

dnfadmin commented Apr 27, 2021

CLA assistant check
All CLA requirements met.

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

The build shows two usages of this property that also need updating, DefaultHttpRequest and HttpLoggingMiddleware.

@BrennanConroy
Copy link
Member

Looks like a couple places need to react to the nullability addition:
src/Middleware/HttpLogging/src/HttpLoggingMiddleware.cs#L118
src/Hosting/Hosting/src/Internal/HostingRequestStartingLog.cs#L50

@Tratcher
Copy link
Member

Good, this fix found a bug in the logging middleware (@jkotalik). MediaTypeHelpers.TryGetEncodingForMediaType should allow string? and this should be changed to TryParse:

var mediaType = new MediaTypeHeaderValue(contentType);

@KellenCarl
Copy link
Contributor Author

I am currently unable to figure out how to apply the TryParse. I see the MediaTypeHeaderValue has a method for it, but can't quite get it to work. I'll go ahead with the other fixes.

@KellenCarl
Copy link
Contributor Author

I think this is what you were referring to:
31. MediaTypeHeaderValue.TryParse(contentType, out var mediaType);

When I added that, I received a build error that line 33 would be exposed to the possibility of a null value. And I didn't want to interfere with that logic or implementation.

  1.        if (mediaType.Charset.HasValue)
    

@KellenCarl
Copy link
Contributor Author

This implementation might work, but I am not sure this is what you are going for, or what you want to do with the null value:

if (!MediaTypeHeaderValue.TryParse(contentType, out var mediaType))
{
return false;
}

@Tratcher
Copy link
Member

This implementation might work, but I am not sure this is what you are going for, or what you want to do with the null value:

if (!MediaTypeHeaderValue.TryParse(contentType, out var mediaType))
{
return false;
}

That's what I had in mind. It avoids throwing for null, empty, or bad contentTypes.

…EncodingForMediaType. If true, continues through Encoding process.
@Tratcher Tratcher added this to the 6.0-preview5 milestone Apr 28, 2021
@ghost
Copy link

ghost commented Apr 28, 2021

Hello @Tratcher!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 5fb8d0f into dotnet:main Apr 28, 2021
@Tratcher
Copy link
Member

Tratcher commented Apr 28, 2021

Thanks @KellenCarl!

Announcement: aspnet/Announcements#444 (comment)

@KellenCarl
Copy link
Contributor Author

Thank you @Tratcher! I appreciate all the help and feedback!

@ghost
Copy link

ghost commented Apr 28, 2021

Hi @KellenCarl. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 6, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares 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.

6 participants