From 2936b37ad0bf57425f51b6281d57f1754b6e492e Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 7 Nov 2013 09:51:31 +1100 Subject: [PATCH 1/3] don't mutate a collection you don't own --- Octokit.Tests/Helpers/UriExtensionsTests.cs | 12 ++++++++++++ Octokit/Helpers/UriExtensions.cs | 10 +++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/Octokit.Tests/Helpers/UriExtensionsTests.cs b/Octokit.Tests/Helpers/UriExtensionsTests.cs index cf799589..c656e62f 100644 --- a/Octokit.Tests/Helpers/UriExtensionsTests.cs +++ b/Octokit.Tests/Helpers/UriExtensionsTests.cs @@ -77,6 +77,18 @@ namespace Octokit.Tests.Helpers Assert.True(actual.Query.Contains("page=2")); } + [Fact] + public void DoesNotAppendNewParameters() + { + var uri = new Uri("https://api.github.com/repositories/1/milestones?state=closed&sort=due_date&direction=asc&page=2"); + + var parameters = new Dictionary { { "state", "open" }, { "sort", "other" } }; + + uri.ApplyParameters(parameters); + + Assert.Equal(2, parameters.Count); + } + [Fact] public void EnsuresArgumentNotNull() { diff --git a/Octokit/Helpers/UriExtensions.cs b/Octokit/Helpers/UriExtensions.cs index 53acbc82..d3d3c102 100644 --- a/Octokit/Helpers/UriExtensions.cs +++ b/Octokit/Helpers/UriExtensions.cs @@ -12,6 +12,10 @@ namespace Octokit if (parameters == null || !parameters.Any()) return uri; + // to prevent values being persisted across requests + // use a temporary dictionary which combines new and existing parameters + IDictionary p = new Dictionary(parameters); + string queryString; if (uri.IsAbsoluteUri) { @@ -34,13 +38,13 @@ namespace Octokit foreach (var existing in existingParameters) { - if (!parameters.ContainsKey(existing.Key)) + if (!p.ContainsKey(existing.Key)) { - parameters.Add(existing); + p.Add(existing); } } - string query = String.Join("&", parameters.Select(kvp => kvp.Key + "=" + kvp.Value)); + string query = String.Join("&", p.Select(kvp => kvp.Key + "=" + kvp.Value)); if (uri.IsAbsoluteUri) { var uriBuilder = new UriBuilder(uri) From 8b5e004068c2d6023e6be05959092ac72213ffa5 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 7 Nov 2013 09:51:52 +1100 Subject: [PATCH 2/3] deleted invalid test --- Octokit.Tests/Helpers/UriExtensionsTests.cs | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/Octokit.Tests/Helpers/UriExtensionsTests.cs b/Octokit.Tests/Helpers/UriExtensionsTests.cs index c656e62f..15f55a59 100644 --- a/Octokit.Tests/Helpers/UriExtensionsTests.cs +++ b/Octokit.Tests/Helpers/UriExtensionsTests.cs @@ -36,20 +36,6 @@ namespace Octokit.Tests.Helpers Assert.Equal(new Uri("issues?foo=fooval&bar=barval", UriKind.Relative), uriWithParameters); } - [Fact(Skip="I don't believe this test is valid")] - public void OverwritesExistingParameters() - { - var uri = new Uri("https://example.com?crap=crapola"); - - var uriWithParameters = uri.ApplyParameters(new Dictionary - { - {"foo", "fooval"}, - {"bar", "barval"} - }); - - Assert.Equal(new Uri("https://example.com?foo=fooval&bar=barval"), uriWithParameters); - } - [Fact] public void DoesNotChangeUrlWhenParametersEmpty() { From 7cdb639551c989b164084f0026c2a3e74bdd16bb Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 7 Nov 2013 11:18:27 +1100 Subject: [PATCH 3/3] updated terrible test name --- Octokit.Tests/Helpers/UriExtensionsTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Octokit.Tests/Helpers/UriExtensionsTests.cs b/Octokit.Tests/Helpers/UriExtensionsTests.cs index 15f55a59..2ab07abb 100644 --- a/Octokit.Tests/Helpers/UriExtensionsTests.cs +++ b/Octokit.Tests/Helpers/UriExtensionsTests.cs @@ -64,7 +64,7 @@ namespace Octokit.Tests.Helpers } [Fact] - public void DoesNotAppendNewParameters() + public void DoesNotChangePassedInDictionary() { var uri = new Uri("https://api.github.com/repositories/1/milestones?state=closed&sort=due_date&direction=asc&page=2");