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.

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
merged 14 commits into from
Apr 4, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/Http/Http.Abstractions/src/HttpRequest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,24 @@ public abstract class HttpRequest
/// <summary>
/// Gets or sets the request body as a form.
/// </summary>
/// <exception cref="System.InvalidOperationException">
/// incorrect content-type.
/// </exception>
/// <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(CancellationToken)"/> can be used.
/// For more information, see <see href="https://aka.ms/aspnet/forms-async" />.
/// </para>
/// </remarks>
public abstract IFormCollection Form { get; set; }

/// <summary>
/// Reads the request body if it is a form.
/// </summary>
/// <exception cref="System.InvalidOperationException">
/// incorrect content-type.
/// </exception>
/// <returns></returns>
public abstract Task<IFormCollection> ReadFormAsync(CancellationToken cancellationToken = new CancellationToken());

Expand Down
14 changes: 14 additions & 0 deletions src/Http/Http.Features/src/IFormCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,24 @@ public interface IFormCollection : IEnumerable<KeyValuePair<string, StringValues
/// <exception cref="System.ArgumentNullException">
/// key is null.
/// </exception>
/// <exception cref="System.InvalidOperationException">
/// incorrect content-type.
/// </exception>
/// <remarks>
/// <para>
/// <see cref="IFormCollection" /> has a different indexer contract than
/// <see cref="IDictionary{TKey, TValue}" />, as it will return <c>StringValues.Empty</c> for missing entries
/// rather than throwing an Exception.
/// </para>
/// <para>
/// This indexer can only be used on POST requests. Otherwise an exception of type
/// <see cref="System.InvalidOperationException" /> is thrown.
/// </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.

/// For more information, see <see href="https://aka.ms/aspnet/forms-async" />.
/// </para>
/// </remarks>
StringValues this[string key] { get; }

Expand Down
5 changes: 2 additions & 3 deletions src/Http/Http/src/Features/FormFeature.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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

}

// TODO: Issue #456 Avoid Sync-over-Async http://blogs.msdn.com/b/pfxteam/archive/2012/04/13/10293638.aspx
// TODO: How do we prevent thread exhaustion?
// c.f., https://aka.ms/aspnet/forms-async
return ReadFormAsync().GetAwaiter().GetResult();
}

Expand Down