Fix Uri.ApplyParameters() to handle relative Uri's that have existing parameters (#1649)

* Expand ApplyParameter() tests to reveal failure case (relative URI with existing query parameters)

* Ensure new Uri is created from base Uri without existing parameters present

Octokit.Tests/Helpers/UriExtensionsTests.cs

@@ -22,6 +22,20 @@ namespace Octokit.Tests.Helpers
                 Assert.Equal(new Uri("https://example.com?foo=foo%20val&bar=barval"), uriWithParameters);
             }
 
+            [Fact]
+            public void AppendsParametersAsQueryStringWithRelativeUri()
+            {
+                var uri = new Uri("issues", UriKind.Relative);
+
+                var uriWithParameters = uri.ApplyParameters(new Dictionary<string, string>
+                {
+                    {"foo", "fooval"},
+                    {"bar", "barval"}
+                });
+
+                Assert.Equal(new Uri("issues?foo=fooval&bar=barval", UriKind.Relative), uriWithParameters);
+            }
+
             [Fact]
             public void ThrowsExceptionWhenNullValueProvided()
             {
@@ -36,17 +50,16 @@ namespace Octokit.Tests.Helpers
             }
 
             [Fact]
-            public void AppendsParametersAsQueryStringToRelativeUri()
+            public void ThrowsExceptionWhenNullValueProvidedWithRelativeUri()
             {
-                var uri = new Uri("issues", UriKind.Relative);
+                var uri = new Uri("api/example", UriKind.Relative);
 
-                var uriWithParameters = uri.ApplyParameters(new Dictionary<string, string>
+                var parameters = new Dictionary<string, string>
                 {
-                    {"foo", "fooval"},
-                    {"bar", "barval"}
-                });
+                    {"foo", null }
+                };
 
-                Assert.Equal(new Uri("issues?foo=fooval&bar=barval", UriKind.Relative), uriWithParameters);
+                Assert.Throws<ArgumentNullException>(() => uri.ApplyParameters(parameters));
             }
 
             [Fact]
@@ -61,19 +74,44 @@ namespace Octokit.Tests.Helpers
                 Assert.Equal(uri, uriWithNullParameters);
             }
 
+            [Fact]
+            public void DoesNotChangeUrlWhenParametersEmptyWithRelativeUri()
+            {
+                var uri = new Uri("api/example", UriKind.Relative);
+
+                var uriWithEmptyParameters = uri.ApplyParameters(new Dictionary<string, string>());
+                var uriWithNullParameters = uri.ApplyParameters(null);
+
+                Assert.Equal(uri, uriWithEmptyParameters);
+                Assert.Equal(uri, uriWithNullParameters);
+            }
+
             [Fact]
             public void CombinesExistingParametersWithNewParameters()
             {
                 var uri = new Uri("https://api.github.com/repositories/1/milestones?state=closed&sort=due_date&direction=asc&page=2");
 
-                var parameters = new Dictionary<string, string> { { "state", "open" }, { "sort", "other" } };
-
+                var parameters = new Dictionary<string, string> { { "state", "open" }, { "sort", "other" }, { "per_page", "5" } };
+                
                 var actual = uri.ApplyParameters(parameters);
 
-                Assert.True(actual.Query.Contains("state=open"));
-                Assert.True(actual.Query.Contains("sort=other"));
-                Assert.True(actual.Query.Contains("direction=asc"));
-                Assert.True(actual.Query.Contains("page=2"));
+                Assert.Equal(
+                    new Uri("https://api.github.com/repositories/1/milestones?state=open&sort=other&per_page=5&direction=asc&page=2"),
+                    actual);
+            }
+
+            [Fact]
+            public void CombinesExistingParametersWithNewParametersToRelativeUri()
+            {
+                var uri = new Uri("repositories/1/milestones?state=closed&sort=due_date&direction=asc&page=2", UriKind.Relative);
+
+                var parameters = new Dictionary<string, string> { { "state", "open" }, { "sort", "other" }, { "per_page", "5" } };
+                
+                var actual = uri.ApplyParameters(parameters);
+
+                Assert.Equal(
+                    new Uri("repositories/1/milestones?state=open&sort=other&per_page=5&direction=asc&page=2", UriKind.Relative),
+                    actual);
             }
 
             [Fact]
@@ -88,6 +126,18 @@ namespace Octokit.Tests.Helpers
                 Assert.Equal(2, parameters.Count);
             }
 
+            [Fact]
+            public void DoesNotChangePassedInDictionaryForRelativeUri()
+            {
+                var uri = new Uri("/repositories/1/milestones?state=closed&sort=due_date&direction=asc&page=2", UriKind.Relative);
+
+                var parameters = new Dictionary<string, string> { { "state", "open" }, { "sort", "other" } };
+
+                uri.ApplyParameters(parameters);
+
+                Assert.Equal(2, parameters.Count);
+            }
+
             [Fact]
             public void EnsuresArgumentNotNull()
             {

Octokit/Helpers/UriExtensions.cs

@@ -25,6 +25,12 @@ namespace Octokit
             // use a temporary dictionary which combines new and existing parameters
             IDictionary<string, string> p = new Dictionary<string, string>(parameters);
 
+            var hasQueryString = uri.OriginalString.IndexOf("?", StringComparison.Ordinal);
+
+            string uriWithoutQuery = hasQueryString == -1
+                    ? uri.ToString()
+                    : uri.OriginalString.Substring(0, hasQueryString);
+
             string queryString;
             if (uri.IsAbsoluteUri)
             {
@@ -32,7 +38,6 @@ namespace Octokit
             }
             else
             {
-                var hasQueryString = uri.OriginalString.IndexOf("?", StringComparison.Ordinal);
                 queryString = hasQueryString == -1
                     ? ""
                     : uri.OriginalString.Substring(hasQueryString);
@@ -65,7 +70,7 @@ namespace Octokit
                 return uriBuilder.Uri;
             }
 
-            return new Uri(uri + "?" + query, UriKind.Relative);
+            return new Uri(uriWithoutQuery + "?" + query, UriKind.Relative);
         }
     }
 }