Proposal: Support for building query string collection in UriBuilder

[deepumi]

Background

Currently UriBuilder class does not have a constructor / method to build query string parameters values.

Please note that I would like to implement this feature in the existing UriBuilder class.

Proposed API

Open questions

• How often is this kind of functionality hand-written by apps? (ideally some links to popular GitHub projects, or links to StackOverflow / blog post samples = real-world examples)
  • In general it looks useful, but it is always good to collect more data.
• What is difference between IEnumerable<KeyValuePair<string, string>> and IDictionary overloads?Do they have different usage patterns? (real-world examples ideally)

public class UriBuilder
{
    public UriQueryStringBuilder();
    public UriQueryStringBuilder(string schemeName, string hostName, string path);
    ...
+    public UriQueryStringBuilder(string schemeName, string hostName, string path, IEnumerable<KeyValuePair<string, string>> queryParameters);
+    public UriQueryStringBuilder(string schemeName, string hostName, string path, IDictionary<string, string> queryParameters);

+    public IDictionary<string, string> QueryParameters {get;}

+    public void AddQueryParameter(string key, string value);
}

Usage

 //1. Add query string
var uri = new UriQueryStringBuilder("https", "api.com", "v1/videos/somevideo");
uri.AddQueryParameter("q", "keword");
uri.AddQueryParameter("s", null);
var result = uri.ToString(); //https://api.com/v1/videos/somevideo?q=keyword&s=

//2. Build query string collection and add query string
var dict = new Dictionary<string, string>();
dict["somthing"] = "value";
dict["query"] = "another value";

var uri1 = new UriQueryStringBuilder("https", "api.com", "v1/videos/somevideo", dict);
uri1.AddQueryParameter("q", "keword");
uri1.AddQueryParameter("s", null);
var result1 = uri1.ToString(); //https://api.com/v1/videos/somevideo?something=value&query=another%20value&q=keyword&s=

//3. Build query string collection only.
var dict1 = new Dictionary<string, string>();
dict1["somthing"] = "value";
dict1["query"] = "another value";

var uri2 = new UriQueryStringBuilder("https", "api.com", "v1/videos/somevideo", dict1);
var result2 = uri2.ToString(); //https://api.com/v1/videos/somevideo?something=value&query=another%20value    

---

Original proposal

public class URIBuilder : System.UriBuilder
{
   private readonly NameValueCollection _queryParameters;

   public URIBuilder(string schemeName, string hostName, string path) : this(schemeName, hostName, path, new NameValueCollection()) { }

   public URIBuilder(string schemeName, string hostName, string path, NameValueCollection queryParameters) : base(schemeName, hostName)
   {
        _queryParameters = queryParameters ?? new NameValueCollection();
        Path = path;
   }

   public NameValueCollection QueryParameters => _queryParameters;

   public URIBuilder AddQueryParameter(string key, string value)
   {
        if (key == null) throw new ArgumentNullException(nameof(key));

        if (value == null) value = string.Empty; //allow empty value otherwise throw null exception

        _queryParameters.Add(key, value);

        return this;
    }

    public override string ToString()
    {
        var query = new StringBuilder();
        foreach (var item in _queryParameters.AllKeys)
        {
            query.Append($"{item}={Uri.EscapeUriString(_queryParameters[item])}&");
        }

        if (query.Length > 0)
        {
            query.Length--; //Trim last character '&'
            Query = query.ToString();
        }
        return base.ToString();
    }
}

Tests

/* 1. Build query string values only.*/

var uri = new  URIBuilder("https","api.com","v1/videos/somevideo")
                      .AddQueryParameter("q","keword")
                      .AddQueryParameter("s",null);

var result = uri.ToString(); //https://api.com/v1/videos/somevideo?q=keyword&s=

/* 2. Build query string collection and add parameter*/

var nvc = new NameValueCollection();
nvc["somthing"] = "value";
nvc["query"] = "another value";

var uri1 = new URIBuilder("https","api.com","v1/videos/somevideo",nvc)
                      .AddQueryParameter("q","keword")
                      .AddQueryParameter("s",null);

var result1 = uri1.ToString(); //https://api.com/v1/videos/somevideo?something=value&query=another%20value&q=keyword&s=

/* 3. Build query string collection only.*/

var nvc1 = new NameValueCollection();
nvc1["somthing"] = "value";
nvc1["query"] = "another value";

var uri2 = new URIBuilder("https","api.com","v1/videos/somevideo",nvc1);

var result2 = uri2.ToString(); //https://api.com/v1/videos/somevideo?something=value&query=another%20value

[svick]

public class URIBuilder : System.UriBuilder

Are you actually suggesting to create a class that differs from UriBuilder only in capitalization? I don't think that's a good idea.

public NameValueCollection QueryParameters

NameValueCollection only implements non-generic collection interfaces. Because of that, I don't think it should be used in new code.

[deepumi]

@svick No, I don't want to add a new class, I wanted the same functionality (method and constructor) to implement in the existing UriBuilder.

How about IEnumerable<KeyValuePair<string, string>> or IDictionary<string,string>?

[deepumi]

@karelz Thank you for adding the label.

Since UriBuilder class belongs to System namespace could you please add area-System label as well.

[karelz]

The area should (typically) match contract assembly (which is System.Runtime.Extensions for UriBuilder).

[deepumi]

@karelz Thank you got it.

[deepumi]

This API needs to be re-designed, Currently there is no unified API in dotnet BCL world to build a URI with QueryString and Path. (Windows, Web and Mobile got a different way to build Query string and path).

[karelz]

Do you mean that your original proposal needs to be updated?

[deepumi]

@karelz I meant to say the existing System.UriBuilder

[deepumi]

@karelz Any idea when this PR will be reviewed?

[karelz]

The API needs to be first reviewed by area experts - @AlexGhiondea @joperezr. They are going through the area now. I am not sure if they will time to finish it this year though.

After that we will need CoreFX API review.

My feedback:

• We will need to change the name. Differentiating between URIBuilder and UriBuilder is really bad IMO. What other alternatives did you consider?
• How often is this kind of functionality hand-written by apps? (ideally some links to popular GitHub projects, or links to StackOverflow / blog post samples)
  • In general it looks useful, but it is always good to collect more data.
• Usage syntax is mixing up fluent API design and modifications API design, that's usually frowned upon (I tried to suggest something similar recently) - we should choose one (IMO the modifications style). If you choose fluent, we should not use Builder in the type name.
• Did you consider integrating the API into UriBuilder? Why would we need a new type? (easier implementation is not a good enough reason)
• What about adding just UriQueryStringBuilder maybe as inner type QueryBuilder?

[deepumi]

@karelz Thank you for your feedback. @AlexGhiondea @joperezr

Did you consider integrating the API into UriBuilder? Why would we need a new type? (easier implementation is not a good enough reason)

Yes, indeed. I have mentioned this in my proposal. I would like to add this into System.UriBuilder class itself, so that we don't need to add a new type.

How often is this kind of functionality hand-written by apps? (ideally some links to popular GitHub projects, or links to StackOverflow / blog post samples)

Well, now a days every single app does some sort of external API calls and currently there is no unified API in BCL world to build a URI with QueryString and Path. (Windows, Web and Mobile got a different way to build Query string and path). Ex: In Asp.Net world we have a type name HttpUtility.ParseQueryString (System.Web). Also, please refer dotnet/corefx#726

Based on your feedback I have integrated my proposal to UriBuilder class.

namespace System
{
    public class UriBuilder
    {
        private readonly NameValueCollection _queryParameters;

        public UriBuilder(string schemeName, string hostName, string path) : this(schemeName, hostName, path, new NameValueCollection()) { }

        public UriBuilder(string schemeName, string hostName, string path, NameValueCollection queryParameters) : this(schemeName, hostName)
        {
            _queryParameters = queryParameters ?? new NameValueCollection();
            Path = path;
        }

        public UriBuilder AddQueryParameter(string key, string value)
        {
            if (key == null) throw new ArgumentNullException(nameof(key));

            _queryParameters.Add(key, value ?? string.Empty);

            return this;
        }

        public NameValueCollection QueryParameters => _queryParameters;

        private void SetQueryParametersFromUri()
        {
            var query = new StringBuilder();
            foreach (var item in _queryParameters.AllKeys)
            {
                query.Append($"{item}={Uri.EscapeUriString(_queryParameters[item])}&");
            }

            if (query.Length > 0)
            {
                query.Length--;
                Query = query.ToString();
            }
        }

        public override string ToString()
        {
            SetQueryParametersFromUri();
            //proceed with rest of the logic here
            //https://referencesource.microsoft.com/#System/net/System/uribuilder.cs,406
        }
    }
}

[AlexGhiondea]

@deepumi thanks for this proposal.

As @svick mentioned, for new APIs we are trying to move away from using non-generic types like NameValueCollection.

As @karelz mentioned, we are trying to stick to a single way of using our APIs. In this case, introducing the fluent APIs might not be something we want to do.

@CIPop as he might have a different view on this.

[deepumi]

@AlexGhiondea Thank you for reviewing it.

I also added a gist to have IDictionary and IEnumerable version for the above code. Please check here https://gist.github.com/deepumi/d6634e38b98e4552ca4dda19bbff0359

[karelz]

@deepumi can you please post just the API shape. Implementation is secondary. Please think through all usage patterns and how it interacts with other APIs (e.g. setting QueryString vs. just adding one pair via your API).

[deepumi]

@karelz sure, thank you.

EDIT: Proposal moved to top post.

[karelz]

Do you plan to address the feedback on fluent API model? What about Query setter interaction?

[deepumi]

@karelz @AlexGhiondea apologize for the delay in getting back to you.

Do you plan to address the feedback on fluent API model?

Yes, modified the API model above and usage version as well.

What about Query setter interaction?

Query setter interaction goes inside ToString() method like this

[AlexGhiondea]

@deepumi can you tell me more about how the IEnumerable<KeyValuePair<string, string>> overload is different than the IDictionary overload? Do they have different usage patterns?

While this is a somewhat useful addition, I am not sure if it is worth the overhead of adding a new type to the framework.

@CIPop, @terrajobst what do you think?

[karelz]

I don't understand the 2 overloads either. @deepumi we need good real-world usage sample links (code patterns which do something similar).

@AlexGhiondea adding a type would be overkill, however latest proposal doesn't do that - it just modifies current UriBuilder. I updated the spec on the top.

[AlexGhiondea]

@karelz updating the existing UriBuilder would be preferable. I would still like to see real world usage patterns.

(also, I am not seeing the updated proposal that uses just UriBuilder)

[karelz]

Yeah, I missed that one, sorry. There is proposal in the middle which integrates it into UriBuilder. @deepumi please announce more carefully all changes you're making to the proposal. It is extremely hard to track changes otherwise.

I rewrote the top post to UriBuilder modification (i.e. I deleted ToString override)

[deepumi]

@karelz thank you.

[CIPop]

@CIPop, @terrajobst what do you think?

I will assume the request is to add functionality to the existing UriBuilder class (not a new class).

I'm not yet convinced: there are quite a few libraries that already do this for REST APIs. It would be an interesting addition if the REST implementations could reuse this implementation.

@davidfowl @danroth27 would ASP.Net benefit from such API? If so, please review the API shape above.

[deepumi]

@CIPop yes, you are correct, I wanted to add the missing features to the existing UriBuilder class.

I'm not yet convinced: there are quite a few libraries that already do this for REST APIs. It would be an interesting addition if the REST implementations could reuse this implementation.

The reason I would like to have this in BCL side because the same API (UriBuilder) can reuse in windows, web and mobile frameworks. Currently building dynamic URL in BCL side I would end up using LINQ and other string operations.

https://api.careeronestop.org/api-explorer/home/index/JobSearch_GetTopLocationsByKeywordAndOnetCode ex :

var qs = new Dictionary<string, string>();
qs["searchId"] = "value";
qs["companyName"] = "value";

var uri = new UriBuilder(Uri.UriSchemeHttps, "api.careeronestop.org")
{
    Path = "/v1/jobsearch/{userId}/{keyword}/{location}/{radius}/{days}",
    Query = string.Join("&", qs.Keys.Select(key => key + "=" + Uri.EscapeUriString(qs[key])))
   //instead of the above linq code it would be great to have a additional parameter in the UriBuilder constructor to accept IDictionary
};

[danroth27]

In ASP.NET Core we have our own QueryBuilder type: https://docs.microsoft.com/en-us/aspnet/core/api/microsoft.aspnetcore.http.extensions.querybuilder. It's available as a NuGet package: https://www.nuget.org/packages/Microsoft.AspNetCore.Http.Extensions.

[KLuuKer]

@danroth27 this issue was created here as per https://github.com/aspnet/HttpAbstractions/issues/726

It would be better to have a more unified class(maybe 2?) that handels all uri\querystring building\parsing.

Also for easier discoverability because for me it also took allot of time (googling in frustration) to find out QueryBuilder existed for a long time while I needed it.

[thomaslevesque]

In fact, this functionality already exists in an unexpected place: HttpUtility.ParseQueryString() returns a specialized implementation of NameValueCollection (private nested class HttpUtility.HttpQSCollection), which overrides ToString() to correctly format the names and values as a query string.

So if you need a query string builder, you can do this:

var builder = HttpUtility.ParseQueryString("");
builder.Add("foo", "bar");
builder.Add("id", "42");
var queryString = builder.ToString();

However, this relies on undocumented behavior, so it's probably not a safe approach...

[qmfrederik]

@thomaslevesque Thanks!

I previously used Microsoft.AspNetCore.Http.Extensions but it no longer ships as a separate package as of .NET Core 3.0, so going back to HttpUtility.ParseQueryString is a good way to fill this gap.

It would be nice to have some of the URL manipulation classes which ship as part of ASP.NET Core available as part of netcoreapp/netstandard, though.

[ghost]

Would really like to see something like this implemented in UriBuilder class as a constructor overload.

I was actually surprised to see the Query Property Type on UriBuilder was a simple string instead of some kind of collection that would support the Keys/Values of a typical query string parameters.

As query string parameters have become such a vital part of APIs these days, the extra effort required to parse them into a string or find hidden utility classes to complete the task is a bit absurd.

[xPaw]

In ASP.NET Core we have our own QueryBuilder type: https://docs.microsoft.com/en-us/aspnet/core/api/microsoft.aspnetcore.http.extensions.querybuilder.

This package is now deprecated. Building query strings (along with list support) is such a glaring omission from the overall uri related APIs in .NET.

For inspiration, URLSearchParams from javascript can be looked at, it works well.

[silkfire]

You're supposed to use HttpUtility.ParseQueryString("") to build correct query strings.

[xPaw]

You're supposed to use HttpUtility.ParseQueryString("")

This is not a good solution. The only reason it works is because it returns a private HttpQSCollection which is an implementation detail. And the ToString on this does not support repeated parameters like key[0]=val&key[1]=val