Skip to content

Improve IFormCollection.Items documentation #44372

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Apr 4, 2023
Merged

Improve IFormCollection.Items documentation #44372

merged 14 commits into from
Apr 4, 2023

Conversation

sebastienros
Copy link
Member

Fixes #36712

I might have too many details in this comment, but at least the reviewers will have all the pieces to suggest what should go where.

@ghost ghost added the area-runtime label Oct 5, 2022
JamesNK
JamesNK requested changes Oct 5, 2022
View reviewed changes
Tratcher
Tratcher requested changes Oct 5, 2022
View reviewed changes
@build-analysis build-analysis bot mentioned this pull request Oct 5, 2022
Closed
2 tasks
JamesNK
JamesNK reviewed Oct 6, 2022
View reviewed changes
Tratcher
Tratcher reviewed Oct 6, 2022
View reviewed changes
@sebastienros
Copy link
Member Author

@Tratcher GTG?

@build-analysis build-analysis bot mentioned this pull request Oct 12, 2022
Closed
2 tasks
Tratcher
Tratcher reviewed Oct 14, 2022
View reviewed changes
src/Http/Http.Features/src/IFormCollection.cs Outdated
/// </para>
/// <para>
/// Invoking this property could result in thread exhaustion since it's wrapping an asynchronous implementation.
/// To prevent this the method <see cref="Microsoft.AspNetCore.Http.Features.FormFeature.ReadFormAsync" /> can be used.
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem to like your ReadFormAsync reference.

Tratcher
Tratcher reviewed Oct 14, 2022
View reviewed changes
@ghost
Copy link

ghost commented Jan 25, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 25, 2023
@adityamandaleeka adityamandaleeka removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 25, 2023
@ghost
Copy link

ghost commented Jan 25, 2023

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 44372 in repo dotnet/aspnetcore

@Tratcher
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 2, 2023
@amcasey
Copy link
Member

amcasey commented Mar 23, 2023

@JamesNK Do you still have concerns?

@amcasey
Copy link
Member

amcasey commented Mar 23, 2023

@sebastienros James might appreciate it if the tests were green before he gave his sign-off. 😉

captainsafia
captainsafia approved these changes Mar 27, 2023
View reviewed changes
src/Http/Http.Abstractions/src/HttpRequest.cs Outdated
/// <remarks>
/// <para>
/// Invoking this property could result in thread exhaustion since it's wrapping an asynchronous implementation.
/// To prevent this the method <see cref="ReadFormAsync" /> can be used.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// To prevent this the method <see cref="ReadFormAsync" /> can be used.
/// To prevent this the method <see cref="ReadFormAsync(CancellationToken)"/> can be used.

I think this should do it?

Copy link
Member

Choose a reason for hiding this comment

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

Why do you need the parameter list if there's only one overload? It seems to work without in local tests.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, it was supposed to be the one in src/Http/Http.Features/src/IFormCollection.cs, not in this file. Yes, either add (CancellationToken) or remove ().

halter73
halter73 reviewed Mar 27, 2023
View reviewed changes
src/Http/Http.Features/src/IFormCollection.cs Outdated
/// </para>
/// <para>
/// Invoking this property could result in thread exhaustion since it's wrapping an asynchronous implementation.
/// To prevent this the method <see cref="HttpContext.Request.ReadFormAsync()" /> can be used.
Copy link
Member

Choose a reason for hiding this comment

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

ReadFormAsync() returns this type (IFormCollection) at which point using this indexer is expected, so I don't think this warning belongs here. I think the warning on HttpRequest.Form is sufficient. We could also add it to IFormFeature.Form. It looks like IFormFeature.ReadForm() already has a similar warning, but we could update it to match.

JamesNK
JamesNK reviewed Mar 27, 2023
View reviewed changes
JamesNK
JamesNK reviewed Mar 27, 2023
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Fix cref

@sebastienros sebastienros enabled auto-merge (squash) March 27, 2023 23:36
JamesNK
JamesNK approved these changes Mar 28, 2023
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Fix cref II

JamesNK
JamesNK reviewed Mar 28, 2023
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Http.Features doesn't have a reference to HttpRequest!

@sebastienros
Copy link
Member Author

Source code generators are almost easier to code

@JamesNK
Copy link
Member

JamesNK commented Mar 28, 2023 •
edited
Loading

I just noticed Stephen's feedback:

ReadFormAsync() returns this type (IFormCollection) at which point using this indexer is expected, so I don't think this warning belongs here. I think the warning on HttpRequest.Form is sufficient. We could also add it to IFormFeature.Form. It looks like IFormFeature.ReadForm() already has a similar warning, but we could update it to match.

I agree that the warning shouldn't be here. IFormCollection is already in memory. Using an indexer on it isn't blocking.

@halter73 halter73 disabled auto-merge March 28, 2023 01:57
@halter73 halter73 removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 28, 2023
campersau
campersau reviewed Mar 28, 2023
View reviewed changes
src/Http/Http/src/Features/FormFeature.cs
@@ -101,11 +101,10 @@ public IFormCollection ReadForm()

if (!HasFormContentType)
{
throw new InvalidOperationException("Incorrect Content-Type: " + _request.ContentType);
throw new InvalidOperationException("This request does not have a Content-Type header. Forms are available from requests with bodies like POSTs and a form Content-Type of either application/x-www-form-urlencoded or multipart/form-data.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This request does not have a Content-Type header.

I think this is not correct, request can have a Content-Type header but maybe not the correct/expected ones, so I liked the previous version with Incorrect more. Maybe Missing or incorrect Content-Type header.?

I personally liked having the actual Content-Type in the exception message as well: _request.ContentType.

Should this also be updated?
https://github.com/dotnet/aspnetcore/pull/44372/files#diff-6c007cfec0e65e3cfcda8478b356cfae73205755d7a3fde70fe101acedee2533R136

@ghost
Copy link

ghost commented Apr 4, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 4, 2023
@sebastienros sebastienros merged commit 5b1c818 into main Apr 4, 2023
@sebastienros sebastienros deleted the sebros/36712 branch April 4, 2023 16:00
@ghost ghost added this to the 8.0-preview4 milestone Apr 4, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 left review comments

@JamesNK JamesNK JamesNK approved these changes

@campersau campersau campersau left review comments

@amcasey amcasey amcasey left review comments

@adityamandaleeka adityamandaleeka adityamandaleeka approved these changes

@Tratcher Tratcher Tratcher approved these changes

@captainsafia captainsafia captainsafia approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Milestone
8.0-preview4
Development

Successfully merging this pull request may close these issues.

Behavior is different to what is documented - IFormCollection.Item[String] Property
8 participants
@sebastienros @Tratcher @amcasey @JamesNK @halter73 @adityamandaleeka @captainsafia @campersau