Aspnetcore: QueryHelpers.AddQueryString should support array type parameters

[CurlyBraceTT]

Is your feature request related to a problem? Please describe.

I am trying to create complex queries from generic object, the opposite of what [fromQuery] attribute is doing related to class-types. QueryHelpers.AddQueryString would do, however I need to access private version with
IEnumerable<KeyValuePair<string, string>> queryString parameter, or a new version with Dictionary<string, StringValues> parameter.

https://github.com/aspnet/HttpAbstractions/blob/master/src/Microsoft.AspNetCore.WebUtilities/QueryHelpers.cs#L63

To support array types like for queries like ids=1&ids=2&ids=3, because Dictionary version forces key uniqueness.

Describe the solution you'd like

public modification for the linked method or another way to create query from the object, including support the list of parameters like ids=1&ids=2&ids=3

Describe alternatives you've considered

Add each value with AddQueryString(string uri, string name, string value). I wonder how massive string concat will impact memory, also I think methods should support each other so I could easily use AddQueryString and ParseQuery together.

Additional context

Code context:

public static string AppendObjectToQueryString(string uri, object requestObject)
{
    var type = requestObject.GetType();
    var data = type.GetProperties(BindingFlags.Public | BindingFlags.Instance)
        .ToDictionary
        (
            p => p.Name,
            p => p.GetValue(requestObject)
        );

    foreach (var d in data)
    {
        if (d.Value == null)
        {
            continue;
        }

        if ((d.Value as string == null) && d.Value is IEnumerable enumerable)
        {
            foreach (var value in enumerable)
            {
                uri = QueryHelpers.AddQueryString(uri, d.Key, value.ToString());
            }
        }
        else
        {
            uri = QueryHelpers.AddQueryString(uri, d.Key, d.Value.ToString());
        }
    }

    return uri;
}

[wojteksbt]

Is changing of access modifier an approved solution? I'm happy to take it as my first issue.

[bharath-nepoleon]

@anurse I am interested in taking this. Is creating a new method with parameter of type Dictionary<string, StringValues> an acceptable solution? or should I just change the access modifier of the existing method?

[ericsampson]

@anurse I am interested in taking this. Is creating a new method with parameter of type Dictionary<string, StringValues> an acceptable solution? or should I just change the access modifier of the existing method?

I think it would be great to have a new method with parameter of type Dictionary<string, StringValues> @bharath-nepoleon . I went looking for this and was sad that it didn't already exist :)

[anurse]

I'd suggest taking IEnumerable<KeyVaultPair<string, StringValues>> if you're going to do that (possibly just making the existing one public, if it's appropriate). The Dictionary<string, StringValues> class implements IEnumerable<KeyVaultPair<string, StringValues>> so you'd be able to pass it in directly.

A new public API like that would need to go through an API review, and it might be worth talking a little about it on this issue, but if you want to go ahead and sketch out a PR @bharath-nepoleon , that would be fine!

cc @Tratcher @davidfowl @rynowak for thoughts here?

[ericsampson]

Of course, didn't look close enough when I copy-pasted the original comment :) Cheers!

[Tratcher]

Making the existing private static string AddQueryString(string uri, IEnumerable<KeyValuePair<string, string>> queryString) public should be fine.

I'd be curious to see the calling code sample above re-written to show the difference between using string vs StringValues.

[ericsampson]

Making the existing private static string AddQueryString(string uri, IEnumerable<KeyValuePair<string, string>> queryString) public should be fine.

Am I missing something @Tratcher , but I was thinking the more desirable would be to take in IEnumerable<KeyVaultPair<string, StringValues>> to mirror the existing public static Dictionary<string, StringValues> ParseQuery(string queryString) no? At least that's how I wanted to use it in the past - extract from ParseQuery of incoming response, modify, then reconstruct with AddQueryString() to call another API etc.

[Tratcher]

You're unlikely to pass the values directly from ParseQuery to AddQueryString, no? You'll probably be creating the collection manually, so which would be easier to create? IEnumerable<KeyVaultPair<string, StringValues>> can require extra effort to aggregate values and create that StringValues, where with IEnumerable<KeyValuePair<string, string>> you can add values at will without worrying about aggregation. Hence asking to see a code sample for both styles.

[FranklinWhale]

@Tratcher A code sample is as follows:

```c#
private static string AddDefaultQueryParams(Uri uri) {
var queries = QueryHelpers.ParseQuery(uri.Query);
if (queries.ContainsKey("param1") == false) {
queries["param1"] = "value1";
}
if (queries.ContainsKey("param2") == false) {
queries["param2"] = "value2";
}

// If `AddQueryString(string uri, IEnumerable<KeyValuePair<string, StringValues>> queryString)` exists:
return QueryHelpers.AddQueryString(uri.GetLeftPart(UriPartial.Path), queries);

// If `AddQueryString(string uri, IEnumerable<KeyValuePair<string, string>> queryString)` is public:
return QueryHelpers.AddQueryString(uri.GetLeftPart(UriPartial.Path), queries.SelectMany(kvp => kvp.Value, (kvp, v) => KeyValuePair.Create(kvp.Key, v.ToString())));

// The current solution:
return new Uri(uri, "?" + queries.ToQueryString()).ToString();

}

public static class QueryDictionaryExtensions {
public static string ToQueryString(this IEnumerable> queries) {
return String.Join('&', queries.SelectMany(kvp => kvp.Value, (kvp, v) => $"{Uri.EscapeDataString(kvp.Key)}={Uri.EscapeDataString(v)}"));
}
}
```

[FranklinWhale]

I have updated the code sample to provide a solution that works in the current release of .NET Core. I think the ToQueryString extension method may be useful in other scenario too.

[Tratcher]

We don't want to add scenario specific extension methods like this to general types like IEnumerable. Yes it's constrained by generics, but it would still show up in unexpected places like request and response header collections.
https://github.com/dotnet/aspnetcore/blob/bea08d997a9a8201d0edf1f4a73de8c38c2830fa/src/Http/Http.Features/src/IHeaderDictionary.cs#L12

[FranklinWhale]

Although not strictly related to this issue, I think there should be a new constructor for QueryBuilder that accepts IEnumerable<KeyValuePair<string, StringValues>>. Currently, it is not easy to utilize the output of ParseQuery because of StringValues.

[Tratcher]

Although not strictly related to this issue, I think there should be a new constructor for QueryBuilder that accepts IEnumerable<KeyValuePair<string, StringValues>>. Currently, it is not easy to utilize the output of ParseQuery because of StringValues.

Sure, if you want to give it a try.