QueryStringEnumerable API

[SteveSandersonMS]

Implements #33840, hopefully covering the suggestions already raised there.

Fixing an inconsistency (or not?)

There was a weird inconsistency in the parsing behavior regarding whitespace.

• QueryHelpers.ParseQuery previously trimmed any leading char.IsWhiteSpace characters from the key (but not any trailing ones from the key, and not either leading or trailing ones from the value)
• QueryFeature's querystring parser did not do that (it retained whitespace at both ends of key and value)

The behavior in QueryHelpers.ParseQuery doesn't really make sense to me. Why only trim leading and not trailing? Why do none of the unit tests cover this? Why didn't QueryFeature have the same behavior? Also note that it was done at the cost of stepping through a potentially 8kb-long string character-by-character.

To clean this up, and since the logic in QueryFeature looks newer, I've standardized on the don't-trim behavior in the new common logic.

Does anyone know why the weird sometimes-trimming behavior existed before? Apparently it's been there for 7 years and is too far back to even see the original commit by following normal GitHub links. I'm happy to put it back if there's a reason, or if this raises a serious concern about breaking. If we are putting it back, do we want to retain the inconsistency between QueryHelpers and QueryFeature, or should QueryFeature also get this odd behavior? Or should we regard the change as breaking and do an announcement?

[SteveSandersonMS]

The reason for shared-source is that, in a PR coming very soon to a repo near you, I'll be using QueryStringEnumerable as an internal API inside Microsoft.AspNetCore.Components. That package does not have a dependency on WebUtilities, and doesn't want to have one, since it's not web-specific and most of the baggage brought with WebUtilities would not be applicable.

[Tratcher]

A query string shouldn't have any white space in it prior to un-escaping keys and values.

This code goes all the way back to Katana where the same parser was shared across a few similar formats like query, form, etc.:
https://github.com/aspnet/AspNetKatana/blob/fae38fae683bb0977bcaf7c3ffce244aced82135/src/Microsoft.Owin/Infrastructure/OwinHelpers.cs#L557

We've since split each of those up into their own parsers, but apparently there were leftovers.

[SteveSandersonMS]

A query string shouldn't have any white space in it prior to un-escaping keys and values.

Can you clarify this point? Are you saying the scenario is not applicable so we don't need to care about it? Or are you saying we must trim whitespace (and if so, why is it OK not to trim trailing whitespace, and why doesn't the QueryFeature code do that?)

I'm guessing you mean the scenario isn't applicable, presumably because HTTP doesn't have a way to represent unencoded whitespace in the request line. Is that right?

[Tratcher]

Any thoughts on adding a new (extension?) method to QueryString that exposed this new enumerator? QueryString.GetEnumerator()?

[Tratcher]

A query string shouldn't have any white space in it prior to un-escaping keys and values.

Can you clarify this point? Are you saying the scenario is not applicable so we don't need to care about it? Or are you saying we must trim whitespace (and if so, why is it OK not to trim trailing whitespace, and why doesn't the QueryFeature code do that?)

I'm guessing you mean the scenario isn't applicable, presumably because HTTP doesn't have a way to represent unencoded whitespace in the request line. Is that right?

It's not applicable an can be ignored because a well formed URI does not allow whitespace anywhere, they must be escaped. This isn't even an HTTP specific requirement.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[pranavkm]

• We think DecodeName / DecodeValue are useful APIs to have. With DecodeName we can optimize for the common cases where the name does not need decoding and return the encoded value as is. For DecodeValue it's less useful in the common case, but could help in the event if the value need to be parsed (e.g. int.TryParse).

• If we do need to share this publicly in the future, we can consider creating a M.Extensions.HttpHelperUtility package.

[SteveSandersonMS]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[ghost]

Hello @SteveSandersonMS!

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.