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 @@ public void AppendsParametersAsQueryString()
  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 @@ public void ThrowsExceptionWhenNullValueProvided()
  }
 
  [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 @@ public void DoesNotChangeUrlWhenParametersEmpty()
  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.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.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("repositories/1/milestones?state=open&sort=other&per_page=5&direction=asc&page=2", UriKind.Relative),
+ actual);
  }
 
  [Fact]
@@ -88,6 +126,18 @@ public void DoesNotChangePassedInDictionary()
  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,14 +25,19 @@ public static Uri ApplyParameters(this Uri uri, IDictionary<string, string> para
  // 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)
  {
  queryString = uri.Query;
  }
  else
  {
- var hasQueryString = uri.OriginalString.IndexOf("?", StringComparison.Ordinal);
  queryString = hasQueryString == -1
  ? ""
  : uri.OriginalString.Substring(hasQueryString);
@@ -65,7 +70,7 @@ public static Uri ApplyParameters(this Uri uri, IDictionary<string, string> para
  return uriBuilder.Uri;
  }
 
- return new Uri(uri + "?" + query, UriKind.Relative);
+ return new Uri(uriWithoutQuery + "?" + query, UriKind.Relative);
  }
  }
 }